-
Notifications
You must be signed in to change notification settings - Fork 501
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
Set up and use logrus logger in main #1819
Conversation
This is working fine when I test locally, but I'm not seeing any log output in my cluster-based tests. Marking as draft until I figure out what's going on. |
Also return errors more consistently from other functions.
f0a4011
to
f5578fc
Compare
This was mostly unrelated to the changes in this PR, as I'd hoped/suspected. (Details: I happen to be post-processing Athens's logs using Anyway, this PR should be good to go from what I can tell -- it's working correctly on my cluster so far. |
Sorry for the delay here -- hoping to get back to this this week. |
Hey @mikesep, any change you can get back to this? |
Actually now conflicts with #1890 so we should either go all in with Logrus and uniformize it across the whole code base or make another PR and replace it with slog if it seems like a better alternative and worth the trouble of switching completely. |
Hi folks, sorry the lack of progress here. I'm rereading what I did here and how logging is still used in the
I think the conservative approach is to lean towards finishing up this PR before considering the merits and costs of moving to |
* Prefer human-readable descriptions to method names * Wrapped errors use gerund forms, e.g. "doing x: %w" * Log traces start with a capital letter
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.
LGTM
Thanks for the reviews and for reminding me to get back to this! Sorry again for taking so long! |
What is the problem I am trying to address?
Similar to #1817, this is part of an effort to get as much logging as possible to be consistently formatted by using the same (logrus) logger.
How is the fix applied?
This PR sets up and use logrus logger as early as possible in
main
.It also updates functions called by
main
to return errors more consistently.I did change some of the logging text strings to the style I personally prefer, but they're not critical to this PR, and we can revert them as needed.
What GitHub issue(s) does this PR fix or close?
None.