[Spring Boot] Collector registered multiple times #279
Comments
|
A given metric is only meant to be registered once, typically via a class static initialiser so this doesn't usually come up in standard usage. Which metrics are conflicting exactly? |
|
Here is the example: https://github.com/lupo112/prometheus_client_java_279 If you try to run all test, it will fail on: Or dev server does not work. Start server and then try to change any file and recompile. It should reload given class. It will failed on: |
|
The recommendation is to make your metric |
|
Thanks, but that will resolve only the first problem. Second problem with dev server still remains: |
|
@lupo112 Great point since the dev server reloads classes for quicker dev experience |
|
@checketts exactly! |
|
With the way metrics work, you need a way to use the same metric object across reloads. |
|
Hello, I have the same issue with a spring-cloud app. In the CollectorRegistry line 192 function filter() I found the following: Thanks Olli |
|
That's unrelated. This issue is due to using the simpleclient incorrectly. |
|
I have the same problem. When using @PrometheusTimeMethod. Some of my @SpringBootTest tests are annotated with @DirtiesContext. When spring creates a new context the metrics are registered again: Collector already registered that provides name: endpoint_availability_count |
|
as a possible workaround in tests: @After
public void tearDown() {
CollectorRegistry.defaultRegistry.clear();
} |
|
The following hack avoids the dev-tools reload problem: package demo;
import io.prometheus.client.CollectorRegistry;
import io.prometheus.client.spring.boot.EnablePrometheusEndpoint;
import io.prometheus.client.spring.boot.EnableSpringBootMetricsCollector;
import org.springframework.context.annotation.Configuration;
@Configuration
// Registers /prometheus endpoint
@EnablePrometheusEndpoint
// Exposes spring boot metrics via the prometheus endpoint
@EnableSpringBootMetricsCollector
class PrometheusConfig {
static {
//HACK Avoids duplicate metrics registration in case of Spring Boot dev-tools restarts
CollectorRegistry.defaultRegistry.clear();
}
} |
|
MetricsFilter implicitly registers a new collector (which you cannot access as it is private) on init. It should deregister it on destroy. What would be the harm of using lifecycle such as this? |
Once a metric is registered, it should stay registered until the process terminates. Time series should not appear and disappear over the lifetime of the process, that's difficult to deal with semantically. |
|
My understanding of the problem is the following:
Am I missing something else here? I just wanted to get as clear a picture as possible of what's going on here because I am sure that there are (or will be in the future) many people that will stumble upon this issue and scratch there heads. |
|
The following servlet filter code makes it impractical to comply with @brian-brazil's desire As you'll notice the metric itself is instantiated and then cached as a private field which we cannot access. Somehow, we are supposed to reuse it when the application reloads. I suppose this means we are supposed to now cache the filter object itself. Basically it is extremely unpractical advise. If there's a desire to ensure only one metric is made in a JVM singleton, there are other approaches to do that. For example, you can idempotently create the metric (by caching its input and if equals don't try again, but do that in the layer that's enforcing everything) Right now, feedback feels quite draconian, limited and arbitrary |
That's a new issue. At a first glance, it is indeed the filter that would need to be reused, or use your own metrics.
We purposefully avoid that on performance grounds. See https://www.robustperception.io/label-lookups-and-the-child/ That sounds like something you might be able to do within Spring Boot though.
The problem as I see it is that you are trying to do something quite complicated (dynamic class reloads) with known drawbacks, and you ran into the drawbacks. I can only look at these things from the Prometheus standpoint, and help you avoid the pitfalls there. I'm afraid I can't hand you a solution as I'm not a Spring Boot expert. |
|
@geoand Yes on the first point. I'd count development as production for the 2nd point ("development" is very broad), but yes on unittests. |
|
@brian-brazil Thanks for clearing that up |
|
To provide some general background on why this is an error: A common user mistake is to not make their metrics |
|
A very crude and also Spring Boot specific solution for sidestepping the problem of metrics getting registered multiple time in tests that create more than one Create a class named Now on each test class (or perhaps only on an Abstract Base class which all Spring tests extend) would look something like the following: Take note that the test above is a JUnit 4 test, but could easily be written in JUnit 5 with the necessary changes. |
|
In zipkin, we are no longer using a servlet filter for http duration. Here's the code we used for a very short while due to this issue. Hopefully will help give a different perspective even if not integrated here. Hope it helps someone. /**
* The normal prometheus metrics filter implicitly registers a histogram which is hidden in a
* field and not deregistered on destroy. A registration of any second instance of that filter
* fails trying to re-register the same collector by design (by brian-brazil). The rationale is
* that you are not supposed to recreate the same histogram. However, this design makes it
* impossible to re-init a servlet (ex in spring boot tests).
*
* <p>This filter replaces the normal prometheus filter, correcting the design flaw by allowing us
* to re-use the JVM singleton. It also corrects a major flaw in the upstream filter which results
* in double-counting of requests when they are performed asynchronously.
*/
static final class PrometheusDurationFilter implements Filter {
final Histogram httpRequestLatency;
PrometheusDurationFilter(Histogram httpRequestLatency) { // << pass singleton here
this.httpRequestLatency = httpRequestLatency;
}
@Override public void init(FilterConfig filterConfig) {
// If you need a no-args ctor, this implies the field httpRequestLatency is null.
// you could look for a context parameter for the histogram (ex assigned by a listener)
// failing that, you can lazy-init/register a new one. This impl just accepts a pre-ordained
// staticly initialized histogram.
}
@Override public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse,
FilterChain filterChain) throws IOException, ServletException {
HttpServletRequest request = (HttpServletRequest) servletRequest;
// async servlets will enter the filter twice
if (request.getAttribute("PrometheusDurationFilter") != null) {
filterChain.doFilter(request, servletResponse);
return;
}
request.setAttribute("PrometheusDurationFilter", "true");
Histogram.Timer timer = httpRequestLatency
.labels(request.getRequestURI(), request.getMethod())
.startTimer();
try {
filterChain.doFilter(servletRequest, servletResponse);
} finally {
if (request.isAsyncStarted()) { // we don't have the actual response, handle later
request.getAsyncContext().addListener(new CompleteTimer(timer));
} else { // we have a synchronous response, so we can finish the recording
timer.observeDuration();
}
}
}
@Override public void destroy() {
}
}
/** Inspired by WingtipsRequestSpanCompletionAsyncListener */
static final class CompleteTimer implements AsyncListener {
final Histogram.Timer timer;
volatile boolean completed = false;
CompleteTimer(Histogram.Timer timer) {
this.timer = timer;
}
@Override public void onComplete(AsyncEvent e) {
tryComplete();
}
@Override public void onTimeout(AsyncEvent e) {
tryComplete();
}
@Override public void onError(AsyncEvent e) {
tryComplete();
}
/** Only observes the first completion event */
void tryComplete() {
if (completed) return;
timer.observeDuration();
completed = true;
}
/** If another async is created (ex via asyncContext.dispatch), this needs to be re-attached */
@Override public void onStartAsync(AsyncEvent event) {
AsyncContext eventAsyncContext = event.getAsyncContext();
if (eventAsyncContext != null) eventAsyncContext.addListener(this);
}
} |
|
Thank you for sharing your code, would someone like to send in a PR to fix the async issue? |
|
the workaround for spring-dev-tools by clearing the registry... just works! are there any 'hidden' side affects for that? |
|
The only issue I can see for tests is if you had a test depending on it not being cleared. I haven't heard of anyone running into that one. |
|
This has gone a bit stale, and I don't think there's anything for us to do here especially as there's Spring 2 now which we don't support. |
|
even after doing this my tests are flaky, sometimes they pass, but other time they fail |
|
This is a very old closed issue. Please create a new one, and provide more context, like which Spring Boot version you are using and how you are using |
Hello
In Spring Boot implementation there are some issues with registering of Collectors:
@SpringBootTestconfigurations, some tests failed on multiple registering of the same collectorAfter some experiments, I have found the solution. The problem is that it is used
CollectorRegistry.defaultRegistry. Using created bean, all works. Can you add support for injecting of CollectorRegistry as Bean? For example:The text was updated successfully, but these errors were encountered: