-
-
Notifications
You must be signed in to change notification settings - Fork 154
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): Support logfmt #2712
feat(logging): Support logfmt #2712
Conversation
✅ Deploy Preview for go-feature-flag-doc-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Dave Henderson <[email protected]>
4ead2ab
to
34d3b14
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2712 +/- ##
==========================================
+ Coverage 84.74% 84.78% +0.04%
==========================================
Files 111 111
Lines 5164 5180 +16
==========================================
+ Hits 4376 4392 +16
Misses 624 624
Partials 164 164 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
cmd/relayproxy/config/config.go
Outdated
if c.LogFormat != "" && c.LogFormat != "json" && c.LogFormat != "logfmt" { | ||
return fmt.Errorf("invalid log format %s", c.LogFormat) | ||
} |
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.
💣 chore: I have refactor this part to use a switch and lowercase the value.
I've also added a test to validate that we fail if we put something different.
Signed-off-by: Thomas Poignant <[email protected]>
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.
Super nice addition, it may be easier to read proper text logs than JSON you are right.
🤓 nitpick: I've added a small test and refactor a bit the validation part.
👏 praise: Thanks a lot also for taking the time to fix the TestLoggerOutput
.
I will put everything in the v1.39.0
version of GO Feature Flag.
Quality Gate passedIssues Measures |
Thanks for the quick review @thomaspoignant!! |
Description
Adds logfmt support through a new
logFormat
global config setting. I ended up needing to refactor things a bit.Also, the original
TestLoggerOutput
unit test actually wasn't testing the logger at all - that logger was being silently ignored. I fixed that test and merged it with theTestInitLogger
test.Closes issue(s)
Resolve #2711
Checklist
README.md
and/website/docs
)