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

Fix/cluster-log-formatting #4759

Merged
merged 6 commits into from
Apr 24, 2024
Merged

Fix/cluster-log-formatting #4759

merged 6 commits into from
Apr 24, 2024

Conversation

nathanwilk7
Copy link
Contributor

@nathanwilk7 nathanwilk7 commented Apr 24, 2024

What's being changed:

The raft logging was recently updated to use logrus instead of slog (see #4753). However, most uses of the logger in the cluster/ directory (and some outside of that directory) were not updated to use the WithField (and similar functions) functionality of logrus.

For example, without this PR some of the logs look like this (note that all the strings are concatenated):

INFO[0000] tcp transportaddress192.168.0.14:8300tcpMaxPool3tcpTimeout10s

After this PR the logs will look like this:

INFO[0000] tcp transport                                 address="192.168.0.14:8300" tcpMaxPool=3 tcpTimeout=10s

This PR updates (most of) the logger usage following these conventions:

  1. Use WithField for dynamic values.
  2. If there are 2+ fields, use WithFields instead of WithField.
  3. For errors, use WithError and log a static error message.
  4. Only use formatting (e.g.: Infof instead of Info) when needed.

It's likely that I missed some poorly formatted logs, but hopefully I found the majority of them (used a combo of grepping and following references). Any bad logging calls I missed can be fixed in follow up PRs.

This PR should be a fairly low risk change.

Review checklist

  • Documentation has been updated, if necessary. Link to changed documentation:
  • Chaos pipeline run or not necessary. Link to pipeline:
  • All new code is covered by tests where it is reasonable.
  • Performance tests have been run or not necessary.

@nathanwilk7 nathanwilk7 marked this pull request as ready for review April 24, 2024 16:54
cluster/store/store.go Outdated Show resolved Hide resolved
moogacs
moogacs previously approved these changes Apr 24, 2024
Copy link
Contributor

@moogacs moogacs left a comment

Choose a reason for hiding this comment

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

LGTM, nice refactor :shipit:

usecases/cluster/delegate.go Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Apr 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@nathanwilk7 nathanwilk7 merged commit 7076751 into main Apr 24, 2024
41 checks passed
@nathanwilk7 nathanwilk7 deleted the fix/cluster-log-formatting branch April 24, 2024 17:53
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.

2 participants