Any support for metrics deduplication

[vasigorc]

Hello 👋 I've yet another question about Opentelemetry Java SDK, specifically about it's exporter part.

We have a bunch of gauge metrics with the same name - attribute. They are basically snapshots of some data which may or may not differ. In the latter case the results should be updated. When running this test scenario we started getting errors similar to this:

Mar. 02, 2022 9:40:28 P.M. io.opentelemetry.sdk.metrics.internal.state.MeterSharedState register
WARNING: Found duplicate metric definition: TestMetric
	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
Original instrument registered with same name but is incompatible.
	at unknown source
		To enable better debugging, run your JVM with -Dotel.experimental.sdk.metrics.debug=true

io.opentelemetry.sdk.metrics.internal.state.DuplicateMetricStorageException: Async metric with same name has already been created. Found previous metric: MetricDescriptor{name=TestMetric, description=, unit=1.0, sourceView=...etc}

The question is this: does SDK contain any tools for deduplication or do we need to perform this before offering metrics to the exporter?

JIC this is our setup for exporter-otlp (source code is in Scala):

    private val serviceName = config.getString("observability.service-name")

    private val metricReaderFactory =
      PeriodicMetricReader
        .builder(OtlpGrpcMetricExporter.getDefault)
        // 10 seconds
        .setInterval(Duration.of(config.getInt("observability.read-metrics.interval-seconds"), ChronoUnit.SECONDS))
        .newMetricReaderFactory()

    private val metricResource = Resource.create(Attributes.of(SERVICE_NAME, serviceName))

    private val meterProvider = SdkMeterProvider
      .builder()
      .registerMetricReader(metricReaderFactory)
      .setResource(metricResource)
      .build()

    private val meter = meterProvider
      .meterBuilder(serviceName)
      .setInstrumentationVersion(config.getString("observability.instrumentation-version"))
      .build()

   // example how metrics are consumed
    override def recordOurMetric(ourMetric: OurMetric): Unit = ourMetric match {
      case LongGaugeMetric(metricName, labels, value) =>
        meter
          .gaugeBuilder(metricName.value)
          .setUnit("1.0")
          .ofLongs()
          .buildWithCallback(measurement => {
            measurement.record(value, toOtelAttributes(labels))
          })

[anuraaga]

Hi @vasigorc - the spec recently got updated to allow requesting metrics multiple time, I believe #4222 implements the change.

That being said for performance reasons and just general idiomatic usage of our API, we'd expect you to keep a reference to any created instruments. So in the subclass with that override def, I guess it should have private val metric = meter.gaugeBuilder()....

Some general points

MeterProvider is meant to be shared within an app - so we wouldn't expect it to be private (it's common to register it into a dependency injection provider)

Meter is meant to be shared within some sort of scope, such as a library. For end users, this unit is generally the app itself though and it's common for this to be treated the same as MeterProvider.

The instruments created from a Meter are often private, final fields because they are measurements for a given component / class. So in your example, it looks somewhat unidiomatic to see Meter being private as we'd expect it to be visible to subclasses at least for calling in recordOurMetric.

[vasigorc]

Thank you for following-up @anuraaga !

We've updated the library and we get warnings instead now:

WARNING: Instrument TestMetric has recorded multiple values for the same attributes.
Mar. 09, 2022 11:51:29 A.M. io.opentelemetry.sdk.internal.ThrottlingLogger log
WARNING: Too many log messages detected. Will only log once per minute from now on.
Mar. 09, 2022 11:51:29 A.M. io.opentelemetry.sdk.internal.ThrottlingLogger doLog
WARNING: Instrument TestMetric has recorded multiple values for the same attributes.

With that I don't think we need any deduping, but that also signals that we may be losing the most recent values for same values. Is it possible to drop head instead - would we need to play with Aggregate to do this? E.g.:

    private val allGaugesSelector = InstrumentSelector.builder().setType(InstrumentType.OBSERVABLE_GAUGE).build()
    private val keepLastGaugeView = View
      .builder()
      .setAggregation(Aggregation.lastValue())
      .setName("Last gauge value")
      .setDescription("Gauges with the same attributes are state snapshots where new overrides an older one")
      .build()
    
    // private to not leak Otel SDK elsewhere in the code
    private val meterProvider = SdkMeterProvider
      .builder()
      .registerMetricReader(metricReaderFactory)
      .registerView(allGaugesSelector, keepLastGaugeView)
      .setResource(metricResource)
      .build()

[anuraaga]

A gauge is effectively always going to return the last value recorded I think. That being said the order that callbacks are called is undefined so it's sort of hard to understand which of your gauges is actually supposed to "win" when being collected. I had imagined that you have multiple gauges with the same identity being created and destroyed within the app, which is the main use case that the recent identity changes meant to address - if multiple identical gauges are valid at a given time how do you know which is the correct to record?

[vasigorc]

In case of identical gauges (if we are on the same page then it should be: having same name and attributes ?) then we would pick the one which arrived latest (but before being exported).

[anuraaga]

What do you mean by "arrive" in this case? The gauges are queried by the SDK in an arbitrary order so there isn't any way to guarantee any one is queried later than a former one. If an old gauge should be invalidated it can be close()d though.

[vasigorc]

I was referring to some sort of storage before exporting. With interval based exporting I believe this is the internal method that is being called.

I couldn't find the quering part in the library, but understanding how it works would be useful. Our use case is this: we export measurements at intervals of 10 seconds. Within the same period we could record same measurements (depending on external calls for example) 2+ times: for example for a delay on a route. Naturally we would want to keep and export the last measurement. It is possible to obtain this using other libraries (RxJava2, Reactor, Akka Streams etc.).

[jkwatson]

Asynchronous instrument callbacks are invoked here: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorage.java#L116-L119

[vasigorc]

Thank you, John. So we are synchronously appending callbacks, but not measurements. Meaning that checking for duplicate attributes won't work. That would probably mean that what we are looking for isn't supported. It doesn't mean that it is bad as our use case is probably not typical.