-
Notifications
You must be signed in to change notification settings - Fork 1
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: fields for labels and non-sugared structured logging #202
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #202 +/- ##
==========================================
+ Coverage 79.31% 79.50% +0.18%
==========================================
Files 70 71 +1
Lines 5222 5342 +120
==========================================
+ Hits 4142 4247 +105
- Misses 876 890 +14
- Partials 204 205 +1 ☔ View full report in Codecov by Sentry. |
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.
sorry for delaying my review, forgot to publish my comments 😅
error error | ||
} | ||
|
||
func (f Field) Name() string { return f.name } |
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.
it would be nice to add some coverage for this file
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.
Doesn't Test_Logger_NonSugared
provide enough coverage? What other scenarios do you have in mind? What if I just add more logs there with all the types like time, duration, error, etc?
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.
The following seem to be uncovered right now:
- Field#Name
- Field#Value
- Field#toZap (partial)
- NewField
- New{Float|Time|Duration|Error}Field
- Expand
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.
You're right, I got used to have the CodeCov report in the Files changed
tab here in the Pull Request and somehow it isn't showing here. We do have the report above though 👍
Description
There has been some work in the rudder-observability-kit to expose some common labels that we can use across our projects to make sure that we are consistent with them while using structured logging.
You can find such labels here.
In this PullRequest I'm doing mostly two things:
logger.Field
to be used with the non-sugared structured logginglog.Infow("foo", field.Name(), field.Value())
, also seeExpand
function in this PRLinear Ticket
< Linear_Link >
Related PRs
For a showcase about how this can be used check the Pull Requests below:
TODOs
Security