Skip to content

[SDK] Reduce lock contention in SyncMetricStorage for concurrent metric recording#3959

Merged
marcalff merged 13 commits intoopen-telemetry:mainfrom
perhapsmaple:optimize-concurrent-metric-recording
Apr 10, 2026
Merged

[SDK] Reduce lock contention in SyncMetricStorage for concurrent metric recording#3959
marcalff merged 13 commits intoopen-telemetry:mainfrom
perhapsmaple:optimize-concurrent-metric-recording

Conversation

@perhapsmaple
Copy link
Copy Markdown
Contributor

Fixes #3927

Changes

First set of changes to reduce lock contention: Move MetricAttributes construction outside the lock

Benchmark before:

Date: 2026-03-31
Build: Release, CXX14, AppleClang 17.0.0, arm64 (M-series, 8 cores)
Repetitions: 5
Threads: 4 (default)

Benchmark                                          Time (ns)     CPU (ns)    Iterations
---------------------------------------------------------------------------------------
BM_MeasurementsTest (1 thread)
  mean                                              2,237,371       12,722        10,000
  median                                            2,234,953       12,774        10,000
  stddev                                                6,464          215
  cv                                                    0.29%         1.69%

BM_MeasurementsThreadsShareCounterTest (4 threads, shared counter)
  mean                                              4,353,910       58,109         1,000
  median                                            4,471,432       59,356         1,000
  stddev                                              250,165        2,399
  cv                                                    5.75%         4.13%

BM_MeasurementsPerThreadCounterTest (4 threads, per-thread counters)
  mean                                              1,120,472       47,976        10,000
  median                                            1,122,614       49,069        10,000
  stddev                                               99,805        1,933
  cv                                                    8.91%         4.03%

Benchmark after:

Date: 2026-03-31
Build: Release, CXX14, AppleClang 17.0.0, arm64 (M-series, 8 cores)
Repetitions: 5
Threads: 4 (default)

Benchmark                                          Time (ns)     CPU (ns)    Iterations
---------------------------------------------------------------------------------------
BM_MeasurementsTest (1 thread)
  mean                                              2,230,225       11,958        10,000
  median                                            2,229,963       11,957        10,000
  stddev                                                  826         24.4
  cv                                                    0.04%         0.20%

BM_MeasurementsThreadsShareCounterTest (4 threads, shared counter)
  mean                                              2,904,767       51,929         1,000
  median                                            2,904,170       51,912         1,000
  stddev                                               17,484          800
  cv                                                    0.60%         1.54%

BM_MeasurementsPerThreadCounterTest (4 threads, per-thread counters)
  mean                                              1,031,435       45,674        10,000
  median                                            1,038,361       46,468        10,000
  stddev                                               64,472        1,719
  cv                                                    6.25%         3.76%

Shared-counter wall time dropped from 4,354us to 2,905us (33% reduction).

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
@perhapsmaple perhapsmaple requested a review from a team as a code owner March 31, 2026 16:31
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.18%. Comparing base (20430d0) to head (d86a58e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3959      +/-   ##
==========================================
+ Coverage   90.18%   90.18%   +0.01%     
==========================================
  Files         230      230              
  Lines        7298     7299       +1     
==========================================
+ Hits         6581     6582       +1     
  Misses        717      717              
Files with missing lines Coverage Δ
...entelemetry/sdk/metrics/state/attributes_hashmap.h 98.34% <ø> (-0.02%) ⬇️
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 89.19% <100.00%> (+0.62%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h
@perhapsmaple
Copy link
Copy Markdown
Contributor Author

Can the cppcheck errors be suppressed? It reports an “access after move” issue here, but this looks like a false positive.

@marcalff
Copy link
Copy Markdown
Member

marcalff commented Apr 2, 2026

Can the cppcheck errors be suppressed? It reports an “access after move” issue here, but this looks like a false positive.

The error reported by cpp-check on this line:

attributes_hashmap_->GetOrSetDefault(std::move(attr), create_default_aggregation_)

is indeed hard to understand, I don't get what the exact issue is either.

Note that there are 3 different GetOrSetDefault() methods, and as @ThomsonTan pointed out, one of them is dead code: please remove it, it will be one less item that can possibly confuse cpp-check.

@marcalff
Copy link
Copy Markdown
Member

marcalff commented Apr 2, 2026

@perhapsmaple

This should work to suppress warnings:

// cppcheck-suppress accessMoved

perhapsmaple and others added 7 commits April 2, 2026 23:13
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Comment thread sdk/test/metrics/async_metric_storage_test.cc
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See possible fix for IWYU.

Comment thread sdk/test/metrics/async_metric_storage_test.cc
perhapsmaple and others added 2 commits April 10, 2026 07:10
Co-authored-by: Marc Alff <marc.alff@free.fr>
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
@marcalff marcalff changed the title Reduce lock contention in SyncMetricStorage for concurrent metric recording [SDK] Reduce lock contention in SyncMetricStorage for concurrent metric recording Apr 10, 2026
Copy link
Copy Markdown
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@marcalff marcalff merged commit 37a57da into open-telemetry:main Apr 10, 2026
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce lock contention in SyncMetricStorage for concurrent metric recording

4 participants