Sample Groovy yields "Found duplicate metric definition"

[rkogelhe]

Description
jmxmetrics using the sample groovy works correctly on the first collection.
On the second collection, we get:
`WARNING: Found duplicate metric definition: my.admin.response.count
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=my.admin.response.count, description=Count of the number of responses from the admin console, unit=By, sourceView=Optional[View{name=null, description=null, aggregation=DefaultAggregation, attributesProcessor=io.opentelemetry.sdk.metrics.internal.view.AttributesProcessor$1@249c65fd, sourceInfo=INSTANCE}], sourceInstrument=InstrumentDescriptor{name=my.admin.response.count, description=Count of the number of responses from the admin console, unit=By, type=OBSERVABLE_GAUGE, valueType=LONG, sourceInfo=INSTANCE}}, cannot register metric: MetricDescriptor{name=my.admin.response.count, description=Count of the number of responses from the admin console, unit=By, sourceView=Optional[View{name=null, description=null, aggregation=DefaultAggregation, attributesProcessor=io.opentelemetry.sdk.metrics.internal.view.AttributesProcessor$1@249c65fd, sourceInfo=INSTANCE}], sourceInstrument=InstrumentDescriptor{name=my.admin.response.count, description=Count of the number of responses from the admin console, unit=By, type=OBSERVABLE_GAUGE, valueType=LONG, sourceInfo=INSTANCE}}
at io.opentelemetry.sdk.metrics.internal.state.MetricStorageRegistry.register(MetricStorageRegistry.java:75)
at io.opentelemetry.sdk.metrics.internal.state.MeterSharedState.register(MeterSharedState.java:139)
...`

Steps to reproduce
Clone opentelemetry-java-contrib and only build jmx-metrics (full build isn't working)
Run with
jmx_jar="./opentelemetry-java-contrib/jmx-metrics/build/libs/opentelemetry-jmx-metrics-1.11.0-SNAPSHOT.jar" java -Dotel.jmx.groovy.script=./my.groovy -jar $jmx_jar -config ./session.properties
where my.groovy is
def adminResponseTime = otel.mbean("my:name=adminResponseTime") otel.instrument( adminResponseTime, "my.admin.response.count", "Count of the number of responses from the admin console", "By", "Count", otel.&longValueCallback )
and session.properties is
otel.jmx.service.url = service:jmx:rmi:///jndi/rmi://localhost:1099/jmxrmi otel.jmx.interval.milliseconds = 5000 otel.jmx.username = jmxuser otel.jmx.password = password

Expectation
I would expect an example groovy that works however many collections occur.

What applicable config did you use?
(see reproduction above)

Relevant Environment Information
Version: 1.11.0
OS: MacOS 10.15.7
Compiler: AdoptOpenJDK (build 11.0.10+9)

Additional context
Verified that my JMX mbean was working via jmx term and JConsole.

[rmfitzpatrick]

Apologies for missing this until now. @Mrod1598 do you think you'd be able to rule out 7184f00 for this?

[rmfitzpatrick]

This appears to be introduced by open-telemetry/opentelemetry-java#4020 where there used to be a silent getOrCreate-like behavior. The underlying callback is updated w/ the most recent value via the proxied observer so no data loss should occur but the warning is distracting. The api is relatively in flux (open-telemetry/opentelemetry-java#4143) but we'll likely need to introduce another registry system for the OtelHelper's instruments to avoid this.

[anuraaga]

We're hoping to not need any registries of async callbacks anymore. @jack-berg would be good to check on this and make sure it works smoothly with all the identity changes.

[rmfitzpatrick]

@anuraaga given https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-type-conflict-detection I think this is a symptom of the jmx-metric gatherer's groovy script running pattern that builds the declared instruments with every interval being at odds with the metric api for both sync and async. This has been the case for a while so having a GroovyMetricEnvironment instrument registry is needed for this library afaict: #253

[anuraaga]

@rmfitzpatrick They have the same identity though right? Also we can close instruments between runs now perhaps so I think there is a way to work without the cache. If there isn't, I want us to get there instead of bothering users with unrelated error messages.

[rmfitzpatrick]

They're the same instruments in all cases and would be cached by their descriptor. The spec suggests to me that our current approach isn't supported without caching on the gatherer's end since there are conflicting registrations and a warning should be emitted. As a user I'd prefer this not be the case but can see it either way.

[jack-berg]

If I understand correctly this should be resolved with #4222.

At this point:

• Async instruments can have multiple callbacks associated with them.
• You can request the same instrument multiple times. Because async instruments now support multiple callbacks, this applies to both sync and async instruments.
• If all the parts of the metric identity match a previous instrument, measurements will be recorded to the same metric stream
• If multiple instruments share the same name, but some other part of their identity is not equal, you've created an identity conflict. We'll log a diagnostic error message, but export the the measurements in separate metric streams. Its up to the backend to decide how to handle this error scenario.

[trask]

@rkogelhe is this still an issue?

[github-actions]

This has been automatically marked as stale because it has been marked as needing author feedback and has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.