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

Built-in support for Micrometer metrics publishing #458

Closed
szpak opened this issue Jul 29, 2024 · 6 comments · Fixed by #459
Closed

Built-in support for Micrometer metrics publishing #458

szpak opened this issue Jul 29, 2024 · 6 comments · Fixed by #459
Milestone

Comments

@szpak
Copy link
Contributor

szpak commented Jul 29, 2024

It would be useful to provide Micrometer metrics related to spring-retry functionality (e.g. number of successful/failed retries (per retry/label) or retry execution times). Preferably, it could be done in the declarative way (no extra code/changes in the end project) - the feature is enabled and/or the extra module is added.

@artembilan
Copy link
Member

Thank you for the request!

If you have anything in mind how that supposed to be implemented on the RetryTemplate, I'll be happy to review.

Although from our experience an unconditional instrumentation is full of errors: no one said that we would like to instrument every single RetryTemplate instance in our application.
This could be like a high-level API (setEnableMetrics()) and respective @Retriable attribute.

We do have some stuff in the org.springframework.retry.stats with a StatisticsListener implementation.
Might be used as an inspiration (or even reused).
Perhaps this also can be implemented as a RetryListener contract to have a Micrometer as an optional dependency.

@artembilan
Copy link
Member

Looks like this project has some idea how to implement it: https://github.com/findinpath/spring-retry-metrics.

I still think that this can be implemented as an injectable RetryListener.
The auto-configuration for this feature is out of this project scope.
This is just library with an API without too much opinion on the default options and classpath decisions.

@artembilan
Copy link
Member

I wonder if @Timed feature from Micrometer does that trick already alongside with the @Retryable: https://docs.micrometer.io/micrometer/reference/concepts/timers.html#_the_timed_annotation.
However I agree that it won't collect the number of attempts...

@szpak
Copy link
Contributor Author

szpak commented Jul 30, 2024

I didn't have anything particular in mind reporting that idea. I've just thought, it could be useful to that those statistics/metrics available and Micrometer is an natural continuation.

The separate - Micrometer aware - module with injectable listener (and probably switchable auto configuration) sounds like a good idea. When added as a dependency (and enable with auto configuration), it could "observe" all the retryable methods, unless annotated with some dedicated annotation (@NotObservlable or something). Or as you suggested, to have some other way to point which methods/beans should be "observed" via the API.

There is a "new" Micrometer observation API and maybe it could be leveraged (however, I haven't had a chance to play with it so far).

Btw, maybe @findinpath has any thoughts after the original implementation in his project (and encountered challenges), years ago.

@artembilan
Copy link
Member

I thought about an Observation API, but that then rejected that idea.
There is no reason to include such an internal (retry) logic of the service into a trace.
There is just enough to observer the whole method call and have it in the trace.
The metric on the other hand (I believe that is going to be Timer) is a good idea to track those retries on the method call.

No, I'm not going to accept a new module just for this feature.
Having micrometer-core as an optional dependency is OK.
And if we go a RetryListener implementation, then it is that opt-in feature you can decide where and how to add into your services.

I'm still struggling to understand how different calls (and retries, respectively) of the same method are going to be distinguished on the metrics consumer side, but probably that's just my low knowledge of the subject at all.

We can ask @marcingrzejszczak for advice.

@artembilan artembilan added this to the 2.0.8 milestone Jul 31, 2024
artembilan added a commit to artembilan/spring-retry that referenced this issue Jul 31, 2024
@artembilan
Copy link
Member

So, I decided to implement something myself: #459.
I'm not totally sure that it supposed to be like that in the Timer part, but at least we have something to discuss further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants