mlbiscoc commented on code in PR #4209:
URL: https://github.com/apache/solr/pull/4209#discussion_r2927045246
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -74,6 +116,14 @@ public void close() {
if (runtimeMetrics != null && isInitialized) {
try {
runtimeMetrics.close();
+ if (systemMemoryTotalGauge != null) {
+ systemMemoryTotalGauge.close();
+ systemMemoryTotalGauge = null;
+ }
+ if (systemMemoryFreeGauge != null) {
+ systemMemoryFreeGauge.close();
+ systemMemoryFreeGauge = null;
+ }
Review Comment:
Close with `closeQuietly` then set to null in finally might be cleaner.
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,38 @@ 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);
Review Comment:
Same comment as above but having 0 free memory could be possible?
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,38 @@ 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);
Review Comment:
WDYT to make it just `jvm.system.memory` as the name and differ by attribute
like "state=total/free` or `type` because the prefixes are the same.
Or `jvm.system.memory` -> `jvm_system_memory_max`? I just want to better
have it in the names that this is `total` without having the suffix `total`
because then it will be confused with counters as `total` is reserved for that
type.
##########
changelog/unreleased/SOLR-18159-physical-memory-metrics.yml:
##########
@@ -0,0 +1,9 @@
+title: Add jvm_system_memory_bytes and jvm_system_memory_free_bytes gauges
exposing
+ total and free physical (host or container) memory to the Prometheus metrics
endpoint.
Review Comment:
I think its better to just drop `Prometheus metrics endpoint`. Just metrics
is fine since we are not only prometheus anymore.
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,38 @@ 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",
Review Comment:
I didn't know the prometheus writer would actually transform these delimited
by dots in to underscores. But since we are _ for everything else, then make
these underscores.
##########
solr/core/src/java/org/apache/solr/metrics/OtelRuntimeJvmMetrics.java:
##########
@@ -65,6 +75,38 @@ 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:
Curious why the > 0 check? Seems wrong but also could this actually be 0?
--
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]
