Repository navigation
[API/SDK] Add Enabled() to synchronous instruments - #4721
adityabavadekar wants to merge 1 commit into
Conversation
- 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).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4721 +/- ##
==========================================
+ Coverage 86.63% 86.73% +0.10%
==========================================
Files 527 527
Lines 20830 20858 +28
==========================================
+ Hits 18045 18089 +44
+ Misses 2785 2769 -16
🚀 New features to boost your workflow:
|
mateenali66
left a comment
There was a problem hiding this comment.
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.
|
|
||
| // Variable to support implementation of Enabled method introduced in ABI V2. | ||
| // A pointer, so an SDK can flip many instruments with a single write. | ||
| const bool *enabled_ = EnabledFlag(); |
There was a problem hiding this comment.
Proposal:
bool enabled_in_api_ = false;
const bool *enabled_ = &enabled_in_api_;This:
- disable the flag by default, when no SDK is present
- avoids to call
this->SetEnabledFlag(this->DisabledFlag());from noop.h, since the flag is already disabled. - avoid header only singletons like the static
EnabledFlag()andDisabledFlag(), which are a problem in general.
For instruments implemented in an SDK, SetEnabledFlag() will work as before, pointing to the shared meter flag.
|
@adityabavadekar - Could you also include the enabled case in the benchmark, comparing |
| * | ||
| * @since ABI_VERSION 2 | ||
| */ | ||
| void SetEnabledFlag(const bool *enabled) noexcept { enabled_ = enabled; } |
There was a problem hiding this comment.
non-blocking - Could we mention that concurrent updates to this flag must use atomic writes? This would help other SDK implementations use it safely.
lalitb
left a comment
There was a problem hiding this comment.
LGTM overall. Could you add the enabled-case benchmark before merging, so we can see the overhead when metrics are being recorded normally?
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 toTracer::Enabled()(#3357). SpecFixes #4719
Changes
Enabled()toSynchronousInstrument(ABI v2 only).false.OPENTELEMETRY_ATOMIC_READ_RELAXED_8, sinceOPENTELEMETRY_ATOMIC_READ_8locks the cache line on MSVC.Benchmarks
Prototypes of each design with meter disabled, all threads sharing one counter. Each call checks
Enabled()before building attributes and callingAdd(), 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.
Enabled()(build attributes, thenAdd)Enabled()For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes