opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

[Proposal] log: LoggerProvider.Logger to accept LoggerConfig instead of options

Open pellared opened this issue 1 year ago • 5 comments

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 LoggerName is a field of the Entry. Each OnEmit may need to get a logger if we do not want to store a map[string]log.Logger in 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/log we could simply change the LoggerConfig to be a simple struct with exported fields and make LoggerProvider.Logger to accept LoggerConfig instead 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).

pellared avatar May 16 '24 10:05 pellared

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

Impacted file tree graph

@@           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%> (ø)

... and 2 files with indirect coverage changes

codecov[bot] avatar May 16 '24 10:05 codecov[bot]

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

pellared avatar May 16 '24 17:05 pellared

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

pellared avatar May 21 '24 17:05 pellared

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).

pellared avatar May 22 '24 17:05 pellared