Third party libraries using Prometheus are advised by the Prometheus docs to "use the default CollectorRegistry
". I think that is because of code like this (from the kubernetes java client):
private static Gauge gaugeWorkQueueLength =
Gauge.build("controller_work_queue_length", "Current length of the controller's work-queue")
.labelNames("name")
.register();
The call to register()
at the end has no choice but to use the default CollectorRegistry
. In consequence, the kubernetes metrics are not exposed via /actuator/prometheus
. But they would be if we just used the default CollectorRegistry
, i.e.:
@Bean
public CollectorRegistry collectorRegistry() {
return CollectorRegistry.defaultRegistry;
}
If Spring Boot did that by default, users wouldn't have to figure this all out.
Comment From: wilkinsona
Thanks. Seems like a good change to make on the face of it. The current behaviour feels like a bug to me given the recommendation in CollectorRegistry
's javadoc:
The majority of users should use the
defaultRegistry
rather than instantiating their ownCreating a registry other than the default is primarily useful for unit tests, or pushing a subset of metrics to the Pushgateway from batch jobs.
I do wonder how much of a breaking changing this would be and, therefore, what branch we should make the change in. It's easy enough to restore the old behaviour which reduces the risk somewhat.
Comment From: wilkinsona
It's potentially quite a large breaking change. The default collector registry is static and a collector may only be registered if no previously registered collector provides the same names. This causes problems with tests as the state of the collector registry is shared between tests resulting in failures like this:
Caused by: java.lang.IllegalArgumentException: Collector already registered that provides name: jvm_gc_max_data_size_bytes
at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:54)
at io.prometheus.client.Collector.register(Collector.java:139)
at io.micrometer.prometheus.PrometheusMeterRegistry.lambda$applyToCollector$16(PrometheusMeterRegistry.java:410)
at java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1853)
at io.micrometer.prometheus.PrometheusMeterRegistry.applyToCollector(PrometheusMeterRegistry.java:406)
at io.micrometer.prometheus.PrometheusMeterRegistry.newGauge(PrometheusMeterRegistry.java:208)
at io.micrometer.core.instrument.MeterRegistry.lambda$gauge$1(MeterRegistry.java:297)
at io.micrometer.core.instrument.MeterRegistry.lambda$registerMeterIfNecessary$5(MeterRegistry.java:561)
at io.micrometer.core.instrument.MeterRegistry.getOrCreateMeter(MeterRegistry.java:614)
at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:568)
at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:561)
at io.micrometer.core.instrument.MeterRegistry.gauge(MeterRegistry.java:297)
at io.micrometer.core.instrument.Gauge$Builder.register(Gauge.java:190)
at io.micrometer.core.instrument.composite.CompositeGauge.registerNewMeter(CompositeGauge.java:58)
at io.micrometer.core.instrument.composite.CompositeGauge.registerNewMeter(CompositeGauge.java:27)
at io.micrometer.core.instrument.composite.AbstractCompositeMeter.add(AbstractCompositeMeter.java:66)
at java.lang.Iterable.forEach(Iterable.java:75)
at java.util.Collections$SetFromMap.forEach(Collections.java:5530)
at io.micrometer.core.instrument.composite.CompositeMeterRegistry.lambda$new$0(CompositeMeterRegistry.java:65)
at io.micrometer.core.instrument.composite.CompositeMeterRegistry.lock(CompositeMeterRegistry.java:184)
at io.micrometer.core.instrument.composite.CompositeMeterRegistry.lambda$new$1(CompositeMeterRegistry.java:65)
at io.micrometer.core.instrument.MeterRegistry.getOrCreateMeter(MeterRegistry.java:624)
at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:568)
at io.micrometer.core.instrument.MeterRegistry.registerMeterIfNecessary(MeterRegistry.java:561)
at io.micrometer.core.instrument.MeterRegistry.gauge(MeterRegistry.java:297)
at io.micrometer.core.instrument.Gauge$Builder.register(Gauge.java:190)
at io.micrometer.core.instrument.binder.jvm.JvmGcMetrics.bindTo(JvmGcMetrics.java:109)
at org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryConfigurer.lambda$2(MeterRegistryConfigurer.java:86)
at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
at java.util.ArrayList.forEach(ArrayList.java:1257)
at java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
at java.util.stream.Sink$ChainedReference.end(Sink.java:258)
at java.util.stream.Sink$ChainedReference.end(Sink.java:258)
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:483)
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:485)
at org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryConfigurer.addBinders(MeterRegistryConfigurer.java:86)
at org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryConfigurer.configure(MeterRegistryConfigurer.java:68)
at org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryPostProcessor.postProcessAfterInitialization(MeterRegistryPostProcessor.java:64)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.applyBeanPostProcessorsAfterInitialization(AbstractAutowireCapableBeanFactory.java:444)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1792)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:609)
... 101 more
Generally speaking, Micrometer's meter binders don't have any clean up methods that could be used to unregister their meters that could be called during application context close processing. It wouldn't be a complete solution even if they did as context caching in integration tests may result in multiple contexts being open at the same time.
In 2.4, we no longer export metrics by default in integration tests but we'd still need something that works with @AutoConfigureMetrics
. Perhaps it could trigger the auto-configuration of a CollectorRegistry
bean that isn't the default registry? That won't help with application context runner-based tests though.
I'm coming to the conclusion that using the default registry by default may actually do more harm than good. @jkschneider came to the same conclusion three years ahead of me.
@dsyer Where did you read the recommendation to use the default registry? Googling for your quote above ("use the default CollectorRegistry") turns up this issue and not much else.
Comment From: dsyer
Where did you read the recommendation to use the default registry?
In the javadocs for CollectorRegistry
.
I think Jon's decision was based on the same discussion about integration tests. It seems a shame to pollute the runtime with behaviour that is designed mainly for testing. Would it be better to try and do something different in tests automatically, and stick to the default registry at runtime?
Comment From: wilkinsona
It seems a shame to pollute the runtime with behaviour that is designed mainly for testing
It seems a shame to me for the runtime behaviour to pollute static state when we have dependency injection.
I'm increasingly convinced that we shouldn't be encouraging the use of static state. Looking at the Kubernetes Java Client, it'll also break in an environment where Prometheus is loaded in a shared parent class loader and the K8S client is loaded in two or more child class loaders.
Would it be better to try and do something different in tests automatically
That's what I was thinking out load about above. We could do something for @SpringBootTest
, but it wouldn't help with ApplicationContextRunner
-based tests.
Fundamentally, I disagree with a design decision in Prometheus and wish that the default registry wasn't shared static state. Unfortunately, that decision has leaked out into the K8S Java Client and probably other places too. I guess it come down to deciding whether we want to further the leak by changing Boot to use the static registry by default. In an environment where dependency injection's available, I'm finding it hard to conclude that that's the right thing to do.
Comment From: dsyer
Isn't there something we can do with the default registry in /actuator/prometheus
then? Tommy pointed out that there might be overlaps or clashes in the metric names, but we can detect that and decide somehow on a priority for the exporter.
Comment From: wilkinsona
It's not just /actuator/prometheus
but also the auto-configuration of the push gateway. It would add quite a lot of complexity to support both the injected CollectorRegistry
and the default static registry as we'd probably have to add config options to control the behaviour, which took priority, etc.
Our feeling is that it would be preferable for the Kubernetes Java Client to allow a different CollectorRegistry
to be used. It could use the static default registry by default as long as it's configurable. In our opinion, that would make the Java client much better suited to use in a DI environment.
Comment From: dsyer
OK. I'll see if I can make that change.
Comment From: wilkinsona
@dsyer How did things go on the K8S Java Client side of this?
Comment From: dsyer
We made a change there so that it configures Spring Boot to use the default registry in autoconfig. I guess we can close this (with some docs, maybe?).
Comment From: wilkinsona
That sounds bad. If you're using the K8S Java Client, you'll now end up with the pollution problems associated with the static default registry. Tests are likely to start breaking, for example. Documenting the K8S Java Client's behaviour feels out of scope for Boot's reference documentation and I really think it should be changed.
I guess we could document how to use of the default registry, cautioning strongly against doing so but that feels a bit odd too: here's how to do something that we think you really shouldn't do…
Comment From: philwebb
I think extra documentation in Boot would add noise and probably wouldn't be relevant for most typical app developers who are hopefully not using the K8S Java Client. We'll close this one for now.