Add batch callback API

[jack-berg]

Implementation of spec issue #2363.

Allows a single callback to record measurements for multiple instruments.

• Can now create observers (ObservableLongMeasurement and ObservableLongMeasurement) for async instruments, i.e. ObservableDoubleMeasurement DoubleCounterBuilder#buildObserver().
• Can now register batch callbacks which record to multiple observers:

ObervableMeasurement myDoubleCounter = ...
ObervableMeasurement myLongCounter = ...
meter.batchCallback(() -> {
    myDoubleCounter.record(1.1);
    myLongCounter.record(1);
  }, myDoubleCounter, myLongCounter);

Resolves #3761.

Replaces earlier prototype #4024.

Updated 04/30/2022

[codecov]

Codecov Report

Merging #4376 (4c99c2e) into main (b98ce55) will decrease coverage by 0.09%.
The diff coverage is 82.26%.

@@             Coverage Diff              @@
##               main    #4376      +/-   ##
============================================
- Coverage     90.12%   90.02%   -0.10%     
- Complexity     5003     5029      +26     
============================================
  Files           569      578       +9     
  Lines         15435    15522      +87     
  Branches       1487     1493       +6     
============================================
+ Hits          13911    13974      +63     
- Misses         1061     1084      +23     
- Partials        463      464       +1     

Impacted Files	Coverage Δ
...va/io/opentelemetry/api/metrics/BatchCallback.java	0.00% <0.00%> (ø)
...pentelemetry/api/metrics/DoubleCounterBuilder.java	0.00% <0.00%> (ø)
.../opentelemetry/api/metrics/DoubleGaugeBuilder.java	0.00% <0.00%> (ø)
...emetry/api/metrics/DoubleUpDownCounterBuilder.java	0.00% <0.00%> (ø)
.../opentelemetry/api/metrics/LongCounterBuilder.java	0.00% <0.00%> (ø)
...io/opentelemetry/api/metrics/LongGaugeBuilder.java	0.00% <0.00%> (ø)
...elemetry/api/metrics/LongUpDownCounterBuilder.java	0.00% <0.00%> (ø)
.../main/java/io/opentelemetry/api/metrics/Meter.java	0.00% <0.00%> (ø)
...ava/io/opentelemetry/api/metrics/DefaultMeter.java	89.71% <21.42%> (-10.29%)	⬇️
...trics/internal/state/SdkObservableMeasurement.java	86.48% <86.48%> (ø)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b98ce55...4c99c2e. Read the comment docs.

[anuraaga]

We had earlier brainstormed about using the sync versions of the instruments themselves within callbacks since the usage seems very sync. Did you have any thoughts on that?

[anuraaga]

meter.batchCallbackBuilder()
  .add(myDoubleCounter)
  .add(myLongCounter)
  .build(() -> {
    myDoubleCounter.record(1.1);
    myLongCounter.record(1);
  });

Also for this I suggest pushing back on the add(myDoubleCounter) double-registration pattern. From what I can tell, this is mostly about a minor shoot-yourself-in-foot scenario, but the API ergonomics suffer too much to accomplish this IMO.

[jack-berg]

We had earlier brainstormed about using the sync versions of the instruments themselves within callbacks since the usage seems very sync. Did you have any thoughts on that?

We could potentially use the the API of the synchronous instruments (e.g. LongCounter instead of the ObservableLongMeasurement). However, I can think of a few reasons why that wouldn't be ideal:

• With async instruments (specifically async counter and async up down counter) we observe sums that have been aggregated from other systems. It feels a bit strange to observe a sum with LongCounter#add.
• I think we would still need a different API for instantiating async versions of the instruments. I.e. we'd need something like LongCounter buildAsync() in LongCounterBuilder. If the LongCounter from LongCounterBuilder#build() was used to record values in a batch callback, I don't think the SDK processing the measurements would be able to tell the difference between values recorded synchronously and asynchronously. Its essential to know whether the measurement was recorded a synchronous or async because they mean different things: sync measurements represent the increment, async measurements represent the sum.
• There is no synchronous version of gauge right now, so we'd have to either create one or have asymmetry in how batch callbacks work for gauges.

Also for this I suggest pushing back on the add(myDoubleCounter) double-registration pattern. From what I can tell, this is mostly about a minor shoot-yourself-in-foot scenario

Do you disagree with having any registration requirement of the instruments that will be involved in the batch callback? Or that there are different methods for registering long instruments and double instruments?

I'm currently convinced that requiring registration of involved instruments is a good thing (see this comment). Doing so:

• Allows us to skip the callback if none of the instruments are configured for export
• Allows us to support variable collection intervals for async instruments in the future
• Allows us to (more easily) enforce that the batch callbacks only records measurements for instruments in the same meter

[anuraaga]

Do you disagree with having any registration requirement of the instruments that will be involved in the batch callback?

I disagree with any registration requirement - it's just really tedious to pass in an instrument, to only then effectively pass it in again when using in the callback. I've seen similar in React Hooks, where it's unfortunately required for the semantics themselves, and most people find it pretty annoying. Luckily there are compiler plugins to automatically add the registration command in that world.

Allows us to skip the callback if none of the instruments are configured for export

Is this if a view configures a metric to be dropped? I don't know if this happens common enough in practice to require the stuttering in the API usage.

Allows us to support variable collection intervals for async instruments in the future

I haven't heard of this before, could you describe the use case and what it would look like? We would have the callbacks not exported as part of the SDK's collection interval?

Allows us to (more easily) enforce that the batch callbacks only records measurements for instruments in the same meter

This is the corner case I was aware of, it seems like a small point to require tedium in the API IMO.

[jack-berg]

I haven't heard of this before, could you describe the use case and what it would look like? We would have the callbacks not exported as part of the SDK's collection interval?

I opened a spec #2120 (also related to #2200 and #1432) a bit back related to this. My example was:

Suppose you were interested in getting JVM runtime metrics every 10 seconds, but a 60 second interval was sufficient for potentially higher cardinality metrics capturing http server usage.

This would be possible if:

• You configure the meter provider with two periodic metric readers, corresponding to the 10 second and 60 second collection intervals
• Allow views to be configured at the metric reader level, proposed here
• Configure the 10 second interval reader with views that drop all instruments, expect those starting with jvm.runtime.*
• Configure the 60 second interval reader to drop instruments starting with jvm.runtime.*

Without knowing which instruments a batch callback can record to, the SDK has to evaluate all the batch callbacks each collection.

FYI, the declarative association of instruments at registration time of the batch callback is currently proposed as a MUST in the spec. I'm in favor because it allows the SDK to be more correct, and the API it produces isn't that bad.

[jack-berg]

We talked about a few different options for the API in the 4/14 Java Office Hours.

Multiple add methods, and build

Explicit registration of instruments via two add methods, with build accepting the Runnable:

ObservableDoubleMeasurement foo = meter.gaugeBuilder("foo").buildObserver();
ObservableDoubleMeasurement bar = meter.gaugeBuilder("bar").buildObserver();
ObservableLongMeasurement baz = meter.gaugeBuilder("baz").ofLongs().buildObserver();

meter.batchCallbackBuilder()
    .add(foo, bar)
    .add(baz)
    .build(() -> {
      MyMetrics myMetrics = readMyMetrics();
      foo.record(myMetrics.foo);
      bar.record(myMetrics.bar);
      baz.record(myMetrics.baz);
    });

An alternative version of this has add accept a single argument instead of varargs. I.e. add(ObservableLongMeasurement) and add(ObservableDoubleMeasurement).

An alternative version of this defines a single add method accepting ObservableMeasurement, a new interface extended by ObservableDoubleMeasurement and ObservableLongMeasurement.

We also discussed a version of this where we introduce a method called something like a strictMode option. When strictMode = true, you can only record to instruments you've added. When strictMode=false you can record to any instrument. However, it would be potentially unwieldy to implement strictMode=false since that would require unlocking all instruments.

Single method

A single method accepts the runnable and the instruments

ObservableDoubleMeasurement foo = meter.gaugeBuilder("foo").buildObserver();
ObservableDoubleMeasurement bar = meter.gaugeBuilder("bar").buildObserver();
ObservableLongMeasurement baz = meter.gaugeBuilder("baz").ofLongs().buildObserver();

meter
    .batchCallback(() -> {
      MyMetrics myMetrics = readMyMetrics();
      foo.record(myMetrics.foo);
      bar.record(myMetrics.bar);
      baz.record(myMetrics.baz);
    }, foo, bar, baz);

This code demonstrates the method signature as batchCallback(Runnable, ObservableMeasurement...), where ObservableMeasurement is an interface extended by ObservableDoubleMeasurement and ObservableLongMeasurement.

An alternative version of this accepts a set of instruments (i.e. batchCallback(Runnable, Set<ObservableMeasurement>)) instead of varargs. This would make it harder to mistakenly omit instruments, but requires more syntax (i.e. new HashSet<>(Arrays.asList(foo, bar, baz)) until java 11 Set.of(..) makes your life easier).

Yet another alternative version of this accepts a set of instruments, and switches the argument order so the Runnable is second. This makes it even harder to ignore configuring instruments, but varargs is no longer an option.

I believe by the end we were agreeing on keeping the instrument registration explicit, justified by it being unlikely that end users will have to register many of these. And after all, the batch callback concept is a performance optimization for users who care about multiple invocations of their callback. It seems unlikely the extra instrument argument will be a big imposition on performance minded folks.

My preference at the moment is a single method: batchCallback(Runnable, Set<ObservableMeasurement>).

[anuraaga]

This would make it harder to mistakenly omit instruments,

How does the Set version make it harder to omit?

Edit: If we want a required argument with vararg it can look like Observable, Observable....

[trask]

+2 cents for

meter
    .batchCallback(() -> {
      MyMetrics myMetrics = readMyMetrics();
      foo.record(myMetrics.foo);
      bar.record(myMetrics.bar);
      baz.record(myMetrics.baz);
    }, foo, bar, baz);

with required arg before the varargs like @anuraaga mentioned

[jack-berg]

How does the Set version make it harder to omit?

With the varargs version you could call it with no observables it would compile. You might not look at the javadoc close enough to understand that you need to include observables.

meter
    .batchCallback(() -> {
      MyMetrics myMetrics = readMyMetrics();
      foo.record(myMetrics.foo);
      bar.record(myMetrics.bar);
      baz.record(myMetrics.baz);
    });

If forced to provide a set, you can't ignore including the observables without a compilation error. Of course you can still provide an empty set, but that feels more intentionally negligent.

I do like the varargs version too though, just slightly easier to stumble.

[jkwatson]

This would make it harder to mistakenly omit instruments,

How does the Set version make it harder to omit?

Edit: If we want a required argument with vararg it can look like Observable, Observable....

Yes, if we're going to varargs, this is the way to do it, since you have to include at least one Observable or nothing will do anything.

[jack-berg]

Edit: If we want a required argument with vararg it can look like Observable, Observable....

Got it. Didn't think about that strategy, but that's a nice solution.

[jack-berg]

Multi instrument callbacks have been merged into the spec, so I've marked this PR as "ready for review".

I'm not in a rush to get it in before the next release, but I'd be happy if it were included assuming folks are comfortable with the additions to the API.

[jkwatson]

As discussed in the SIG meeting last evening, we need to hold off on merging this until the spec has been officially released containing this functionality.

[jack-berg]

FYI, the spec has released this feature in v1.11.0.

If we're happy with the API, we can safely include it.

[ghost]

I believe we're all OK with the current API (single Observable required, multiple possible).

[jack-berg]

@jkwatson do you have any additional opinions about this addition?

@jsuereth I know you've expressed interest in the past about a batch API. Do you have any thoughts?

[jkwatson]

@jkwatson do you have any additional opinions about this addition?

I think it's fine, as long as instrumentation folks are happy with it, since they are the main driver of the feature.

[jsuereth]

@jack-berg This LGTM given the current spec. I do wish we had spent some additional time thinking through type-system level enforcement we could provide to prevent "bad" usage at compile time, but what you propose here is likely good enough.