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".
There was a problem hiding this comment.
I just ran the new test you had on my main and it still passes which didn't seem right. For TDD it should have failed proving this bug exists. I took a look and also otel_scope_version matters here.
The java17 scope adds otel_scope_version that java8 doesn't have, so even with otel_scope_name excluded, the two rows still look different.
To really fix this group by metric name, then check how many distinct otel_scope_name values appear for that name. If it's more than one, that metric has two sources.
Can you fix the test and make sure it fails on main then passes on this branch after the fix? After that, I am good with the rest of the PR.
|
@mlbiscoc can you try now? |
The test still doesn't test the problem. Please see #4907 (comment) |
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