Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When using @Timed, recommend that care is taken to avoid double instrumentation #44037

Open
helpermethod opened this issue Jan 31, 2025 · 4 comments
Labels
type: documentation A documentation update
Milestone

Comments

@helpermethod
Copy link
Contributor

helpermethod commented Jan 31, 2025

Today, I've stumbled over this issue:

micrometer-metrics/micrometer#780

It seems that this happens not only when annotating controller methods with @Timed but also when annotating Spring Data repositories (and possibly other beans).

Is there some way for Spring Boot to prevent this?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 31, 2025
@wilkinsona
Copy link
Member

wilkinsona commented Jan 31, 2025

Unfortunately, I don't think there's anything that Boot can do here as it doesn't have the necessary information to detect a double registration. To do so, it would have to:

  1. Detect @Timed on a method
  2. Detect that the TimedAspect is in play
  3. Know that the annotated method is also instrumented by the underlying framework

1 is possible. 2 is harder as it relies both on a TimedAspect bean and the necessary AOP machinery being in place. 3 is the real stumbling block as it isn't possible without coupling Boot more tightly than we would like to behavior in Spring MVC, Spring Data, etc.

If you've opted into using @Timed (or @Counted), you may also want to consider defining TimedAspect yourself with a predicate that performs your desired filtering, as described in this comment from @jonatan-ivanov.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2025
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 31, 2025
@helpermethod
Copy link
Contributor Author

helpermethod commented Jan 31, 2025

Hi @wilkinsona,

thanks for the detailed answer, that makes sense.

I've used the predicate workaround to solve this issue, but it took me quite a while to find out what the actual problem was. Maybe the metrics chapter @Timed usage should link to the workaround / give some hints. Wdyt?

@wilkinsona wilkinsona changed the title Warn / prevent double micrometer metric registrations When using @Timed, recommend that care if taken to avoid double instrumentation Jan 31, 2025
@wilkinsona wilkinsona added type: documentation A documentation update and removed status: declined A suggestion or change that we don't feel we should currently apply labels Jan 31, 2025
@wilkinsona wilkinsona added this to the 3.3.x milestone Jan 31, 2025
@wilkinsona wilkinsona reopened this Jan 31, 2025
@wilkinsona
Copy link
Member

Yes, I think we could add something to https://docs.spring.io/spring-boot/3.3/reference/actuator/metrics.html#actuator.metrics.supported.timed-annotation. Thanks for the suggestion.

@jonatan-ivanov
Copy link
Member

FWIW, I think by adding @Timed/@Observed/etc. to something that is already instrumented out of the box, (without disabling that instrumentation) you are explicitly asking for double instrumentation (built-in + annotation).
Also, since annotating a controller method with @Timed/@Observed/etc. is technically not exactly the same (Spring Framework instruments these on the ServletFilter level), this "double instrumentation" could be a ~valid use-case if you want to distinguish between just the method call and the method call + Spring overhead.

Because of these, I think Boot should not do anything but making it easy to turn auto-instrumentation on/off (this is already happening).

@wilkinsona For the doc change, could you please also consider @Counted, @Observed, and @NewSpan next to @Timed?

@wilkinsona wilkinsona changed the title When using @Timed, recommend that care if taken to avoid double instrumentation When using @Timed, recommend that care is taken to avoid double instrumentation Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

4 participants