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 logr with zap throughout #4688

Merged
merged 27 commits into from
Oct 13, 2023
Merged

Use logr with zap throughout #4688

merged 27 commits into from
Oct 13, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Sep 20, 2023

What this PR does / why we need it:

Converts pure logrus subsystems to logr.

Uses zapr+zap in place of logrusr+logrus to create the logr.Logger.

Which issue this PR fixes:

Makes logging consistent throughout the project, except for the odd KLog line from client-go and one hack script that I don't really care about.

Allows swapping out logging engines without touching individual log lines.

Fix #1893

Special notes for your reviewer:

Review open for code. Want to discuss the below in sync before merging or building additional functionality.

  • There is no equivalent to the logrus console format available in zap without rolling our own, though there's an open community PR we could build off. This has been left with the zap default (tab-delimited, field names omitted) with JSON as the designated "feed it into your structured log SIEM" option.
  • Hopes for zapr handling verbosity to level conversions were a bit overly optimistic. The truth is kinda weird. I've set the logger to omit the level and use the actual verbosity instead. We could reasonably re-enable level with the understanding that there's no longer WARN, since neither we nor controller-runtime use levels greater than 1 (debug, effectively) often, so the info -> debug -> info again progression doesn't really show up. We could try fixing that upstream.
  • I added superfluous verbosity setters (V(util.InfoLevel).Info() and the like) to log lines I converted. These don't do anything useful at present, but I figure it lets us change them later via the constants if we decide to do something fancier with the verbosity levels.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Attention: 87 lines in your changes are missing coverage. Please review.

Comparison is base (7c2a104) 77.9% compared to head (a7f449d) 77.7%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4688     +/-   ##
=======================================
- Coverage   77.9%   77.7%   -0.2%     
=======================================
  Files        163     162      -1     
  Lines      18537   18471     -66     
=======================================
- Hits       14451   14363     -88     
- Misses      3275    3293     +18     
- Partials     811     815      +4     
Files Coverage Δ
internal/admission/validation/ingress/ingress.go 82.3% <ø> (-1.0%) ⬇️
internal/dataplane/configfetcher/config_fetcher.go 93.8% <100.0%> (ø)
internal/dataplane/failures/failures.go 100.0% <100.0%> (ø)
internal/dataplane/kongstate/route.go 97.5% <100.0%> (ø)
internal/dataplane/parser/backendref.go 100.0% <100.0%> (ø)
internal/dataplane/parser/ingressrules.go 95.1% <100.0%> (-0.1%) ⬇️
internal/dataplane/parser/translate_utils.go 93.9% <ø> (ø)
internal/dataplane/sendconfig/backoff_strategy.go 84.6% <100.0%> (ø)
...nal/dataplane/sendconfig/config_change_detector.go 100.0% <100.0%> (ø)
internal/dataplane/sendconfig/inmemory.go 85.7% <100.0%> (ø)
... and 32 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rainest rainest force-pushed the feat/zap-revamp branch 3 times, most recently from 1b009f2 to 79e166f Compare September 22, 2023 23:36
@rainest rainest added the do not merge let the author merge this, don't merge for them. label Sep 25, 2023
@rainest rainest marked this pull request as ready for review September 25, 2023 19:34
@rainest rainest requested a review from a team as a code owner September 25, 2023 19:34
Copy link
Member

@programmer04 programmer04 left a comment

Choose a reason for hiding this comment

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

Great effort (I can't wait for the merge)🎖️

FYI: in go-logr docs, there is a good go-logr vs. slog, worth checking. slog is less feature reach

CHANGELOG.md Outdated Show resolved Hide resolved
internal/admission/validation/ingress/ingress.go Outdated Show resolved Hide resolved
internal/dataplane/failures/failures_test.go Outdated Show resolved Hide resolved
internal/dataplane/kongstate/route.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translate_udproute.go Outdated Show resolved Hide resolved
internal/util/logging.go Show resolved Hide resolved
internal/util/logging.go Show resolved Hide resolved
internal/manager/setup.go Show resolved Hide resolved
Replace use of logrus with logr, and use zap as the underlying engine
instead of logrus. The controller now uses a single logging interface
throughout.

As logr does not use semantic log levels, controller log level options
are now collapsed into trace, debug, info, and errors only. Warn is
identical to info.

Console formatting uses zap's tab-delimited, no field name format. JSON
formatting should be compatible with the old logrus format for existing
fields.

Remove the log stifler, which was unused. zap has built-in sampling if
we wish to use it, though it is not enabled, to match previous behavior.
Fix TODOs or remove note-only TODOs.
@rainest
Copy link
Contributor Author

rainest commented Sep 29, 2023

From sync/other feedback:

  • Error() calls no longer explicitly indicate an error V level.
  • Restored the level key. We'd need to remove it or change something in libraries to use more verbosity levels, but we're going to follow controller-runtime's basic info/error/debug, where the zapr level handling is correct.
  • Removed the redundant config log levels. They're just a minor breaking change now.
  • Added a warning key to allow filtering those still.

@rainest rainest removed the do not merge let the author merge this, don't merge for them. label Sep 29, 2023
randmonkey
randmonkey previously approved these changes Oct 11, 2023
internal/store/store.go Outdated Show resolved Hide resolved
internal/store/store.go Outdated Show resolved Hide resolved
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

Just some nits and the conflicts (once again) and I believe this is ready 👍

randmonkey
randmonkey previously approved these changes Oct 12, 2023
pmalek
pmalek previously approved these changes Oct 12, 2023
@pmalek
Copy link
Member

pmalek commented Oct 12, 2023

Might want to adjust the PR title to e.g. adhere to conventional commits.

@rainest
Copy link
Contributor Author

rainest commented Oct 12, 2023

Might want to adjust the PR title to e.g. adhere to conventional commits.

I override the squash commit title and message if I use it instead of rebase and merge.

@rainest rainest requested a review from pmalek October 12, 2023 21:01
@rainest rainest merged commit 6b2ada6 into main Oct 13, 2023
34 checks passed
@rainest rainest deleted the feat/zap-revamp branch October 13, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spike: dual logging implementations in Controller Manager
4 participants