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

Logs Bridge API prototype

Open pellared opened this issue 9 months ago • 9 comments

Towards https://github.com/open-telemetry/opentelemetry-go/issues/4696

The design doc PR: https://github.com/open-telemetry/opentelemetry-go/pull/4809

The PR contains a prototype implementation (proposed API and prototypes of its implementation and log bridges for slog, logr) with benchmarks.

Current benchmark results:

$ go test -run=^$ -bench=.
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/log/internal
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
BenchmarkEmit/noop/no_attrs-16          481625260                2.357 ns/op           0 B/op          0 allocs/op
BenchmarkEmit/noop/3_attrs-16           172127840               11.77 ns/op            0 B/op          0 allocs/op
BenchmarkEmit/noop/5_attrs-16           66471745                17.22 ns/op            0 B/op          0 allocs/op
BenchmarkEmit/noop/10_attrs-16           6897639               157.6 ns/op           208 B/op          1 allocs/op
BenchmarkEmit/noop/40_attrs-16           1613598               863.9 ns/op          1408 B/op          1 allocs/op
BenchmarkEmit/writer/no_attrs-16        19277108                65.10 ns/op           16 B/op          1 allocs/op
BenchmarkEmit/writer/3_attrs-16          6325507               197.1 ns/op            48 B/op          4 allocs/op
BenchmarkEmit/writer/5_attrs-16          5215214               213.0 ns/op            48 B/op          4 allocs/op
BenchmarkEmit/writer/10_attrs-16         2246091               496.2 ns/op           296 B/op          8 allocs/op
BenchmarkEmit/writer/40_attrs-16          539629              2130 ns/op            1736 B/op         26 allocs/op
BenchmarkSlog/no_attrs-16                9658542               132.0 ns/op             0 B/op          0 allocs/op
BenchmarkSlog/3_attrs-16                 5917756               196.9 ns/op             0 B/op          0 allocs/op
BenchmarkSlog/5_attrs-16                 5342199               218.0 ns/op             0 B/op          0 allocs/op
BenchmarkSlog/10_attrs-16                1227394               884.2 ns/op           816 B/op          5 allocs/op
BenchmarkSlog/40_attrs-16                 212421              4966 ns/op            6624 B/op          8 allocs/op
BenchmarkLogr/no_attrs-16               197448330                6.707 ns/op           0 B/op          0 allocs/op
BenchmarkLogr/3_attrs-16                 7464216               142.7 ns/op           128 B/op          4 allocs/op
BenchmarkLogr/5_attrs-16                 5563734               211.2 ns/op           208 B/op          5 allocs/op
BenchmarkLogr/10_attrs-16                1378154               841.4 ns/op          1024 B/op         13 allocs/op
BenchmarkLogr/40_attrs-16                 230208              4922 ns/op            6880 B/op         40 allocs/op
PASS
ok      go.opentelemetry.io/otel/log/internal   30.867s

pellared avatar Nov 17 '23 10:11 pellared

@open-telemetry/go-instrumentation-approvers The docs/design/log-api.md is ready for initial review.

pellared avatar Nov 17 '23 12:11 pellared

It may also be good to document the package structure.

dashpole avatar Nov 17 '23 14:11 dashpole

It may also be good to document the package structure.

For bridge API it would only be

  • go.opentelemetry.io/otel/log
  • go.opentelemetry.io/otel/log/embedded
  • go.opentelemetry.io/otel/log/noop

Do you agree?

pellared avatar Nov 17 '23 14:11 pellared

Do you agree?

Yes. Just suggesting we document it, since we often decide to revisit it

dashpole avatar Nov 17 '23 14:11 dashpole

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (11ebd19) 82.6% compared to head (e89444d) 82.6%. Report is 30 commits behind head on main.

:exclamation: Current head e89444d differs from pull request most recent head 147762f. Consider uploading reports for the commit 147762f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4725     +/-   ##
=======================================
- Coverage   82.6%   82.6%   -0.1%     
=======================================
  Files        232     241      +9     
  Lines      18870   19265    +395     
=======================================
+ Hits       15603   15926    +323     
- Misses      2977    3041     +64     
- Partials     290     298      +8     
Files Coverage Δ
log/provider.go 100.0% <100.0%> (ø)
log/record.go 100.0% <100.0%> (ø)
log/kind_string.go 40.0% <40.0%> (ø)
log/severity_string.go 50.0% <50.0%> (ø)
log/slice.go 80.0% <80.0%> (ø)
log/internal/writer_logger.go 82.6% <82.6%> (ø)
log/internal/logr.go 61.3% <61.3%> (ø)
log/value.go 88.9% <88.9%> (ø)
log/internal/slog.go 57.4% <57.4%> (ø)

codecov[bot] avatar Nov 21 '23 09:11 codecov[bot]

I added draft code. I left TODO comments, basically misses docs and unit tests for Record and NewLoggerConfig.

pellared avatar Nov 24 '23 13:11 pellared

SIG meeting: @dashpole made a proposal for making record attributes as fields. We can explore if the user can use sync.Pool for decreasing the number of allocations.

EDIT: I think that

  1. We can have Attributes []attribute.KeyValue as field in Record.
  2. The log bridge implementation (e.g. slog handler) would use sync.Pool for the created slices to decrease the heap allocations for the passed attributes.
  3. The API implementation can use an [5]attribute.KeyValue + []attribute.KeyValue (nil when len(record.Attributes) <= 5) when copying the passed attributes. This would be needed when log records are processed asynchronously e.g. batched before exporting, for synchronous processing it wouldn't be even needed.

EDIT 2: PR with the proposal: https://github.com/pellared/opentelemetry-go/pull/4

pellared avatar Dec 14 '23 18:12 pellared

Hi, dear guys, I'm extremely interested in this feature lately. And I'm familiar with go programming but not very familiar with opentelemetry. Is there any thing I can do to help? BTW, is there any plan I can know to proceed further? Thanks for your attention. 👍

cloorc avatar Dec 20 '23 02:12 cloorc

Hi, dear guys, I'm extremely interested in this feature lately. And I'm familiar with go programming but not very familiar with opentelemetry. Is there any thing I can do to help? BTW, is there any plan I can know to proceed further? Thanks for your attention. 👍

You can review this PR and https://github.com/pellared/opentelemetry-go/pull/4.

pellared avatar Dec 20 '23 07:12 pellared

Logs API is implemented. Closing. The prototype may still be helpful in future (e.g. when implementing bridges).

pellared avatar Feb 23 '24 07:02 pellared