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 bklog.G(ctx) instead of logrus directly #3705

Merged
merged 1 commit into from
Mar 15, 2023
Merged

use bklog.G(ctx) instead of logrus directly #3705

merged 1 commit into from
Mar 15, 2023

Conversation

coryb
Copy link
Collaborator

@coryb coryb commented Mar 12, 2023

This is a continuation of #2236

I noticed there were several lines in our logs that were missing traceID fields and found they were still using logrus directly.

I have modified a few apis to pass through the context, and in some cases (where a request context was not applicable) I used bklog.L although I could change that to use bklog.G(context.Background()) if that is preferable.

@coryb coryb force-pushed the bklog branch 3 times, most recently from 589dba3 to 4478171 Compare March 12, 2023 19:30
@coryb coryb requested a review from tonistiigi March 12, 2023 21:01
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Sorry, looks like this needs a rebase.

cc @morlay

As a follow-up would be good to get a linter rule that checks this (unless there are still valid exceptions).

@jedevc
Copy link
Member

jedevc commented Mar 15, 2023

For a linting rule, I think something like jedevc@6377bbb should do the job?

Signed-off-by: coryb <cbennett@netflix.com>
@coryb
Copy link
Collaborator Author

coryb commented Mar 15, 2023

For a linting rule, I think something like jedevc@6377bbb should do the job?

Thanks, I put in something similar, the line anchors ^ and $ I think were preventing matches in my testing, I also added a few more possible matches for logrus to restrict.

@coryb coryb merged commit 13ea5ae into moby:master Mar 15, 2023
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.

3 participants