-
Notifications
You must be signed in to change notification settings - Fork 25
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
RHCLOUD-36895 | refactor: reduce logging noise #241
RHCLOUD-36895 | refactor: reduce logging noise #241
Conversation
The log messages about the ignored committed offsets are not really useful unless we are debugging problems with that. Therefore, we could simply leave an error log message for when the offset committing goes wrong, and a debug one when we really want to know that offsets are being committed. RHCLOUD-36895
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I left a couple of suggestions.
Thank you for digging into this!
internal/kafka/kafka.go
Outdated
break | ||
} | ||
|
||
l.Log.Debugf("Ignored OffsetsComitted: %#v", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about making this a trace level log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even better. Done!
internal/kafka/kafka.go
Outdated
default: | ||
l.Log.Infof("Ignored %v\n", e) | ||
l.Log.Infof("Ignored %#v\n", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a bit more context to this log message? Maybe something like "Ignored kafka event"? I was a bit stumped the first time I saw the original log message. It took me a minute to figure out what I was seeing getting logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea too! Done!
RHCLOUD-36895
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
What?
Puts the "Ignored OffsetCommitted" error messages under the "debug" log level.
Why?
The log messages about the ignored committed offsets are not really useful unless we are debugging problems with that. Therefore, we could simply leave an error log message for when the offset committing goes wrong, and a debug one when we really want to know that offsets are being committed.
Jira ticket
[RHCLOUD-36895]