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

feat(logging): Allow configuring logging verbosity level #872

Merged
merged 10 commits into from
Nov 14, 2024

Conversation

garyhodgson
Copy link
Contributor

Summary

Introduces logr library as per #871

I'm not a Go Developer, so apologies if the attempt isn't very go-ish.

I couldn't see a way to use enums for mapping the log level names to logr.Level ints, so I made use of this library: https://hermannm.dev/enumnames

I performed a few manual tests, not sure how unit tests of log output would work.

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Updated documentation in README.md, if applicable.

Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Given that you've never written Go before, you're doing excellent work!

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@TwiN
Copy link
Owner

TwiN commented Sep 30, 2024

FYI, don't hesitate to let me know if you want me to add something else to the logr library.

@TwiN TwiN changed the title introduces TwiN/logr library feat(logging): Allow configuring logging verbosity level Sep 30, 2024
@TwiN TwiN added the feature New feature or request label Sep 30, 2024
@garyhodgson
Copy link
Contributor Author

Hi. All good suggestions, the changes to logr make it a easier to integrate 👍

As far as suggestions, I can imagine convenience methods such as isDebug() for logr.GetThreshold() == logr.LevelDebug checks would prove useful.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.53%. Comparing base (c6ff6ec) to head (6f63289).

Files with missing lines Patch % Lines
main.go 0.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
- Coverage   77.59%   77.53%   -0.06%     
==========================================
  Files          72       72              
  Lines        5775     5779       +4     
==========================================
  Hits         4481     4481              
- Misses       1078     1082       +4     
  Partials      216      216              

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

Comment on lines 72 to 74

logr.Debugf("[watchdog.execute] Monitoring group=%s; endpoint=%s", ep.Group, ep.Name)

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
logr.Debugf("[watchdog.execute] Monitoring group=%s; endpoint=%s", ep.Group, ep.Name)
logr.Debugf("[watchdog.execute] Monitoring group=%s; endpoint=%s", ep.Group, ep.Name)

main.go Outdated
@@ -23,6 +25,8 @@ func main() {
if err != nil {
panic(err)
}

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change

main.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
watchdog/watchdog.go Outdated Show resolved Hide resolved
@g-hodgson-tup
Copy link

Hi - requested changes have been pushed.

config.yaml Outdated Show resolved Hide resolved
watchdog/watchdog.go Outdated Show resolved Hide resolved
watchdog/watchdog.go Outdated Show resolved Hide resolved
Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@TwiN TwiN merged commit 8060a77 into TwiN:master Nov 14, 2024
1 check passed
@TwiN
Copy link
Owner

TwiN commented Nov 14, 2024

@garyhodgson FYI I decided to move log-level to an environment variable - see #895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants