Conversation
github-actions
Bot
added
documentation
|
Sorry the way I wrote my comment on the last PR was confusing. What I mean to say lets fix up #4851 and merge that. Then we fix this PR after. |
…ivate code . PrometheusMetricReader's constructor became package-private in favor of a builder, so using FilterablePrometheusMetricReader.
mlbiscoc
left a comment
mlbiscoc
left a comment
There was a problem hiding this comment.
Can we have a test proving that the duplicate jvm metrics are gone?
| // The runtime-telemetry-java8/java17 split was unified into this single module (see | ||
| // https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/16087). Used this | ||
| // way (as a library, not via the OTel javaagent), its extra metric sets are only reachable | ||
| // through this internal-but-public "Experimental" bridge, not system properties -- those are | ||
| // only read by the javaagent's own auto-configuration. The two calls below reproduce the old | ||
| // library's .enableAllFeatures() breadth (buffers, file descriptors, GC cause, network I/O, | ||
| // etc.) and activate JFR, which -- unlike the old java8/java17 split this module replaced -- | ||
| // suppresses each metric's JMX series whenever its JFR series can serve as a full replacement, | ||
| // instead of emitting both. |
There was a problem hiding this comment.
I don't think we need all this here. Keep the TODO as well.
| @@ -0,0 +1,7 @@ | |||
| title: Fix Solr emitting duplicate/conflicting JVM metrics from both runtime-telemetry-java8 and runtime-telemetry-java17 OTel scopes on Java 17, by migrating to the unified opentelemetry-runtime-telemetry module (upgraded to 2.31.1-alpha) that replaced the java8/java17 split. | |||
There was a problem hiding this comment.
You are also upgrading OTEL versions here. Worth flagging.
Done! |
|
@mlbiscoc I think I've addressed your comments, would love a +1. |
|
I've got an itchy merge finger.... |
| // Guards against the java8/java17 RuntimeMetrics split emitting the same series twice (each | ||
| // scope reporting e.g. jvm.memory.committed with identical labels), which is what motivated | ||
| // migrating to the unified opentelemetry-runtime-telemetry module. |
There was a problem hiding this comment.
This comment looks unnecessary. We are just testing no duplicates. No need to reference past dependency.
| for (MetricSnapshot snapshot : snapshots) { | ||
| Set<Labels> seen = new HashSet<>(); | ||
| for (DataPointSnapshot dataPoint : snapshot.getDataPoints()) { | ||
| assertTrue( | ||
| "Duplicate series for metric " | ||
| + snapshot.getMetadata().getPrometheusName() | ||
| + " with labels " | ||
| + dataPoint.getLabels(), | ||
| seen.add(dataPoint.getLabels())); | ||
| } |
There was a problem hiding this comment.
I don't think this test is actually testing to catch the bug. I recommend doing TDD for this then make the change to confirm we actually fix it. This is a sample what the duplicate looks like:
jvm_memory_used_bytes{...,otel_scope_name="io.opentelemetry.runtime-telemetry-java8"}
jvm_memory_used_bytes{...,otel_scope_name="io.opentelemetry.runtime-telemetry-java17"}
The reason is that otel_scope_name is part of the label set that makes it a "duplicate".
2 participants
https://issues.apache.org/jira/browse/SOLR-18407
Description
Doing what @mlbiscoc suggested!
Solution
Upgrade is dragging along some other upgrades and code updates, but does fix the bug!
Tests
existing and ai