Add setMeterConfigurator support to MeterProvider
#7346
Conversation
|
I need to add a bunch more tests, I just wanted to check in to see if the approach is sound. The idea is to have a sort of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7346 +/- ##
============================================
- Coverage 90.06% 90.04% -0.02%
- Complexity 7104 7114 +10
============================================
Files 805 805
Lines 21484 21517 +33
Branches 2093 2098 +5
============================================
+ Hits 19349 19376 +27
- Misses 1472 1476 +4
- Partials 663 665 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...c/main/java/io/opentelemetry/sdk/metrics/internal/state/DefaultSynchronousMetricStorage.java
Outdated
Show resolved
Hide resolved
…l/state/DefaultSynchronousMetricStorage.java
|
Hi @jack-berg, any comments on this? I think it's ready for a first round of review |
|
Hey @fandreuz - sorry for the delay. I'm swamped with internal stuff right now. Trying to get in a spot to be able to spend more time doing code reviews for otel next week. |
...c/main/java/io/opentelemetry/sdk/metrics/internal/state/DefaultSynchronousMetricStorage.java
Outdated
Show resolved
Hide resolved
…l/state/DefaultSynchronousMetricStorage.java
| private final Map<RegisteredReader, MetricStorageRegistry> readerStorageRegistries; | ||
| private final boolean meterEnabled; | ||
|
|
||
| private boolean meterEnabled; |
There was a problem hiding this comment.
I suspect this is going to need some sort of memory barrier around it. Either make it an AtomicBoolean, or provide some kind of synchronization around the update to it.
There was a problem hiding this comment.
@jack-berg should this field be marked volatile ?
There was a problem hiding this comment.
In other similar situations we've punted on the volatile keyword with the argument that even the small performance impact of reading a volatile field vs a non-volatile field is too high a price to pay the dynamism we get from this feature, which will only be leveraged by a few users. I know that in theory not marking it volatile may mean that the changes are never seen by readers, but I'd like to see complaints of that in practice before paying the performance penalty.
- SdkTracer
- [SdkLogger](https://github.com/open-telemetry/opentelemetry-java/blob/567e737d7de943e22a8ed5fe59b0970cbc09f0ef/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLogger.java#L36-L38]
In this particular instance, adding volatile isn't terribly consequential because the field is only read on collection. The read work happens down in DefaultSynchronousMetricStorage, which has an enabled field which is also marked non-volatile and which is read each and every time a measurement is recorded. So we could update SdkMeter#meterEnabled to be volatile, but I don't think we should make DefaultSynchronousMetricStorage#enabled volatile. So the question is, should we be mark SdkMeter#meterEnabled because we can, even though its not really consequential? Or leave it be for consistency with SdkTracer, SdkLogger, DefaultSynchronousMetricStorage#enabled?
There was a problem hiding this comment.
fair enough. I'm ok with it, if you're ok with it.
| private final AggregationTemporality aggregationTemporality; | ||
| private final Aggregator<T, U> aggregator; | ||
| private volatile AggregatorHolder<T, U> aggregatorHolder = new AggregatorHolder<>(); | ||
| private final AtomicReference<AggregatorHolder<T, U>> aggregatorHolder; |
There was a problem hiding this comment.
does this change affect performance at all? I wouldn't mix changes like this into this PR unless there's a very specific reason for it. If we change internals like this, we should do it in an isolated PR and provide before/after benchmarks to show it is not harmful, or (hopefully) helpful.
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Sorry for the delay in review.
Been thinking about this for a while, and don't think its completely obvious how the SDK should behave in a couple of cases surrounding changes to a meter being enabled:
Consider instrument A has cumulative temporality, and is part of meter Foo. Foo is initially enabled, then disabled, then later enabled, all while receiving a steady stream of measurements. It shouldn't be part of exported metrics when its disabled. Its measurements shouldn't be considered when its disabled. But when it is re-enabled, should it include measurements prior to disabling? What if its disabled and re-enabled quickly, before its omitted from any export? What should the start timestamp of the metric be in each export?
If this same thing occurs when temporality is delta its simpler to reason about. Since delta state is reset after each collection, there is no need to answer the question about measurements prior to disabling.
Now consider the same instrument, but the meter is initially disabled then later enabled. Same questions as above about what the start timestamp of each metric export should be.
Given that there is no specification language on this, I propose we do the simplest reasonable thing for the time being. The goal being to avoid cluttering / making the metrics SDK implementation even more complex, while getting something out there to solicit feedback. If the simplest thing turns out to be insufficient, we can iterate and improve.
Here's a commit where I tinker with your branch a bit with dirt simple mechanics: cd130c8
Idea:
- Whether or not a meter is enabled, always instantiate the instrument and storage normally
- When a meter is disabled, don't record measurements to synchronous instruments
- When a meter is disabled, don't call asynchronous instrument callbacks
- Whether or not a meter is enabled, always call collect on all the storage for the meter. Update storage implementations to be aware of whether their meter is enabled / disabled, and if disabled, return an empty metric after performing all state management.
This has the following properties:
- Minimally invasive
- Lets metric storage manage state as is
- Avoids perf his of recording measurements when a meter is disabled
- When temporality is delta and a meter is updated to be disabled, state is naturally reset after the next collection, and subsequent collects are cheap
- When temporality is cumulative and a meter is updated to be disabled, state is retained and may be incremented if the meter is later reenabled. Exported cumulative metrics always carry a start time corresponding to the start time of the metrics SDK.
|
Hi @jack-berg, thanks for getting back on this. I agree that my solution is a bit invasive and some choices I took are arbitrary. Yours seems to be a good compromise. Let me know what I should do with this PR. I see many tests commented, are they all failing/not compiling? |
I only commented those out to quickly see if the idea was feasible. I'm happy to uncomment / fix those tests and push them to this PR branch, or let you merge in my commit and fix up the tests. No preference here! |
|
I ran the tests and some of them are failing, I'll try to fix them |
What's the status on this? |
|
Hi @jkwatson, I'm a bit swamped. Somebody else can take it, otherwise I'll get back to this when I have some more time. |
|
I'll pick this up and push commits to this PR if you don't mind @fandreuz |
…#7346) Co-authored-by: Jack Berg <[email protected]> Co-authored-by: Jack Berg <[email protected]>
3 participants
Fixes #7051