Copilot commented on code in PR #4209:
URL: https://github.com/apache/solr/pull/4209#discussion_r2924267128
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,36 @@ public ContextPropagators getPropagators() {
// TODO: We should have this configurable to enable/disable
specific JVM metrics
.enableAllFeatures()
.build();
+ java.lang.management.OperatingSystemMXBean osMxBean =
+ ManagementFactory.getOperatingSystemMXBean();
+ if (osMxBean instanceof com.sun.management.OperatingSystemMXBean
extOsMxBean) {
+ systemMemoryTotalGauge =
+ solrMetricManager.observableLongGauge(
+ registryName,
+ "jvm.system.memory",
+ "Total physical memory of the host or container in bytes."
+ + " On Linux with cgroup limits, reflects the container
memory limit.",
+ measurement -> {
+ long total = extOsMxBean.getTotalMemorySize();
+ if (total > 0) measurement.record(total);
+ },
+ OtelUnit.BYTES);
+ systemMemoryFreeGauge =
+ solrMetricManager.observableLongGauge(
+ registryName,
+ "jvm.system.memory.free",
+ "Free (unused) physical memory of the host or container in
bytes.",
+ measurement -> {
+ long free = extOsMxBean.getFreeMemorySize();
+ if (free > 0) measurement.record(free);
+ },
+ OtelUnit.BYTES);
+ log.info("Physical memory metrics enabled");
+ } else {
+ log.info(
Review Comment:
The ref-guide says these metrics are “silently absent” when unsupported, but
this code logs an INFO message on every startup when
`com.sun.management.OperatingSystemMXBean` isn’t present. Consider downgrading
these messages to DEBUG (or removing them) or update the documentation to match
the actual behavior.
```suggestion
log.debug(
```
##########
solr/core/src/test/org/apache/solr/metrics/JvmMetricsTest.java:
##########
@@ -105,4 +107,51 @@ public void testSetupJvmMetrics() throws
InterruptedException {
"Should have JVM buffer metrics",
metricNames.stream().anyMatch(name -> name.startsWith("jvm_buffer")));
}
+
+ @Test
+ @SuppressForbidden(reason = "Testing
com.sun.management.OperatingSystemMXBean availability")
+ public void testSystemMemoryMetrics() {
+ PrometheusMetricReader reader =
+ solrTestRule
+ .getJetty()
+ .getCoreContainer()
+ .getMetricManager()
+ .getPrometheusMetricReader("solr.jvm");
+ MetricSnapshots snapshots = reader.collect();
+
+ Set<String> metricNames =
+ snapshots.stream()
+ .map(metric -> metric.getMetadata().getPrometheusName())
+ .collect(Collectors.toSet());
+
+ // Physical memory metrics are only present on HotSpot JDK
(com.sun.management available).
+ // If absent, the test is skipped (graceful degradation is tested
separately).
+ boolean isHotSpot =
+ java.lang.management.ManagementFactory.getOperatingSystemMXBean()
+ instanceof com.sun.management.OperatingSystemMXBean;
+ Assume.assumeTrue(
+ "Skipping: com.sun.management.OperatingSystemMXBean not available",
isHotSpot);
+
+ assertTrue(
+ "Should have jvm_system_memory_bytes metric",
+ metricNames.contains("jvm_system_memory_bytes"));
+ assertTrue(
+ "Should have jvm_system_memory_free_bytes metric",
+ metricNames.contains("jvm_system_memory_free_bytes"));
+ }
+
+ @Test
+ public void testJvmMetricsDisabledNoSystemMemory() throws Exception {
+ // Verify that when JVM metrics are disabled, initialization is a no-op
and close() is safe
+ SolrMetricManager metricManager =
solrTestRule.getJetty().getCoreContainer().getMetricManager();
+ System.setProperty("solr.metrics.jvm.enabled", "false");
+ try {
+ OtelRuntimeJvmMetrics disabledMetrics = new OtelRuntimeJvmMetrics();
+ OtelRuntimeJvmMetrics result = disabledMetrics.initialize(metricManager,
"solr.jvm");
+ assertFalse("Should not be initialized when JVM metrics disabled",
result.isInitialized());
+ disabledMetrics.close(); // must not throw
+ } finally {
+ System.setProperty("solr.metrics.jvm.enabled", "true");
+ }
Review Comment:
This test permanently forces `solr.metrics.jvm.enabled` back to "true" in
the `finally` block, which can leak state into other tests (and overrides cases
where the property was originally unset). Save the previous value and restore
it (or clear the property) after the test.
##########
solr/solr-ref-guide/modules/deployment-guide/pages/metrics-reporting.adoc:
##########
@@ -98,6 +98,21 @@ The `JVM Registry` gathers metrics from the JVM using the
OpenTelemetry instrume
JVM metrics are enabled by default but can be disabled by setting either the
system property `-Dsolr.metrics.jvm.enabled=false` or the environment variable
`SOLR_METRICS_JVM_ENABLED=false`.
+==== Physical Memory Metrics
+
+Solr exposes two additional gauges for host or container physical memory,
registered under the `solr.jvm` registry:
+
+[cols="2,1,3",options="header"]
+|===
+| Prometheus Metric Name | Type | Description
+| `jvm_system_memory_bytes` | gauge | Total physical memory of the host or
container in bytes. On Linux with cgroup memory limits, reflects the container
limit rather than host RAM.
+| `jvm_system_memory_free_bytes` | gauge | Free (unused) physical memory of
the host or container in bytes.
+|===
+
+NOTE: These metrics are available only on HotSpot-based JDK distributions
(Oracle JDK, Eclipse Temurin, Amazon Corretto, IBM Semeru, and others). On JVMs
that do not provide `com.sun.management.OperatingSystemMXBean`, the metrics are
silently absent.
Review Comment:
This NOTE claims the metrics are available only on “HotSpot-based JDK
distributions” and lists vendors, but the actual availability condition in code
is whether `com.sun.management.OperatingSystemMXBean` is present. To avoid
misinformation (and vendor lists that can easily be wrong/outdated), consider
rephrasing this note to only describe the concrete API requirement (and, if
desired, mention “most HotSpot-derived JVMs” without enumerating vendors).
```suggestion
NOTE: These metrics are available when the JVM provides the
`com.sun.management.OperatingSystemMXBean` interface (this includes most
HotSpot-derived JVMs). On JVMs that do not provide
`com.sun.management.OperatingSystemMXBean`, the metrics are silently absent.
```
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,36 @@ public ContextPropagators getPropagators() {
// TODO: We should have this configurable to enable/disable
specific JVM metrics
.enableAllFeatures()
.build();
+ java.lang.management.OperatingSystemMXBean osMxBean =
+ ManagementFactory.getOperatingSystemMXBean();
+ if (osMxBean instanceof com.sun.management.OperatingSystemMXBean
extOsMxBean) {
+ systemMemoryTotalGauge =
+ solrMetricManager.observableLongGauge(
+ registryName,
+ "jvm.system.memory",
+ "Total physical memory of the host or container in bytes."
+ + " On Linux with cgroup limits, reflects the container
memory limit.",
+ measurement -> {
+ long total = extOsMxBean.getTotalMemorySize();
+ if (total > 0) measurement.record(total);
+ },
+ OtelUnit.BYTES);
+ systemMemoryFreeGauge =
+ solrMetricManager.observableLongGauge(
+ registryName,
+ "jvm.system.memory.free",
+ "Free (unused) physical memory of the host or container in
bytes.",
+ measurement -> {
+ long free = extOsMxBean.getFreeMemorySize();
Review Comment:
`com.sun.management.OperatingSystemMXBean` does not define
`getFreeMemorySize()` on Java 21, so this won’t compile. Use the appropriate
method (e.g., `getFreePhysicalMemorySize()`) and ensure the metric
name/description matches the underlying meaning.
```suggestion
"Free (unused) physical memory of the host in bytes.",
measurement -> {
long free = extOsMxBean.getFreePhysicalMemorySize();
```
##########
solr/core/src/test/org/apache/solr/metrics/JvmMetricsTest.java:
##########
@@ -105,4 +107,51 @@ public void testSetupJvmMetrics() throws
InterruptedException {
"Should have JVM buffer metrics",
metricNames.stream().anyMatch(name -> name.startsWith("jvm_buffer")));
}
+
+ @Test
+ @SuppressForbidden(reason = "Testing
com.sun.management.OperatingSystemMXBean availability")
+ public void testSystemMemoryMetrics() {
+ PrometheusMetricReader reader =
+ solrTestRule
+ .getJetty()
+ .getCoreContainer()
+ .getMetricManager()
+ .getPrometheusMetricReader("solr.jvm");
+ MetricSnapshots snapshots = reader.collect();
+
+ Set<String> metricNames =
+ snapshots.stream()
+ .map(metric -> metric.getMetadata().getPrometheusName())
+ .collect(Collectors.toSet());
+
+ // Physical memory metrics are only present on HotSpot JDK
(com.sun.management available).
+ // If absent, the test is skipped (graceful degradation is tested
separately).
Review Comment:
The comment says “graceful degradation is tested separately”, but this class
doesn’t appear to include a test that asserts behavior when
`com.sun.management.OperatingSystemMXBean` is *not* available (this test is
just skipped). Either add such a test (e.g., by factoring OS bean access behind
a seam that can be substituted) or adjust the comment to avoid implying
coverage that isn’t present.
```suggestion
// If absent, the test is skipped.
```
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,36 @@ public ContextPropagators getPropagators() {
// TODO: We should have this configurable to enable/disable
specific JVM metrics
.enableAllFeatures()
.build();
+ java.lang.management.OperatingSystemMXBean osMxBean =
+ ManagementFactory.getOperatingSystemMXBean();
+ if (osMxBean instanceof com.sun.management.OperatingSystemMXBean
extOsMxBean) {
+ systemMemoryTotalGauge =
+ solrMetricManager.observableLongGauge(
+ registryName,
+ "jvm.system.memory",
+ "Total physical memory of the host or container in bytes."
+ + " On Linux with cgroup limits, reflects the container
memory limit.",
+ measurement -> {
+ long total = extOsMxBean.getTotalMemorySize();
+ if (total > 0) measurement.record(total);
+ },
Review Comment:
`com.sun.management.OperatingSystemMXBean` (at least on the project’s
minimum Java 21) does not define `getTotalMemorySize()`, so this won’t compile.
Use the correct API method (e.g., `getTotalPhysicalMemorySize()`), and keep the
metric description aligned with whatever semantics that method provides (host
vs container-aware).
--
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]
