Fix consumers of JVM CPU metrics renamed by OTel Prometheus exporter update - #4711
Fix consumers of JVM CPU metrics renamed by OTel Prometheus exporter update#4711janhoy wants to merge 5 commits into
Conversation
The OpenTelemetry Prometheus exporter 1.63 (updated in apache#4594) no longer maps unit "1" to a "_ratio" name suffix, so the JVM metric previously exposed as jvm_system_cpu_utilization_ratio is now exposed as jvm_system_cpu_utilization. Update the three consumers still using the old name: NodeMetricImpl.SYSLOAD_AVG and NodeValueFetcher.SYSLOADAVG (broke PlacementPluginIntegrationTest.testAttributeFetcherImpl / replica placement attribute fetching) and CPUCircuitBreaker (silently returned -1 for CPU usage).
The OTel Prometheus exporter rename of jvm_system_cpu_utilization_ratio and jvm_cpu_recent_utilization_ratio (unit "1" no longer maps to a _ratio suffix) breaks dashboards, alerts and API clients using the old names. FilterablePrometheusMetricReader now duplicates these gauges under their legacy names, in both full scrapes and name-filtered /admin/metrics requests.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Thanks for catching this!
FWIW I think no need for metric name backwards-compatibility break in 10.1.
There was a problem hiding this comment.
wow; I suppose whatever tests exist for CPU circuit breaker are really lacking.
There was a problem hiding this comment.
TestCircuitBreakers should add a check that org.apache.solr.util.circuitbreaker.CPUCircuitBreaker#calculateLiveCPUUsage returns >= 0 (not -1).
There was a problem hiding this comment.
Problem was that it was impossible to distinguish if the -1 was because the JVM did not expose cpu usage or because wrong metric name was used. Have added a separate test for whether cpu is available at all, and now assert that liveCPUUsage() returns >0.
Meaning you agree with the aliasing of metric name? A bit confused by your "no need for" wording. |
|
Sorry; my statement was highly ambiguous. I mean we needn't bother trying to be backwards-compatible with the 10.0 name, IMO. |
…s; test CPU breaker Per review, remove the FilterablePrometheusMetricReader alias shim and instead document the jvm_system_cpu_utilization / jvm_cpu_recent_utilization Prometheus renames in major-changes-in-solr-10.adoc under 10.1. Add a TestCircuitBreakers test asserting calculateLiveCPUUsage() >= 0, gated by an independent native-CPU probe so a metric-name regression fails rather than being mistaken for an unsupported machine.
github-actions
Bot
added
documentation
I dropped the shim and instead added a bullet in upgrade-notes about the name change. |
2 participants
The Telemetry update in #4594 brought in a Prometheus exporter (1.57+) that no longer maps OTel unit
"1"to a_rationame suffix, renamingjvm_system_cpu_utilization_ratio→jvm_system_cpu_utilizationandjvm_cpu_recent_utilization_ratio→jvm_cpu_recent_utilization. This broke replica placement attribute fetching (CI failure inPlacementPluginIntegrationTest.testAttributeFetcherImpl) and silently brokeCPUCircuitBreaker(calculateLiveCPUUsage()returned -1).NodeMetricImpl,NodeValueFetcher,CPUCircuitBreaker) to the new metric namesmajor-changes-in-solr-10.adoc), since 9x and 10x will also get this OTel upgrade — no back-compat shimTestCircuitBreakerstest assertingcalculateLiveCPUUsage()returns >= 0, gated by an independent native-CPU probe (com.sun.management.OperatingSystemMXBean.getCpuLoad()) so a future metric-name regression fails instead of being mistaken for an unsupported machine