opentelemetry-go
opentelemetry-go copied to clipboard
[Proposal] log: LoggerProvider.Logger to accept LoggerConfig instead of options
Showcase https://github.com/open-telemetry/opentelemetry-go-contrib/issues/5586#issuecomment-2114208299
It may occur that getting a logger for some bridges is going to be on the hot-path. E.g. in zap
LoggerNameis a field of the Entry. EachOnEmitmay need to get a logger if we do not want to store amap[string]log.Loggerin the bridge. We might prefer using structs instead of options pattern for getting a logger to decrease the amount of heap allocations.In
go.opentelemetry.io/otel/logwe could simply change theLoggerConfigto be a simple struct with exported fields and makeLoggerProvider.Loggerto acceptLoggerConfiginstead of options.
Basically the thesis from https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#passing-struct-as-parameter-to-loggerproviderlogger:
The performance of acquiring a logger is not as critical as the performance of emitting a log record.
may not be true (as we may want to acquire a logger before emitting each log record).
Codecov Report
Attention: Patch coverage is 90.47619% with 2 lines in your changes are missing coverage. Please review.
Project coverage is 84.5%. Comparing base (
bf06b80) to head (5e28602). Report is 24 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #5367 +/- ##
=======================================
- Coverage 84.6% 84.5% -0.1%
=======================================
Files 268 267 -1
Lines 17704 17673 -31
=======================================
- Hits 14982 14951 -31
Misses 2410 2410
Partials 312 312
| Files | Coverage Δ | |
|---|---|---|
| log/internal/global/log.go | 100.0% <100.0%> (ø) |
|
| log/logtest/recorder.go | 100.0% <100.0%> (ø) |
|
| log/noop/noop.go | 100.0% <100.0%> (ø) |
|
| sdk/log/provider.go | 100.0% <100.0%> (ø) |
|
| log/global/log.go | 66.6% <0.0%> (ø) |
Closing per https://github.com/open-telemetry/opentelemetry-go-contrib/issues/5586#issuecomment-2115452118 . We may resurrect this if bridge implementation would give a feedback that it would be better to change the Bridge API.
CC @MrAlias
If a user cares about the allocations for performance they can do similar things to the metric signal and cache option allocations outside of the hot-path.
I think that the cache would need to be synchronized to avoid race conditions and it would decrease the performance.
CC @MrAlias
Reopening and making as draft per https://github.com/open-telemetry/opentelemetry-go/pull/5367#issuecomment-2123147927.
Adding to blocked in the project to indicated that it is blocked until there is a use case that would support this change (e.g. ease of use in bridge implementations, better performance of bridge implementations).