Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(output.kinesis): Log kinesis consumer events #15843

Merged
merged 7 commits into from
Sep 16, 2024

Conversation

mskonovalov
Copy link
Contributor

@mskonovalov mskonovalov commented Sep 4, 2024

Summary

Currently we have 3 levels of abstraction:
telegraf output kinesis plugin -> kinesis-consumer lib -> aws sdk

Logging is happening only on Telegraf's level. So if there are some warnings or some debug logging on other levels, it cannot be enabled ATM.

In this PR I'm wrapping telegraf logger to comply with interfaces of kinesis-consumer and aws-sdk loggers and pass it when initializing the kinesis client.

For kinesis-consumer there is only a single Log function, so I map it to the DEBUG level.

Also enabling logging of retries in aws sdk.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15854

@telegraf-tiger telegraf-tiger bot added chore plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Sep 4, 2024
Copy link
Member

@DStrand1 DStrand1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I just have one comment for now on the code, but could you also open an issue for this PR to resolve? Thanks!

plugins/inputs/kinesis_consumer/kinesis_consumer.go Outdated Show resolved Hide resolved
@DStrand1 DStrand1 self-assigned this Sep 4, 2024
@DStrand1 DStrand1 assigned srebhan and unassigned DStrand1 Sep 11, 2024
@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 12, 2024
@srebhan srebhan changed the title chore(output.kinesis): log kinesis consumer events chore(output.kinesis): Log kinesis consumer events Sep 12, 2024
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mskonovalov for your contribution! I would mark the inner logs of the library with log-level "trace" to be able to selectively enable those as I don't think this is something users need to see all the time!?

plugins/inputs/kinesis_consumer/kinesis_consumer.go Outdated Show resolved Hide resolved
plugins/inputs/kinesis_consumer/kinesis_consumer.go Outdated Show resolved Hide resolved
Comment on lines 103 to 110
switch classification {
case logging.Debug:
t.Logger.Debugf(format, v...)
case logging.Warn:
t.Logger.Warnf(format, v...)
default:
t.Logger.Infof(format, v...)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use log-level Trace for all those and add the classification as prefix to the message. This way we can control if we want to see the inner logs of the library or not.

Copy link
Contributor Author

@mskonovalov mskonovalov Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I believe it is better to map Warn inside the library to Warn here as well, though Debug and Info I guess is OK to map to Trace.
But it is up to you as a maintainer. So let me know wdyt and I can fix accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I didn't get what you mean here :(
If I change it to Trace, then the prefix will be TRACE, so if I add classification as a prefix it will look like TRACE DEBUG <message>. I guess this will be confusing. Or maybe I'm missing something

Copy link
Member

@srebhan srebhan Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mskonovalov in normal operations it should not be necessary to see the library-internal log messages! If you map those to e.g. WARN everybody will now see library-internal warnings without a sensible way to ignore those. What I want is that the user CAN enable those log-messages setting the trace level for the plugin in Telegraf if he whishes to see them. You can keep the classification information by prepending the library message like

	switch classification {
	case logging.Debug:
		format  = "DEBUG " + format
	case logging.Warn:
		format  = "WARN" + format
	case logging.Info:
		format  = "INFO " + format
	}
	t.Logger.Tracef(format, v...)

which will produce

<timestamp> T! WARN this is an internal warn message

in case of a warning...

Copy link
Contributor Author

@mskonovalov mskonovalov Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I can make this change.
Although, I'd just mention that I don't fully agree.
If we are using the underlying library, there are not a lot of options how the library can notify us that something is wrong:

  1. of course, if the issue is critical - it will return the error
  2. if it is just some informational messages, then it is fine to put it under Trace level
  3. But what if it is in between: not critical to return error but still something that need to be displayed. E.g. library that writes into some storage can only do it after 3rd retry for every write. The write is successful so it is not returning error, but if we map WARN messages to TRACE level we will never see that something is not working properly.

So I still believe for WARN case it is better to allow to write it to WARN Telegraf log.
But it is up to you to decide.
Though I agree we need to hope that the lib author put only meaningful WARN messages and didn't make it too noisy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well TBH if a library requires logging to propagate important information there is something wrong with the library. A library is an abstraction that should allow the user to use the functionality without knowledge of the internal implementation. On error the library should return an error. If something goes wrong the library should either return some information and let the caller decide on how to continue or return an error and provide means to retry accepting the issue.

So at best the logging serves the purpose of debugging some connection issues or other trouble but this should not be necessary in normal working conditions.

@srebhan srebhan added the waiting for response waiting for response from contributor label Sep 13, 2024
@telegraf-tiger
Copy link
Contributor

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mskonovalov!

@srebhan srebhan removed the waiting for response waiting for response from contributor label Sep 16, 2024
@srebhan srebhan assigned DStrand1 and unassigned srebhan Sep 16, 2024
@srebhan
Copy link
Member

srebhan commented Sep 16, 2024

@DStrand1 assigning it back to you if you want to have another short look as some of the code changed. Feel free to directly merge the PR!

@DStrand1 DStrand1 merged commit 336a5e2 into influxdata:master Sep 16, 2024
27 checks passed
@github-actions github-actions bot added this to the v1.32.1 milestone Sep 16, 2024
@mskonovalov mskonovalov deleted the kinesis-log branch September 17, 2024 06:23
srebhan pushed a commit that referenced this pull request Oct 7, 2024
asaharn pushed a commit to asaharn/telegraf that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log Kinesis consumer events
3 participants