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

[WIP] slog handler #5195

Closed
wants to merge 34 commits into from
Closed

[WIP] slog handler #5195

wants to merge 34 commits into from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 1, 2024

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:

Outstanding tasks

  • Migrate from slogtest.TestHandler to slogtest.Run when support for Go 1.21 is dropped.
  • Implement the Enabled method.
  • Integrate the module version number into the Logger version using our build tooling and a Version function.
  • Limit the returned size of objects returned to the kvBufferPool (integration testing will be needed): otelslog: Limit the size of kvBuffer returned to the pool #5334

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 61.7%. Comparing base (6137d41) to head (20ce015).

Additional details and impacted files

Impacted file tree graph

@@           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     
Files Coverage Δ
bridges/sloghandler/version.go 100.0% <100.0%> (ø)
bridges/sloghandler/handler.go 91.5% <91.5%> (ø)

@MrAlias MrAlias changed the title [WIP] Initial slog handler [WIP] slog handler Mar 1, 2024
@MrAlias MrAlias force-pushed the sloghandler branch 2 times, most recently from 51829bd to 6da730f Compare March 7, 2024 17:53
bridges/sloghandler/handler.go Show resolved Hide resolved
record.SetBody(log.StringValue(r.Message))

const sevOffset = slog.Level(log.SeverityDebug) - slog.LevelDebug
record.SetSeverity(log.Severity(r.Level + sevOffset))
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Comment on lines 54 to 56
bridgeName,
log.WithInstrumentationVersion(bridgeVersion),
),
Copy link
Contributor Author

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.

Copy link
Member

@pellared pellared Mar 13, 2024

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.

Copy link
Contributor Author

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.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 12, 2024

Add the benchmarks to the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants