Skip to content

[API/SDK] Add Enabled() to synchronous instruments - #4721

Merged
marcalff merged 4 commits into
open-telemetry:mainfrom
adityabavadekar:sync_instrument_enabled
Nov 1, 2026
Merged

marcalff merged 4 commits into
open-telemetry:mainfrom
adityabavadekar:sync_instrument_enabled

Conversation

@adityabavadekar

Copy link
Copy Markdown
Contributor

Adds Enabled() to synchronous instruments, allowing callers to skip expensive work, such as building attributes, when nothing will be recorded. Follow-up to #4313, similar to Tracer::Enabled() (#3357). Spec

Fixes #4719

Changes

  • Adds a non-virtual Enabled() to SynchronousInstrument (ABI v2 only).
  • SDK instruments read their Meter's enabled flag, so a configurator update reaches all of them with one write. Instruments dropped by all matching views are always disabled.
  • No-op instruments return false.
  • Adds OPENTELEMETRY_ATOMIC_READ_RELAXED_8, since OPENTELEMETRY_ATOMIC_READ_8 locks the cache line on MSVC.
  • Adds tests covering all 8 SDK instruments, runtime configuration updates, Drop views, and no-op instruments.

Benchmarks

Prototypes of each design with meter disabled, all threads sharing one counter. Each call checks Enabled() before building attributes and calling Add(), while 8 background threads export spans over OTLP. GCC 16.2 -O3, i7-1360P, median of 3 runs.

Reading the meter's enabled flag through a pointer is around ~3× faster than a virtual Enabled() call.

A per-instrument flag is similarly fast to read, but configuration updates cost O(n) instead of O(1). With 65,536 instruments, updates took 665 µs versus 11.5 ns with the pointer.

Design 1 thread (ns/call) 16 threads (ns/call)
No Enabled() (build attributes, then Add) 258 499
Virtual Enabled() 2.62 5.42
Per-instrument flag 0.95 1.50
Pointer to Meter flag (this PR) 0.76 1.65

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

- Instruments report disabled when their meter is disabled or every matching view drops them
- The check is a single read, also on MSVC
- No-op instruments always report disabled (ABI v2 only).
@adityabavadekar
adityabavadekar requested a review from a team as a code owner October 21, 2026 07:21
@codecov

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.61%. Comparing base (050fb13) to head (79f382d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4721      +/-   ##
==========================================
+ Coverage   86.51%   86.61%   +0.11%     
==========================================
  Files         530      530              
  Lines       21136    21165      +29     
==========================================
+ Hits        18284    18331      +47     
+ Misses       2852     2834      -18     
Files with missing lines Coverage Δ
...i/include/opentelemetry/metrics/sync_instruments.h 100.00% <100.00%> (ø)
sdk/include/opentelemetry/sdk/metrics/meter.h 62.50% <ø> (ø)
...de/opentelemetry/sdk/metrics/meter_enabled_state.h 100.00% <100.00%> (ø)
...telemetry/sdk/metrics/state/multi_metric_storage.h 92.31% <100.00%> (+0.16%) ⬆️
sdk/src/metrics/meter.cc 89.07% <100.00%> (+5.07%) ⬆️
sdk/src/metrics/sync_instruments.cc 66.12% <100.00%> (+0.66%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mateenali66 mateenali66 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

checked the lifetime first. each sdk instrument holds the shared_ptr<MeterEnabledState> that owns its flag, so Enabled() stays valid after the provider is destroyed or the global provider is swapped. asan is clean on both, and on a dropped instrument. tsan is clean with 6 threads calling Enabled() and Add() while the configurator flips 4000 times. abi v1 layouts and vtables are unchanged, v2 only grows SynchronousInstrument by the pointer.

one optional thing. an instrument created from a meter whose provider is already gone has no storage and logs a warning on every Add(), but Enabled() returns true. storage == nullptr || storage->Empty() in GetInstrumentEnabledState would match what the Meter constructor already does without a context.

needs a rebase, only CHANGELOG conflicts. 1.30.0 shipped, so the entry moves under the new Unreleased.

@lalitb

lalitb commented Oct 27, 2026

Copy link
Copy Markdown
Member

@adityabavadekar - Could you also include the enabled case in the benchmark, comparing Add() directly with Enabled() followed by Add(), using the same attributes? The disabled results show the benefit clearly; this would show the overhead when metrics are being recorded normally.

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM overall. Could you add the enabled-case benchmark before merging, so we can see the overhead when metrics are being recorded normally?

- Give each instrument a disabled flag, keep copies pointing at the correct one
- Treat instruments without storage as disabled
- Benchmark Enabled() both when the meter is on and when it is off
@adityabavadekar

Copy link
Copy Markdown
Contributor Author

@lalitb Added both cases to sync_instruments_benchmark.cc, keeping the same attributes and thread range as the existing benchmarks.

Ran a release build and took the median of 5 runs.

Case 1 thread 2 threads 4 threads
Enabled, Add() 673 ns 1225 ns 3213 ns
Enabled, Enabled() + Add() 676 ns 1211 ns 3151 ns
Disabled, Add() 2.28 ns 2.33 ns 2.43 ns
Disabled, Enabled() + Add() 0.58 ns 0.60 ns 0.72 ns

Enabled: negligible overhead. Disabled: checking Enabled() first is ~4x faster, even with prebuilt attributes.

@adityabavadekar

Copy link
Copy Markdown
Contributor Author

@mateenali66 Fixed, instruments without storage are now treated as disabled, with a test. Also merged main and moved the CHANGELOG entry to under Unreleased.

Co-authored-by: Marc Alff <marc.alff@oracle.com>

@marcalff marcalff left a comment

Copy link
Copy Markdown
Member

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 90af189 into open-telemetry:main Nov 1, 2026
73 of 75 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.

[METRICS] Add Enabled() to synchronous instruments

4 participants