Update metrics documentation to note the benefits of using a MeterBinder when registering a metric that relies on other beans #19557
Comments
spring-projects-issues
added
the
status: waiting-for-triage
|
This was fixed in #12006 but #12121 removed the need to declare the destroy method explicitly. The tests written in #12006 are still green so I would think the stop methods are called when necessary. @waschmittel Could you provide a small sample with steps on how to reproduce the issue? |
mbhave
added
the
status: waiting-for-feedback
|
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed. |
spring-projects-issues
added
the
status: feedback-reminder
|
Tried to reproduce ... the first thing Datadog's PushMeterRegistry does when close() is called, is calling publish(). So this seems to be a Datadog bug after all. |
|
Oh wait ... PushMeterRegistry is a part of micrometer (in io.micrometer.core.instrument.push) So PushMeterRegistry's close() looks like this: @Override
public void close() {
if (config.enabled()) {
publishSafely();
}
stop();
super.close();
}And publishSafely looks like this: private void publishSafely() {
try {
publish();
} catch (Throwable e) {
logger.warn("Unexpected exception thrown while publishing metrics for " + this.getClass().getSimpleName(), e);
}
}So Micrometer apparently expects metrics to still be extractable when close() is called. So I guess Spring should handle a PushMeterRegistry differently from a Non-Push MeterRegistry - Although I assume it would be difficult to call close() while the context is still fully available. So I can can probably my metrics to handle the Exceptions that occur while the context is already being closed, but I would prefer a generic solution for this issue, of course. |
spring-projects-issues
added
status: feedback-provided
@waschmittel I think the intention is to let the registry publish metrics "safely" (as the method name states) before closing it. The problem is that other parts of the infrastructure may have already been closed when the registry itself is closed. Perhaps the error message is misleading and can be refined (but that would have to be dealt in Micrometer itself). Paging @shakuzen for insights. |
|
Or Spring could do something to make config.enabled() false before calling close() (if possible - I have no idea). |
Why would we do that? I'd rather understand the intention of Micrometer rather than trying to force it to do something different. Doing something different may be the right call after all but there is no reason it should be specific to a spring app. |
|
Hi team, Any resolution to this issue as me too having the same concern and need some explanation to put up. Thanks in advance. |
|
@arijitdeb1 I pinged @shakuzen for feedback and we need to follow-up. Thanks for the nudge, I've flagged this one for team attention. |
snicoll
added
the
for: team-attention
wilkinsona
added
status: blocked
|
We might be able to do something with |
|
Sorry for not responding before. The intention of calling publish on close is to do a final publish so any changes in metrics between the previous publish (possibly never for a short-lived application) and the registry being closed are not lost.
If that is possible, it sounds like the route to go. If that could be achieved, I would expect there won't be any exceptions thrown on the final publish (unless exceptions are being thrown on other publishes for some reason). |
|
Thinking some more about I think a more natural way to fix this would be via dependency relationships between the various beans that are involved. In this context, it's interesting that Spring Data is apparently triggering bean creation. That suggests that the bean in question isn't being injected and, therefore, that the container probably doesn't have a depends on relationship between the repository and the bean. @waschmittel I think we're back to needing a sample, please. I'd like to understand exactly what's happening in terms of the bean that are involved in getting the gauge's value and the dependency relationships that are in play when the application context is being closed. |
wilkinsona
added
status: waiting-for-feedback
|
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed. |
spring-projects-issues
added
the
status: feedback-reminder
|
I was unpexcedly busy this week, but I will try to set up a sample project that reproduces this next week. |
spring-projects-issues
added
the
status: feedback-provided
spring-projects-issues
removed
status: waiting-for-feedback
snicoll
added
status: waiting-for-feedback
|
Please take a look at https://github.com/waschmittel/spring-boot-19557 that reproduces this @wilkinsona |
spring-projects-issues
added
status: feedback-provided
|
Thanks for the sample, @waschmittel. I've reproduced the problem. Here's the full stack trace of the failure: A bean lookup is being performed to determine the persistence exception translator to use. The exception that has triggered this is due to an attempt to start a transaction when the entity manager factory has been closed. |
|
You can avoid the problem by adding the following @Bean
static AbstractDependsOnBeanFactoryPostProcessor entityManagerFactoryMeterRegistryDependency() {
return new AbstractDependsOnBeanFactoryPostProcessor(DatadogMeterRegistry.class, EntityManagerFactory.class) {};
}This ensures that the meter registry is closed before the |
|
On closer inspection, the lack of dependency relationships between the various beans that are involved is due to how the The registration is being performed as a side-effect of the Switching to use a In short, there's no need for the workaround above if the gauge is registered using a diff --git a/src/main/java/com/example/demo/MetricsConfig.java b/src/main/java/com/example/demo/MetricsConfig.java
index d3e77a2..04020ea 100644
--- a/src/main/java/com/example/demo/MetricsConfig.java
+++ b/src/main/java/com/example/demo/MetricsConfig.java
@@ -1,16 +1,20 @@
package com.example.demo;
-import io.micrometer.core.instrument.Gauge;
-import io.micrometer.core.instrument.MeterRegistry;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
+import io.micrometer.core.instrument.Gauge;
+import io.micrometer.core.instrument.MeterRegistry;
+import io.micrometer.core.instrument.binder.MeterBinder;
+
@Configuration
public class MetricsConfig {
- @Bean
- public Gauge queueSize(MeterRegistry registry, QueueItemRepository repository) {
- return Gauge.builder("queueSize", repository::count).register(registry);
+ @Bean
+ public MeterBinder queueMeterBinder(QueueItemRepository repository) {
+ return (registry) -> {
+ Gauge.builder("queueSize", repository::count).register(registry);
+ };
}
}We should update the documentation to recommend broader use of a |
wilkinsona
added
type: documentation
wilkinsona
changed the title
|
Hi @wilkinsona - after upgrading to the latest spring-boot 3.1.5 we've spot similar issue. I've reproduced it using the same project from previous comment - you can check it: https://github.com/wojciechkedziora/spring-boot-3-19557 (changes: spring boot version + MeterBinder solution from your previous comment). Applying short-term solution from this post obviously resolves the issue. Could you take a look? Thanks |
|
Thanks, @wojciechkedziora. I've reproduced the problem with your updated sample and can see that things have changed since 2.2. I'm not yet sure of the cause of that, but it may be due to #30636. Please open a new issue so that we can investigate further. |
|
@wilkinsona Thanks for your answer. I've created new issue - #38240. |
We use Micrometer in our Spring Boot applications.
When shutting down our applications we sometimes get WARN log messages with Exceptions that say:
All of these value functions fail because some part of the application is already shut down. For example, we have a metric that's based on a Spring Data Repository, so we get
I assume this would be avoidable if publishing of Metrics is stopped by calling
MeterRegistry.clear()andPushMeterRegistry.stop()when the shutdown begins.(We are using Datadog, which apparently implements PushMeterRegistry)
The text was updated successfully, but these errors were encountered: