sdk/log: Add BenchmarkLoggerProviderLoggerSame

[pellared]

What

Benchmark that demonstrates the performance of reacquiring the same logger.

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/log
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
BenchmarkLoggerProviderLoggerSame-16    	13867015	        92.33 ns/op	      24 B/op	       1 allocs/op

Why

I want to add this benchmark as I want to showcase that passing an option causes a heap allocation. In order to not have this heap allocation the caller would need to create a cache which would increase the memory footprint.

Some bridge implementations (e.g. for zap; see here) would have to get a logger for each log record. I prefer if bridges could directly call logger.Logger("name", LoggerConfig{ InstrumentationVersion: "v1.2.3" } that would not cause a heap allocation instead of e.g. caching passed options (or loggers). Caching on the bridge side will increase the memory overhead and the complexity of the bridge implementation. It makes it also easier to use the Bridge API in an non-efficient way. While (I think) I know how to handle the problem in otelzap becasue the zapcore.Entry only has a "logger name" the problem would be if logging library has e.g. a "version" in its log entry. Therefore, I would feel safer if we would not use options as we are never sure if these statements from design doc are good:

The performance of acquiring a logger is not as critical as the performance of emitting a log record.
[...]
The bridge implementation should reuse loggers whenever possible.

#5367 looks to be a more performance-robust API. The Bridge API is intended to be used to implement bridges. Applications should not use it directly. Therefore, I think that we can have a different design from Trace and Metrics API. We already have a different design for Processor and "Record" for performance reasons.

Remarks

Even if we do not decide to go for #5367, I think this benchmark is useful to demonstrate to performance of reacquiring the logger when options are created directly in the invocation.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (f2bc57b) to head (5243f76).

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5599     +/-   ##
=======================================
- Coverage   84.5%   84.5%   -0.1%     
=======================================
  Files        271     271             
  Lines      22427   22427             
=======================================
- Hits       18970   18969      -1     
- Misses      3124    3125      +1     
  Partials     333     333             

see 1 file with indirect coverage changes

[dmathieu]

Approving the benchmark per-se, will leave another comment as well.

[dmathieu]

If we provide p.Logger(name, log.LoggerConfig{}), won't the bridge still have to cache the config (or recreate it every time, and create an allocation)?

Then, what would be the difference between:

p.LoggerFromConfig(name, cachedConfig)

Versus:

p.Logger(name, cachedListOfOptions...)

In both cases, the bridge has to cache a config/list of options.

[pellared]

Config passed by value will be allocated on stack which is cheap - there would be no need to cache the config. I can make a draft PR once the benchmark is merged.

[MrAlias]

Similar to our metric benchmarks, this passed option should be allocated outside of the evaluation loop. Otherwise, we are measuring the inefficient misuse of the API.

[pellared]

Otherwise, we are measuring the inefficient misuse of the API.

This is the whole point of the benchmark is to show that such usage introduces a heap allocation. I find it is too easy to use the API in an inefficient way,

I think that it would be better if the user would not need to preallocate instrumentation version in order not introduce a heap allocation.

It would be better if "strightforward" usage (like inlining options) would not introduce performance overhead. I prefer to have an API which makes writing inefficient code harder. At the same time the most straightforward usage should be performant.

[pellared]

Maybe I should add a comment like: "Showcase that not pre-allocating the options slice causes a heap allocation introduced by the compiler"?

[MrAlias]

If that is the case, then this seems like a re-hash of the already concluded extensive conversation about how we handle configuration:

• https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#configuration
• Update Contributing style guide section #971
• Unify Configuration and Initialization #536

Additionally, since we have already discussed this specific detail for the metric signal¹² extensively, unless there are new developments, I would prefer to not have the same discussion again. Especially since there is are now 2 stable signals that have set our precedence and a documented policy.

Footnotes

1. https://github.com/open-telemetry/opentelemetry-go/pull/3971#discussion_r1163272609 ↩

2. topic of discussion in the April 13 2023 SIG meeting: https://docs.google.com/document/d/1E5e7Ld0NuU1iVvf-42tOBpu2VBBLYnh73GJuITGJTTU/edit#heading=h.9lojwomaurj2 ↩

[pellared]

I am fine using options for things bootstrapping the SDK (creating exporters, providers, processors).

However, I find that using options on code that may be on the hot-path makes it too easy for the callers to write inefficient code. Take notice that log.Record already diverges from these guidelines. Maybe, we should work on an alternate guideline for handling configuration for code that we asses that may be on the hot-path (e.g. when recording a value)?

I also think that there is very low probability that we would need to create logger provider options on the fly in the bridges. If it would occur, we can always create a new API.

I am closing this PR and creating a new issue. Thank you for your insight.