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

Set up and use logrus logger in main #1819

Merged
merged 7 commits into from
Jan 4, 2024

Conversation

mikesep
Copy link
Contributor

@mikesep mikesep commented Jan 27, 2023

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.

@mikesep mikesep requested a review from a team as a code owner January 27, 2023 21:36
@mikesep mikesep marked this pull request as draft January 27, 2023 21:53
@mikesep
Copy link
Contributor Author

mikesep commented Jan 27, 2023

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.
@mikesep
Copy link
Contributor Author

mikesep commented Jan 31, 2023

I'm not seeing any log output in my cluster-based tests

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 sed, which seems to be doing more buffering now than before. Adding --unbuffered fixed the issue for me.)

Anyway, this PR should be good to go from what I can tell -- it's working correctly on my cluster so far.

@mikesep mikesep marked this pull request as ready for review January 31, 2023 15:30
cmd/proxy/actions/app.go Show resolved Hide resolved
cmd/proxy/main.go Outdated Show resolved Hide resolved
cmd/proxy/actions/app.go Outdated Show resolved Hide resolved
@mikesep
Copy link
Contributor Author

mikesep commented Feb 27, 2023

Sorry for the delay here -- hoping to get back to this this week.

@DrPsychick
Copy link
Member

Sorry for the delay here -- hoping to get back to this this week.

Hey @mikesep, any change you can get back to this?

@Ullaakut
Copy link
Collaborator

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.

@mikesep
Copy link
Contributor Author

mikesep commented Jan 2, 2024

Sorry for the delay here -- hoping to get back to this this week.

Hey @mikesep, any [chance] you can get back to this?

Hi folks, sorry the lack of progress here. I'm rereading what I did here and how logging is still used in the main branch to get back up to speed.

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.

I think the conservative approach is to lean towards finishing up this PR before considering the merits and costs of moving to slog and/or redesigning how Athens uses its pkg/log and pkg/errors packages.

* Prefer human-readable descriptions to method names
* Wrapped errors use gerund forms, e.g. "doing x: %w"
* Log traces start with a capital letter
Copy link
Contributor

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

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

LGTM

@DrPsychick DrPsychick merged commit 7284004 into gomods:main Jan 4, 2024
11 checks passed
@DrPsychick DrPsychick added this to the 0.13.1 milestone Jan 4, 2024
@mikesep mikesep deleted the logrus-logger-in-main branch January 4, 2024 15:26
@mikesep
Copy link
Contributor Author

mikesep commented Jan 4, 2024

Thanks for the reviews and for reminding me to get back to this! Sorry again for taking so long!

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.

5 participants