-
Notifications
You must be signed in to change notification settings - Fork 589
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
[WIP] slog handler #5195
[WIP] slog handler #5195
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5195 +/- ##
=======================================
+ Coverage 61.2% 61.7% +0.4%
=======================================
Files 185 187 +2
Lines 11207 11375 +168
=======================================
+ Hits 6865 7019 +154
- Misses 4142 4153 +11
- Partials 200 203 +3
|
51829bd
to
6da730f
Compare
Don't measure testing allocation
Refactor so attribute interactions are standardized using the kvBuffer type.
These are checked by slog.Logger
record.SetBody(log.StringValue(r.Message)) | ||
|
||
const sevOffset = slog.Level(log.SeverityDebug) - slog.LevelDebug | ||
record.SetSeverity(log.Severity(r.Level + sevOffset)) |
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.
Should we also set the SeverityText
to the same values as https://cs.opensource.google/go/go/+/refs/tags/go1.22.1:src/log/slog/level.go;l=50-58?
Should it be configurable via an option?
If so then we would just need to make sure it does not cause any heap allocation.
TBH, I am not sure what is even the use case of having/using SeverityText
. The specification does not say when/how it should be used.
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.
Yeah, I think there is some confusion in general about this: open-telemetry/opentelemetry-specification#3933
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.
Let's track with an issue.
bridges/sloghandler/handler.go
Outdated
bridgeName, | ||
log.WithInstrumentationVersion(bridgeVersion), | ||
), |
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.
These need to likely be the package/module name, not the bridge name: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#get-a-logger
The bridge name and version could be added as an attribute here. More investigation is required.
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.
Maybe the bridge should simply accept log.Logger
instead of log.LoggerProvider
? Then the user would have full control what is the instrumentation scope.
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.
Switched to accepting a logger.
Add the benchmarks to the PR description. |
Use a default Logger unless the user overrides.
This provides an overview of how to add the slog bridge. This PR as a whole is too large to ask review of. The plan to break this into smaller sections is as follows:
Option
Handler
Handler.Handle
Handler.Enabled
Handler.WithAttrs
Handler.WithGroup
New
WithAttrs
,WithGroup
, andconvertRecord
while addingslogtest
testing: Implement the slog Handler functionality #5314Outstanding tasks
slogtest.TestHandler
toslogtest.Run
when support for Go 1.21 is dropped.Enabled
method.Logger
version using our build tooling and aVersion
function.kvBufferPool
(integration testing will be needed): otelslog: Limit the size of kvBuffer returned to the pool #5334