Remove some metrics that are already covered in upstream #1005
Conversation
...ava/com/splunk/opentelemetry/instrumentation/jvmmetrics/otel/OtelJvmHeapPressureMetrics.java
Outdated
Show resolved
Hide resolved
| // runtime.jvm.threads.peak was removed; easy to replace with | ||
| // max(process.runtime.jvm.threads.count) | ||
| // runtime.jvm.threads.daemon is replaced by OTel process.runtime.jvm.threads.count{daemon=true} | ||
| // runtime.jvm.threads.live is replaced by OTel process.runtime.jvm.threads.count{daemon=false} | ||
|
|
There was a problem hiding this comment.
There is one more meter runtime.jvm.threads.states in this class: there is currently no equivalent in OTel metrics, but there's an issue requesting this: open-telemetry/opentelemetry-java-instrumentation#7006
I think we can leave it as it is for now, hopefully at some point it'll get implemented in the upstream
| .setDescription("Size of long-lived heap memory pool after reclamation.") | ||
| .buildWithCallback(measurement -> measurement.record(liveDataSize.get())); | ||
| // runtime.jvm.gc.live.data.size is replaced by OTel | ||
| // process.runtime.jvm.memory.usage_after_last_gc{pool=<long lived pools>} | ||
|
|
There was a problem hiding this comment.
There are two other metrics left in this class: runtime.jvm.gc.memory.allocated and runtime.jvm.gc.memory.promoted. Both of these emit data that needs to be calculated on the instrumentation side, I think it's probably not possible to reproduce these by querying the backend (dashboards).
Still, I think we should remove them: both of them depend on hard-coded GC & memory pool names, long-term maintaining them and ensuring they're correct for all GC possible implementations is going to be painful.
Also I don't know how useful these really are, maybe the "standard" memory & GC metrics are enough anyway?
There was a problem hiding this comment.
Also, I wonder if the process.runtime.jvm.memory.allocated could perhaps replace runtime.jvm.gc.memory.allocated?
There was a problem hiding this comment.
Ok I'm good with that. It does seem weird that we have memory.allocated inside a runtime.jvm.gc namespace anyway.
I'm not sure if you saw the JFR based approach in contrib...but it's also nontrivial and bakes in some expectations about names.
There was a problem hiding this comment.
I'm not sure if you saw the JFR based approach in contrib...but it's also nontrivial and bakes in some expectations about names.
Yeeaaaah... from what I read, it seems to be similar (I can't really tell if it's 100% equivalent) to what we're doing in the profiling metric.
| .setDescription("Size of long-lived heap memory pool after reclamation.") | ||
| .buildWithCallback(measurement -> measurement.record(liveDataSize.get())); | ||
| // runtime.jvm.gc.live.data.size is replaced by OTel | ||
| // process.runtime.jvm.memory.usage_after_last_gc{pool=<long lived pools>} | ||
|
|
There was a problem hiding this comment.
Ok I'm good with that. It does seem weird that we have memory.allocated inside a runtime.jvm.gc namespace anyway.
I'm not sure if you saw the JFR based approach in contrib...but it's also nontrivial and bakes in some expectations about names.
mateuszrzeszutek
force-pushed
the
metrics-cleanup
branch
from
d5754a5 to
a7b287c
Compare
| // process.runtime.jvm.gc.duration{gc!=<concurrent gcs>} | ||
| // runtime.jvm.gc.max.data.size is replaced by OTel | ||
| // process.runtime.jvm.memory.limit{pool=<long lived pools>} | ||
| // runtime.jvm.gc.live.data.size is replaced by OTel |
There was a problem hiding this comment.
The runtime.jvm.gc.pause and runtime.jvm.gc.live.data.size metrics are used by Splunk Memory Profiling, we should not remove them from the agent until there is full server-side support for the new metrics.
mateuszrzeszutek
force-pushed
the
metrics-cleanup
branch
from
a7b287c to
3061a19
Compare
Removed some of the duplicated metrics (OTel-based) that are already emitted in the upstream javaagent (mostly GC related)