mlbiscoc commented on code in PR #4907:
URL: https://github.com/apache/solr/pull/4907#discussion_r3992279682
##########
changelog/unreleased/SOLR-18407.yml:
##########
@@ -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.
Review Comment:
You are also upgrading OTEL versions here. Worth flagging.
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -75,11 +77,19 @@ public ContextPropagators getPropagators() {
return OpenTelemetry.noop().getPropagators();
}
};
- this.runtimeMetrics =
- RuntimeMetrics.builder(otel)
- // TODO: We should have this configurable to enable/disable
specific JVM metrics
- .enableAllFeatures()
- .build();
+ // 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.
Review Comment:
I don't think we need all this here. Keep the TODO as well.
##########
solr/core/src/java/org/apache/solr/metrics/otel/FilterablePrometheusMetricReader.java:
##########
@@ -37,16 +43,47 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class FilterablePrometheusMetricReader extends PrometheusMetricReader {
Review Comment:
This is an interesting change with the APIs. If tests pass then I guess its
ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
