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

Use custom logger to downgrade canceled context errors to warnings #2936

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

orouz
Copy link
Collaborator

@orouz orouz commented Jan 20, 2025

Summary of your changes

replace logp with clog (custom logger) that wraps Errorf in order to downgrade context.Canceled to warnings

the errors described in #1154 (table in google doc) are mostly the same ones shown in #2843, but all originate from the same reason - some ongoing request being canceled.

@mergify mergify bot assigned orouz Jan 20, 2025
Copy link

mergify bot commented Jan 20, 2025

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b clog upstream/clog
git merge upstream/main
git push upstream clog

Copy link

mergify bot commented Jan 20, 2025

This pull request does not have a backport label. Could you fix it @orouz? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
    NOTE: backport-v8.x has been added to help with the transition to the new branch 8.x.

@orouz orouz force-pushed the clog branch 5 times, most recently from f6db853 to d9a4656 Compare January 21, 2025 15:59
Comment on lines +34 to +37
if hasErrorType(context.Canceled, args...) {
l.Warnf(template, args...)
return
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is reason for this PR - downgrading errors to warnings when we see context canceled

Comment on lines +41 to +50
func (l *Logger) Named(name string) *Logger {
return &Logger{l.Logger.Named(name)}
}

func (l *Logger) WithOptions(options ...logp.LogOption) *Logger {
return &Logger{l.Logger.WithOptions(options...)}
}
func (l *Logger) With(args ...any) *Logger {
return &Logger{l.Logger.With(args...)}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we're using these methods from logp and to make types work i had to implement them so their usage returns our custom logger

the implementation just calls the original methods

and the original logger is still available at l.Logger

so i believe it's fine and poses no risk afaik

Comment on lines +60 to +68
// Check if the error is of the same type
if err, ok := arg.(error); ok && errors.Is(err, errorType) {
return true
}

// Check if the error message contains the error type string
if str, ok := arg.(string); ok && strings.Contains(str, errorTypeStr) {
return true
}
Copy link
Collaborator Author

@orouz orouz Jan 21, 2025

Choose a reason for hiding this comment

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

need to account for both these cases as we sometimes log Errorf("error: %v", err) and sometimes we log Errorf("error: ", err.Error())

@orouz orouz changed the title use custom logger Use custom logger to downgrade canceled context errors to warnings Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle context canceled errors on shutdown
1 participant