open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.35k stars 1.09k forks source link

Revert Cleanup interaction of exemplar and aggregation #5913

Closed XSAM closed 1 month ago

XSAM commented 1 month ago

Topic: #5249

This reverts commit 8041156518ee2f31ec9b36852fdc29f093d0f468 (PR: #5899) due to the performance degradation found by Benchmarks CI https://github.com/open-telemetry/opentelemetry-go/actions/runs/11447364022/job/31848519243

Here is the benchmark test on my machine:

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/sdk/metric
                                       │   old.txt   │                new.txt                 │
                                       │   sec/op    │    sec/op     vs base                  │
Instrument/instrumentImpl/aggregate-10   3.378µ ± 3%   49.366µ ± 1%  +1361.40% (p=0.000 n=10)
Instrument/observable/observe-10         2.288µ ± 2%   37.791µ ± 1%  +1551.73% (p=0.000 n=10)
geomean                                  2.780µ         43.19µ       +1453.65%

                                       │   old.txt    │                 new.txt                 │
                                       │     B/op     │     B/op       vs base                  │
Instrument/instrumentImpl/aggregate-10   1.245Ki ± 1%   22.363Ki ± 0%  +1696.08% (p=0.000 n=10)
Instrument/observable/observe-10           823.0 ± 1%    17432.5 ± 0%  +2018.17% (p=0.000 n=10)
geomean                                  1.000Ki         19.51Ki       +1850.48%

                                       │  old.txt   │                new.txt                │
                                       │ allocs/op  │  allocs/op   vs base                  │
Instrument/instrumentImpl/aggregate-10   1.000 ± 0%   21.000 ± 0%  +2000.00% (p=0.000 n=10)
Instrument/observable/observe-10         1.000 ± 0%   16.000 ± 0%  +1500.00% (p=0.000 n=10)
codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.6%. Comparing base (7a153a0) to head (b1d8e66). Report is 1 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-go/pull/5913/graphs/tree.svg?width=650&height=150&src=pr&token=8efTmh4kvf&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-go/pull/5913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #5913 +/- ## ===================================== Coverage 84.6% 84.6% ===================================== Files 271 272 +1 Lines 22825 22834 +9 ===================================== + Hits 19310 19320 +10 + Misses 3171 3170 -1 Partials 344 344 ``` [see 6 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-go/pull/5913/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
dashpole commented 1 month ago

It is probably from replacing nil with AlwaysOffFilter, if I had to guess.