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

Not omit error in logger #1096

Merged
merged 1 commit into from
May 5, 2023

Conversation

itechdima
Copy link
Contributor

Add explicit errors as arguments so anyone can handle them in the logger implementation.

We stumbled across issue that during Kafka rebalancing we have plenty of different error logs (EOF, Unexpected EOF, not a leader, etc.). The only possible way to handle that noise is to add some middleware logic into our implementation of logger. So, to have better backward compatibility it's better to not check against string values, but against errors explicitly.

@marthjod
Copy link

Our goal here is to reduce alert noise for our alerts which refer to logged errors. A lot of errors we see are transient and can be treated with lower criticality in our case. As of now, we have no way of differentiating between errors we care about and those transient ones. To get a better handle on the different kinds of errors without hacky string matching, we would like to be able to check and filter via the actual error types in our middleware. This way, we can more cleanly filter and decide what to bubble up and what to treat as intermittent failure.

@itechdima
Copy link
Contributor Author

@nlsun @achille-roussel
could you please take a look?

@itechdima
Copy link
Contributor Author

@rhansen2
Maybe you can facilitate here? :)

reader.go Outdated
@@ -1346,7 +1346,7 @@ func (r *reader) run(ctx context.Context, offset int64) {

case errors.Is(err, UnknownTopicOrPartition):
r.withErrorLogger(func(log Logger) {
log.Printf("failed to read from current broker for partition %d of %s at offset %d, topic or parition not found on this broker, %v", r.partition, r.topic, toHumanOffset(offset), r.brokers)
log.Printf("failed to read from current broker for partition %d of %s at offset %d, topic or parition not found on this broker, %v: %v", r.partition, r.topic, toHumanOffset(offset), r.brokers, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like with these new formats we'll have
failed to read from current broker for partition %d of %s at offset %d, topic or parition not found on this broker, %v: Unknown Topic Or Partition

Could we possible reformat these a bit so we don't log redundant messages?

Potentially like
failed to read from current broker for partition %d of %s at offset %d, Unknown Topic Or Partition, %v

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the messages in logs

Add explicit errors as arguments so anyone can handle them in the logger implementation.
@itechdima itechdima requested a review from rhansen2 May 4, 2023 10:03
Copy link
Collaborator

@rhansen2 rhansen2 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 contribution!

@rhansen2 rhansen2 merged commit fcf85ac into segmentio:main May 5, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants