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 logging.FromContext instead of logging.Default where possible #6312

Merged
merged 3 commits into from
Aug 7, 2023

Conversation

arielshaqed
Copy link
Contributor

Loggers created using logging.FromContext include a request ID field,
allowing readers to thread together actions performed for the same request.
Also in future they allow us to add additional fields, for example to
distinguish S3 from lakeFS API operations deep inside the call tree.

Rename logging.Default to something more descriptive but perhaps less
pleasant, to encourage calling logging.FromContext.

Fixes #2682.

Loggers created using `logging.FromContext` include a request ID field,
allowing readers to thread together actions performed for the same request.
Also in future they allow us to add _additional_ fields, for example to
distinguish S3 from lakeFS API operations deep inside the call tree.

Fixes #2682.
This name is longer and more descriptive, to encourage using
`logging.Default()`.
@arielshaqed arielshaqed requested a review from N-o-Z August 3, 2023 09:12
@arielshaqed arielshaqed added bug Something isn't working include-changelog PR description should be included in next release changelog tech-debt labels Aug 3, 2023
Not part of the same project, so missed by my LSP-based refactoring tools.
@arielshaqed
Copy link
Contributor Author

Reviewers, this one is probably easiest to discuss by commit:

  • da8b2c0 is all about using logging.FromContext(ctx) instead of logging.Default(). We place fields (request_id) on the context, and this is the only foolproof way to determine the flow of logs for a particular request.
  • d8530cd and 666e0c4 try to make it harder to use logging.Default() in future, by changing its name to be less appealing. This is intentional - whenever a context is available, logging.FromContext is the right choice!

@arielshaqed arielshaqed requested a review from nopcoder August 3, 2023 09:33
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

The adapter use of ctx for logging is great.
But renaming the Default() to ContextUnavailable() looks like we try to force the use of context why we ask for default logger.
Can we rename it back?

@arielshaqed
Copy link
Contributor Author

The adapter use of ctx for logging is great. But renaming the Default() to ContextUnavailable() looks like we try to force the use of context why we ask for default logger. Can we rename it back?

Yes, indeed! I really want to make it unpleasant to use a logger without providing a context. It is almost always correct to provide a context to get a logger.

Would you be happier if I improved its godoc?

@arielshaqed arielshaqed requested a review from nopcoder August 6, 2023 12:45
@nopcoder
Copy link
Contributor

nopcoder commented Aug 6, 2023

The adapter use of ctx for logging is great. But renaming the Default() to ContextUnavailable() looks like we try to force the use of context why we ask for default logger. Can we rename it back?

Yes, indeed! I really want to make it unpleasant to use a logger without providing a context. It is almost always correct to provide a context to get a logger.

Would you be happier if I improved its godoc?

No need to improve the godocs - approved.
Will try to think of a better way of using it.
Usually I'll have a logger in place and just need to pass the context - don't want that to be mix with getting a fresh logger from context.
If I'll have something better I'll suggest in a different PR.

@arielshaqed
Copy link
Contributor Author

The adapter use of ctx for logging is great. But renaming the Default() to ContextUnavailable() looks like we try to force the use of context why we ask for default logger. Can we rename it back?

Yes, indeed! I really want to make it unpleasant to use a logger without providing a context. It is almost always correct to provide a context to get a logger.
Would you be happier if I improved its godoc?

No need to improve the godocs - approved. Will try to think of a better way of using it. Usually I'll have a logger in place and just need to pass the context - don't want that to be mix with getting a fresh logger from context. If I'll have something better I'll suggest in a different PR.

Thanks... pulling!

I've been thinking about your idea. The point is that we already had Logger.WithContext: it adds the context onto an existing logger. But nobody used it! One way to force it would be to call Logger-without-context "LoggerBuilder" or "LoggerTemplate", and not have any logging methods on it. And you'd need to call "LoggerBuilder.WithTemplate" to get a Logger. Anyway, that's for another day.

@arielshaqed arielshaqed merged commit c73c228 into master Aug 7, 2023
@arielshaqed arielshaqed deleted the chore/2682-more-context-logs branch August 7, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working include-changelog PR description should be included in next release changelog tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put request_id on (almost) every log
2 participants