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

[pellared]

Showcase open-telemetry/opentelemetry-go-contrib#5586 (comment)

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

[codecov]

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

... and 2 files with indirect coverage changes

[pellared]

A very similar alternative would be to have the name as a separate parameter to indicate that it is required:

Suggested change

	Logger(cfg LoggerConfig) Logger
	}
	// LoggerConfig contains options for a [Logger].
	type LoggerConfig struct {
	// Ensure forward compatibility by explicitly making this not comparable.
	noCmp [0]func() //nolint: unused // This is indeed used.
	Name string
	Version string
	SchemaURL string
	Attributes attribute.Set
	Logger(name string, cfg LoggerConfig) Logger
	}
	// LoggerConfig contains options for a [Logger].
	type LoggerConfig struct {
	// Ensure forward compatibility by explicitly making this not comparable.
	noCmp [0]func() //nolint: unused // This is indeed used.
	Version string
	SchemaURL string
	Attributes attribute.Set

[pellared]

Closing per open-telemetry/opentelemetry-go-contrib#5586 (comment) . We may resurrect this if bridge implementation would give a feedback that it would be better to change the Bridge API.

CC @MrAlias

[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]

Reopening and making as draft per #5367 (comment).

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]

I plan to make some PoC bridge implementation for zap or/and logr just to validate the "logger" optimizations to make sure that this proposal is not needed.

EDIT: Upon quick look I think that a PoC for zap would be enough as it would be the most problematic one because the "logger name" is a on "log record level" in zap. See: https://pkg.go.dev/go.uber.org/zap/zapcore#Entry

EDIT2: Most probably using a sync.Map (logger name to logger) in otelzap would be the most pragmatic optimization we can do - we would not need to make any changes in the Bridge API.