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

New module that collects overall information #1625

Closed
mkopylec opened this issue Feb 4, 2022 · 24 comments
Closed

New module that collects overall information #1625

mkopylec opened this issue Feb 4, 2022 · 24 comments
Milestone

Comments

@mkopylec
Copy link
Contributor

mkopylec commented Feb 4, 2022

Resilience4j version: newest

Java version: doesn't matter

Hi.
What do you think about adding a new module, let's say resilience4j-status, which will collect overall metrics (maybe more things like logs or something) for each operation it decorates?
By overall metrics I mean:
counters:

resilience4j.status.operation-1.successes
resilience4j.status.operation-1.exceptions
resilience4j.status.operation-1.exceptions.java-lang-exception (exception class name)

timers:

resilience4j.status.operation-1.duration
@madhava-sridhar
Copy link

there's micrometer module that already exposes these metrics
https://resilience4j.readme.io/docs/micrometer, is this requirement quite different from what "resilience4j-micrometer" already does ?

@RobWin
Copy link
Member

RobWin commented Feb 10, 2022

Hi,
I think the difference is that the micrometer metrics are per circuitbreaker instance or retry instance.
He would like to have a metric per decorated "method", correct?

@mkopylec
Copy link
Contributor Author

Yes, exactly @RobWin

@mkopylec
Copy link
Contributor Author

Maybe the new decorator could be implemented inside resilience4j-micrometer module.

@madhava-sridhar
Copy link

thanks @RobWin and @mkopylec , this weekend i will give a try with building decorator for resilience4j-micrometer module.

@mkopylec
Copy link
Contributor Author

@madhava-sridhar Any update on this? :)

@mkopylec
Copy link
Contributor Author

@RobWin I've added a PR for that. Please check it out.

@RobWin
Copy link
Member

RobWin commented Jun 16, 2023

Hi,

I think I would not mix metrics and logging in a single decorator. For logging we can make use of event handlers.

I've implemented an Aspect in one of my projects which combined a several metrics (Meter and Timer) into a single annotation which could be attached to any public method or class. It measured the execution rates, success/error counts and execution time of methods.
The Aspect added the class name and method name dynamically as tags to the metrics so that I was able to filter in Grafana or Prometheus.
That way I was able to see which methods or classes are the slowest or the methods which are called the most often.

I think that could be a nice addition. Unfortunately I don't have access to the code anymore.

Would someone be interested to implement this as an aspect?

@mkopylec
Copy link
Contributor Author

mkopylec commented Jun 17, 2023

Hi, IMO it should be implemented as a raw API like any other decorators. It could be an aspect in spring boot module like there is for any other decorators. The reason is that it would be nice to have a possibility to combine the new module with others using resilience4j-all. Also aspects force you to have a public method that wraps the decorated function. You don't have this limitation by using in-code API. Also not everyone uses Spring. Setting up AOP without a framework that supports it can be difficult.
As for the module itself. The PR is just a draft. I can implement it (and name it) however you like.

For logging we can make use of event handlers.

Events are published while using decorators. What if someone would like to just use the logging feature, without any decorator?

I'm also struggled how to implement metrics. In the PR I did it analogically to other decoratos, but in other decorators metrics are just additional feature. In the introduced module metrics are the core feature, thus maybe they shouldn't relay on event handlers.

@mkopylec
Copy link
Contributor Author

Any feedback here? How would you like to implement these features?

@RobWin
Copy link
Member

RobWin commented Jun 30, 2023

Hi, I still like the idea, but I prefer to separate measuring metrics from logging.

I once introduced a Timer decorator for Dropwizard metrics, but never migrated it to Micrometer.

https://github.com/resilience4j/resilience4j/blob/master/resilience4j-metrics/src/main/java/io/github/resilience4j/metrics/Timer.java

Maybe you are interesting in creating something comparable for Micrometer.
We also once had an idea to implement a Tracing decorator. See: #27
A logging decorator which logs whenver a method is entered, successful or failed would also be nice. But I really see it as a separate decorator so that users can choose more fine granular what they need.

@mkopylec
Copy link
Contributor Author

Thanks for response.
Ok, I can create metrics feature based on your Timer decorator in the micrometer module.
As for the logging decorator, do you see it as separate module in the project, or should I embed it into one of the existing modules?

@RobWin
Copy link
Member

RobWin commented Jun 30, 2023

I think we could move it into a dedicated module, maybe something like resilience4j-sl4j-logging ?

@mkopylec
Copy link
Contributor Author

mkopylec commented Jul 4, 2023

I created a draft for metrics feature. Please check it out. If it is OK for you I'll add unit tests, spring support, kotlin support etc.

@mkopylec
Copy link
Contributor Author

mkopylec commented Jul 4, 2023

I think we could move it into a dedicated module, maybe something like resilience4j-sl4j-logging ?

Sounds good, will add the new module after finishing the metrics feature.

@mkopylec
Copy link
Contributor Author

mkopylec commented Jul 5, 2023

I'm wondering what order should be set for the Timer Spring aspect. Currently we have:
Retry ( CircuitBreaker ( RateLimiter ( TimeLimiter ( Bulkhead ( Function ) ) ) ) )
If we set it like this:
Retry ( CircuitBreaker ( RateLimiter ( TimeLimiter ( Bulkhead ( **Timer** ( Function ) ) ) ) ) )
there will be no metrics if any of the previous aspects fail.
If we set it like this:
**Timer** ( Retry ( CircuitBreaker ( RateLimiter ( TimeLimiter ( Bulkhead ( Function ) ) ) ) ) )
the rate metrics won't count retries, and the duration metric will sum up all retries durations.

What's your recommendation?

@RobWin
Copy link
Member

RobWin commented Jul 5, 2023

Good question.
Usually I would like to know if a call takes 5s, even if there is a 2s timeout configured.
Also with a ThreadPoolBulkhead. Do you want to know how long it takes until a thread takes over a task, or do you only want to know how long the task itself takes.
I guess it really depends on your usecase.
Most simple solution would be: Retry ( CircuitBreaker ( RateLimiter ( TimeLimiter ( Bulkhead ( **Timer** ( Function ) ) ) ) ) )

I think I would vote for this.

@mkopylec
Copy link
Contributor Author

mkopylec commented Jul 5, 2023

Yeah, I think you're right. I would also vote this option.

@mkopylec
Copy link
Contributor Author

I added unit tests and converted the draft to PR, as it is ready for review.
Please take a look.

Should I create a full support for Timer (like kotlin, reactive stuff, spring boot, etc.) before merging the PR or rather split the whole implementation into multiple PRs?

@RobWin
Copy link
Member

RobWin commented Jul 10, 2023

Hi, splitting into multiple PRs is fine

@mkopylec
Copy link
Contributor Author

Can we move forward with this?
I've already have prepared the reactive support for the Timer.

@RobWin
Copy link
Member

RobWin commented Jul 19, 2023

I can merge your MR. Could you just change onNoResult to onSuccess?

@mkopylec
Copy link
Contributor Author

Done.

@mkopylec
Copy link
Contributor Author

mkopylec commented Aug 25, 2023

The above is the last PR when it comes to Timer.
Next PRs will implement the SLF4J logging decorator.

@RobWin RobWin added this to the 2.1.1 milestone Dec 6, 2023
@RobWin RobWin modified the milestones: 2.1.1, 2.2.0 Dec 17, 2023
@RobWin RobWin closed this as completed Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants