-
Notifications
You must be signed in to change notification settings - Fork 812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow multiple async callbacks, allow callbacks to be removed #4143
Conversation
api/all/src/main/java/io/opentelemetry/api/metrics/ObservableDoubleCounter.java
Outdated
Show resolved
Hide resolved
} | ||
this.callbacks.remove(callback); | ||
if (callbacks.size() == 0) { | ||
metricStorageRegistry.unregister(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like removing a callback should cause total unregistration. From my reading of the code, it looks like this should but wouldn't work
addCallback
removeCallback
addCallback
Second addCallback
would be unregistered
If the storage gets recreated at a higher level then maybe it's ok but I'd expect symmetry either at this level or a higher level, currently add and remove aren't symmetric and makes me wonder if there is possible thread safety issues at that higher level if this is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I wonder if unregistration causes the metric to stop being reported (not sure so just confirming) - if that is true, then we should avoid it for another reason, we'd want async and sync behavior to be consistent. There is currently no way for a sync metric to stop being reported, so we would want to solve both at the same time rather than just one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the storage gets recreated at a higher level then maybe it's ok
That's what happening - the second time you call addCallback
a new AynchronousMetricStorage
is registered with the MetricStorageRegistry
because none currently exists at that point. Its likely an unnecessary optimization, since it only frees up resources of asynchronous metrics that have no more callbacks registered at all, which is probably unusual.
Also I wonder if unregistration causes the metric to stop being reported (not sure so just confirming)
Asynchronous metrics are different from synchronous metrics in that we delegate the management of the metrics stream to the callback. So today (i.e. without this code) if a callback stops reporting a metric stream, it will stop being exported to cumulative exporters. In contrast, synchronous instruments continue to report values to exporters even when there is no data reported to the metric stream during the collection interval. This PR doesn't address that behavior.
Here's a little demo of what I mean:
InMemoryMetricReader cumulativeReader = InMemoryMetricReader.create();
Meter meter = SdkMeterProvider.builder()
.registerMetricReader(cumulativeReader)
.setMinimumCollectionInterval(Duration.ofSeconds(0))
.build()
.get("test-meter");
AtomicLong counter = new AtomicLong(2);
meter.counterBuilder("foo").buildWithCallback(measurement -> {
long count = counter.decrementAndGet();
if (count >= 0) {
measurement.record(10);
}
});
for (int i = 0; i < 5; i++) {
System.out.println(cumulativeReader.collectAllMetrics());
}
That code yields:
[...data...]
[...data...]
[]
[]
[]
…y-java into multiple-callbacks
Codecov Report
@@ Coverage Diff @@
## main #4143 +/- ##
============================================
- Coverage 90.24% 90.23% -0.02%
- Complexity 4744 4745 +1
============================================
Files 547 554 +7
Lines 14570 14591 +21
Branches 1400 1402 +2
============================================
+ Hits 13149 13166 +17
- Misses 958 966 +8
+ Partials 463 459 -4
Continue to review full report at Codecov.
|
...ics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorage.java
Outdated
Show resolved
Hide resolved
Now that #2317 has been merged in the spec, we should be unblocked to merge this. Before we do, a question: Is it clear / intuitive that when you call |
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkObservableInstrument.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MeterSharedState.java
Outdated
Show resolved
Hide resolved
...src/test/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorageTest.java
Outdated
Show resolved
Hide resolved
@jkwatson are you comfortable with merging this? |
…y-java into multiple-callbacks
|
||
@Override | ||
public void close() { | ||
if (removed.compareAndSet(false, true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, I think
if (!compareAndSet) {
LOG
return
}
is a bit of a better pattern so that logic can be increased at the unindented level, the false case is constant 2 LoC no matter what happens.
…y-java into multiple-callbacks
There has been discussion in the java SIG and at the spec level about allowing asynchronous instruments to have multiple callbacks. This PR demonstrates how that might function. One of the issues that comes up with supporting multiple callbacks is the need to remove or unregister them at a later time. This PR adds a
remove()
method to theObservable*
instrument interfaces.Related java issues: #4013, #3549
Related spec issues: #2249, #2232