Log warning when duplicate async instruments are registered

[jack-berg]

Resolves #4005.

The example in the issue now yields the following results:

    AtomicLong countUp = new AtomicLong(0);
    AtomicLong countDown = new AtomicLong(100);
    meter.gaugeBuilder("foo").buildWithCallback(observe -> observe.record(countUp.incrementAndGet()));
    meter.gaugeBuilder("foo").buildWithCallback(observe -> observe.record(countDown.decrementAndGet()));

....
WARNING: Found duplicate metric definition: foo
	at unknown source
		To enable better debugging, run your JVM with -Dotel.experimental.sdk.metrics.debug=true
Causes
- InstrumentType [OBSERVABLE_GAUGE] is async and already registered

[codecov]

Codecov Report

Merging #4020 (ae0a5cc) into main (905c1ff) will increase coverage by 0.03%.
The diff coverage is 78.57%.

@@             Coverage Diff              @@
##               main    #4020      +/-   ##
============================================
+ Coverage     89.90%   89.93%   +0.03%     
- Complexity     4310     4316       +6     
============================================
  Files           513      513              
  Lines         13079    13091      +12     
  Branches       1266     1268       +2     
============================================
+ Hits          11759    11774      +15     
+ Misses          915      914       -1     
+ Partials        405      403       -2     

Impacted Files	Coverage Δ
.../metrics/internal/descriptor/MetricDescriptor.java	84.21% <40.00%> (-15.79%)	⬇️
...lemetry/sdk/metrics/internal/state/DebugUtils.java	100.00% <100.00%> (ø)
.../metrics/internal/state/MetricStorageRegistry.java	100.00% <100.00%> (ø)
...entelemetry/exporter/jaeger/PostSpansResponse.java	0.00% <0.00%> (-100.00%)	⬇️
...exporter/jaeger/MarshalerCollectorServiceGrpc.java	85.71% <0.00%> (-4.77%)	⬇️
...ntelemetry/sdk/extension/resources/OsResource.java	90.69% <0.00%> (+4.65%)	⬆️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java	100.00% <0.00%> (+5.88%)	⬆️
...metry/sdk/extension/resources/ProcessResource.java	87.50% <0.00%> (+6.25%)	⬆️
...elemetry/sdk/extension/resources/HostResource.java	92.30% <0.00%> (+15.38%)	⬆️
...dk/extension/resources/ProcessRuntimeResource.java	100.00% <0.00%> (+16.66%)	⬆️

---

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 905c1ff...ae0a5cc. Read the comment docs.

[ghost]

Looks good, although codecov complains about new cov < 80% ;)

[jsuereth]

There's one valid use case of multiple registrations that I would like an answer to (via batch API) before we lock this down. Specifically, registering callbacks that report different attributes.

    AtomicLong countUp = new AtomicLong(0);
    Attributes countUpAttr = ...;
    meter.gaugeBuilder("foo").buildWithCallback(observe -> observe.record(countUp.incrementAndGet(), countUpAtttr));
    
    AtomicLong countDown = new AtomicLong(100);
    Attributes countDownAttr = ...;
    meter.gaugeBuilder("foo").buildWithCallback(observe -> observe.record(countDown.decrementAndGet(), countDownAttr));
}

Nominally, users could just put each observation in a single callback and maybe that's ok. Just want to make sure we document HOW we want users to do that, especially if we lock down use cases.

[jack-berg]

Nominally, users could just put each observation in a single callback and maybe that's ok. Just want to make sure we document HOW we want users to do that, especially if we lock down use cases.

Small clarification - this PR doesn't change the behavior. If you register multiple callbacks today the first will be called and subsequent ones are silently ignored. This just tries to help users debug why their callback isn't being called.

I think we have to recommend that for now. An option for users is to use a helper function to transform several callbacks into a composite callback:

    meter.gaugeBuilder("foo").buildWithCallback(compositeCallback(
        observer -> observer.record(countUp.incrementAndGet(), upAttrs),
        observer -> observer.record(countDown.incrementAndGet(), downAttrs)
    ));

  ...

  static Consumer<ObservableDoubleMeasurement> compositeCallback(
      Consumer<ObservableDoubleMeasurement>... callbacks) {
    return observer -> {
      for (Consumer<ObservableDoubleMeasurement> callback : callbacks) {
        callback.accept(observer);
      }
    };
  }

[jack-berg]

There's an outstanding issue related to registering multiple async callbacks here.

I believe its possible to support this type of use case by. We'd simply have to make AsynchronousMetricStorage instances mutable such that when another callback is registered for the same instrument, that the storage is updated to have a callback which is a composite of the original and the new.

However, the issue references that the spec is the limiting factor right now, which is likely due to the following section:

Meter implementations MUST return an error when multiple Instruments are registered under the same Meter instance using the same name.

Thinking about it more and my other comment, a user could implement their own mutable callback to solve that problem. For example:

    MutableCallback mutableCallback = new MutableCallback();
    meter.gaugeBuilder("foo").buildWithCallback(mutableCallback);
    mutableCallback.add("up", observer -> observer.record(countUp.incrementAndGet(), upAttrs));
    mutableCallback.add("down", observer -> observer.record(countDown.decrementAndGet(), downAttrs));

  ...

  static class MutableCallback implements Consumer<ObservableDoubleMeasurement> {
    private final Map<String, Consumer<ObservableDoubleMeasurement>> callbacks = new HashMap<>();

    void add(String id, Consumer<ObservableDoubleMeasurement> callback) {
      callbacks.put(id, callback);
    }

    void remove(String id) {
      callbacks.remove(id);
    }

    @Override
    public void accept(ObservableDoubleMeasurement observable) {
      List<Consumer<ObservableDoubleMeasurement>> callbacksCopy = new ArrayList<>(callbacks.values());
      for (Consumer<ObservableDoubleMeasurement> callback : callbacksCopy) {
        callback.accept(observable);
      }
    }
  }

[mateuszrzeszutek]

There's one valid use case of multiple registrations that I would like an answer to (via batch API) before we lock this down. Specifically, registering callbacks that report different attributes.

Just for the record, I've encountered exactly this case in my Micrometer instrumentation -- ended up implementing a mutable composite callback, works fine; especially when you have to stop/remove certain measurements.

[anuraaga]

Let's get this logging in, and if we do in the future allow multiple callbacks even with the same API, it would be accepting more input than before by removing the log message and a semver-comaptible change