Zipkin: Consider what to do about the prometheus library

[adriancole]

Until recently, we had our own prometheus exporter. In efforts to reduce code, we used the up-and-coming standard library. Unfortunately, this library is intentionally written to not cleanup after itself, noted for architecture reasons etc.

This leads to reloading problems because a metrics filter assumes it is a JVM singleton eventhough it lives in the scope of an application context. There's currently no way to unload it (without a terrible hack which clears the whole registry)
https://github.com/prometheus/client_java/issues/279

Here's an example error which displaced a fair amount of time to understand, as one wouldn't assume that a servlet filter initializes something it doesn't destroy!

2017-11-22 14:04:20.770 ERROR 26628 --- [ost-startStop-1] o.a.c.c.C.[Tomcat-1].[localhost].[/]     : Exception starting filter [prometheusMetricsFilter]

java.lang.IllegalArgumentException: Collector already registered that provides name: http_request_duration_seconds_count
        at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:54) ~[simpleclient-0.1.0.jar:na]
        at io.prometheus.client.SimpleCollector$Builder.register(SimpleCollector.java:245) ~[simpleclient-0.1.0.jar:na]
        at io.prometheus.client.SimpleCollector$Builder.register(SimpleCollector.java:237) ~[simpleclient-0.1.0.jar:na]
        at io.prometheus.client.filter.MetricsFilter.init(MetricsFilter.java:151) ~[simpleclient_servlet-0.1.0.jar:na]
        at org.apache.catalina.core.ApplicationFilterConfig.initFilter(ApplicationFilterConfig.java:285) ~[tomcat-embed-core-8.5.23.jar:8.5.23]
        at org.apache.catalina.core.ApplicationFilterConfig.<init>(ApplicationFilterConfig.java:112) ~[tomcat-embed-core-8.5.23.jar:8.5.23]
        at org.apache.catalina.core.StandardContext.filterStart(StandardContext.java:4591) [tomcat-embed-core-8.5.23.jar:8.5.23]
        at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5233) [tomcat-embed-core-8.5.23.jar:8.5.23]
        at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:150) [tomcat-embed-core-8.5.23.jar:8.5.23]
        at org.apache.catalina.core.ContainerBase$StartChild.call(ContainerBase.java:1419) [tomcat-embed-core-8.5.23.jar:8.5.23]
        at org.apache.catalina.core.ContainerBase$StartChild.call(ContainerBase.java:1409) [tomcat-embed-core-8.5.23.jar:8.5.23]
        at java.util.concurrent.FutureTask.run(FutureTask.java:266) [na:1.8.0_144]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [na:1.8.0_144]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [na:1.8.0_144]
        at java.lang.Thread.run(Thread.java:748) [na:1.8.0_144]

An attempt to work around this was made, but unfortunately closed with no remedy
https://github.com/prometheus/client_java/pull/302

Right now, we use the "terrible hack", but should track the above issues. If we run into more scenarios where the prometheus spring plugin doesn't work in spring environments, we should re-evaluate the tool choice. Reason being is that we can't afford dodgy code as it sets little timebombs which prevent us from doing meaningful work.

[adriancole]

cc @abesto fyi

[adriancole]

one way is to try "best of" for example, the prometheus spring boot setup is not written for dependency injection, hence this bug and also needing to call a magic static DefaultExports.initialize()

Another option is to use micrometer, which can handle the spring boot layer and still use the prometheus exporter. This is used by folks close to home such as @trustin iiuc as well, so while new, not first to fire. (Disclaimer: author @jkschneider is a colleague!)
https://github.com/micrometer-metrics/micrometer/blob/736fd95ca5a9fb9846699489877eb4021882b9df/micrometer-spring-legacy/src/main/java/io/micrometer/spring/autoconfigure/export/prometheus/PrometheusExportConfiguration.java

[abesto]

Well 💩 that's unexpected. Here's hoping the upstream issue gets a constructive resolution in the end.

I _guess_ if not, we could still use this lib and something utterly horrible on our end like exposing the collector with reflection and call cleanup ourselves at the right point in the Spring life-cycle. Deciding whether that's better, or replacing the library altogether, I'll leave to you.

[adriancole]

>

I guess if not, we could still use this lib and something utterly
horrible on our end like exposing the collector with reflection and call
cleanup ourselves at the right point in the Spring life-cycle. Deciding
whether that's better, or replacing the library altogether, I'll leave to
you.

yeah I started subclassing their filter and exactly that (via reflection),
then got depressed :P

[brian-brazil]

You need to use the same metric object across reloads, otherwise the metric will cause problems for users as it'll keep on resetting.

[adriancole]

You need to use the same metric object across reloads, otherwise the metric
will cause problems for users as it'll keep on resetting.

A metric that even before being used causes an inability to reload the
application scope under which it is defined is a problem you dont seem to
be able to hear. I wish you could

[brian-brazil]

Registering a metric counts as using it. The rest of your comment got cut off.

I pointed out this incompatibility in models as far back as when the Spring boot integration was being added, and I've yet to hear of any solutions proposed that make sense.

[adriancole]

I'm going to stop commenting on this issue for the day as the fact that this library makes a little bomb in our test suite is something I guess we'll have to deal with until @brian-brazil allows the library to be usable

[adriancole]

here's a workaround pull request, named for brian in hopes one day it will catch enough attention to reverse the madness https://github.com/openzipkin/zipkin/pull/1812

[adriancole]

https://github.com/openzipkin/zipkin/pull/1816 will fix this by not using the buggy filter. It was also double-counting stats, so not worth using anyway.

We can evaluate things like micrometer independently