Add Compilation JVM metric binder #1589
Conversation
gquintana
force-pushed
the
jvm-compilation-binder
branch
from
234e164
to
2becdda
Compare
micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jvm/CompilationMetrics.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jvm/CompilationMetrics.java
Outdated
Show resolved
Hide resolved
| MeterRegistry registry = new SimpleMeterRegistry(); | ||
| new CompilationMetrics().bindTo(registry); | ||
|
|
||
| assertThat(registry.get("jvm.compilation.time.total").gauge().value()).isGreaterThan(0); |
There was a problem hiding this comment.
Will this test fail on builds that run on alternate JDKs? I'm not too familiar with the build setup in that regard @shakuzen
There was a problem hiding this comment.
I was of the false memory that we already had some kind of matrix build which don't just test the different Hotspot JVMs. Apparently that isn't the case. Was quickly testing a ./gradlew test with 8u222-openJ9 and there's quite some test failures before I could reach this one. (Hope I didn't screw up.)
There was a problem hiding this comment.
Is it a problem in the end?
There was a problem hiding this comment.
I was of the false memory that we already had some kind of matrix build which don't just test the different Hotspot JVMs. Apparently that isn't the case.
I opened an issue for it but never got around to doing it - #1462. If a test is specific to a JVM, you can use JUnit Assumptions to skip it when those assumptions aren't meant. We do this for example in the ProcessorMetricsTest:
gquintana
force-pushed
the
jvm-compilation-binder
branch
from
8ef8b8b
to
fd4d499
Compare
|
|
||
| @NonNullApi | ||
| @NonNullFields | ||
| public class CompilationMetrics implements MeterBinder { |
There was a problem hiding this comment.
@shakuzen @jkschneider Should this class be named JvmCompilationMetrics since these are metrics around the JVM?
There was a problem hiding this comment.
I don't mind renaming the class but there is already a ClassLoaderMetrics class which should be renamed JvmClassLoaderMetrics in that case.
There was a problem hiding this comment.
I agree JvmCompilationMetrics is more clear here. ClassLoaderMetrics is already released public API so we can't change that without breaking all users of it.
| public void bindTo(MeterRegistry registry) { | ||
| CompilationMXBean compilationBean = ManagementFactory.getCompilationMXBean(); | ||
| if (compilationBean.isCompilationTimeMonitoringSupported()) { | ||
| FunctionCounter.builder("jvm.compilation.time.total", compilationBean, CompilationMXBean::getTotalCompilationTime) |
There was a problem hiding this comment.
I am unsure about the naming and the baseUnit usage below.
The PrometheusNamingConvention adds the unit and then the "_total" suffix again due to how it works for Counters:
jvm_compilation_time_total_ms_total
Unfortunately we can't make this a Timer due to not being triggered by the JVM so we don't really have any counts on compilation frequency.
There was a problem hiding this comment.
What about jvm.compilation.ms.total ?
There was a problem hiding this comment.
That will become jvm_compilation_ms_total_ms_total because the PrometheusNamingConvention will first add the base unit and then the _total.
How about just jvm.compilation.time and any NamingConvention specific to a registry implementation will make sure that it generates whatever is best practice for that backend. For Prometheus this would then become jvm_compilation_time_ms_total.
There was a problem hiding this comment.
jvm.compilation.time sounds good to me.
shakuzen
added
the
spring-boot change
| public void bindTo(MeterRegistry registry) { | ||
| CompilationMXBean compilationBean = ManagementFactory.getCompilationMXBean(); | ||
| if (compilationBean.isCompilationTimeMonitoringSupported()) { | ||
| FunctionCounter.builder("jvm.compilation.time.total", compilationBean, CompilationMXBean::getTotalCompilationTime) |
There was a problem hiding this comment.
jvm.compilation.time sounds good to me.
|
Polish in 114d62b |
Add a metric binder to get JVM compilation time