spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
336 stars 110 forks source link

Only configure metrics if Resilience4J does not #127

Closed ryanjbaxter closed 2 years ago

ryanjbaxter commented 2 years ago

Fixes #126

ryanjbaxter commented 2 years ago

@edwardsre I tested this and it seemed to work for me

codecov[bot] commented 2 years ago

Codecov Report

Merging #127 (a574db1) into 2.0.x (4b98327) will decrease coverage by 6.18%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.0.x     #127      +/-   ##
============================================
- Coverage     83.57%   77.39%   -6.19%     
  Complexity       78       78              
============================================
  Files            13       13              
  Lines           341      345       +4     
  Branches          8       10       +2     
============================================
- Hits            285      267      -18     
- Misses           50       74      +24     
+ Partials          6        4       -2     
Impacted Files Coverage Δ
...ience4j/ReactiveResilience4JAutoConfiguration.java 41.66% <0.00%> (-48.34%) :arrow_down:
...er/resilience4j/Resilience4JAutoConfiguration.java 37.03% <0.00%> (-58.97%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4b98327...a574db1. Read the comment docs.

edwardsre commented 2 years ago

@ryanjbaxter Resilience4j doesn't configure a TaggedCircuitBreakerMetrics by default, but rather TaggedCircuitBreakerMetricsPublisher, unless resilience4j.circuitbreaker.metrics.legacy.enabled is true, at least in the version of resilience4j in Spring Cloud 2021.0.0.

Wouldn't the spring auto configuration still create and configure TaggedCircuitBreakerMetrics based on the proposed changes?

See io.github.resilience4j.circuitbreaker.autoconfigure.CircuitBreakerMetricsAutoConfiguration

By the way, thank you for looking at this issue so quickly!

ryanjbaxter commented 2 years ago

It would only call the bindTo method in that case

edwardsre commented 2 years ago

@ryanjbaxter I have tried these changes with resilience4j.circuitbreaker.metrics.legacy.enabled=false and verified the metrics are only published once.

I also tried these changes with resilience4j.circuitbreaker.metrics.legacy.enabled=true, and found that bindTo gets called three times on the injected TaggedCircuitBreakerMetrics. Once by the MeterRegistryPostProcessor, and two other times by MicrometerReactiveResilience4JCustomizerConfiguration.init() and MicrometerResilience4JCustomizerConfiguration.init(). The metrics are still only published once, so that is good. I don't know if there are other ramifications to calling bindTo multiple times.

ryanjbaxter commented 2 years ago

I noticed this as well, but like you said it does not effect the count, and I noticed no other side effects. I am fine with merging it as is since it addressed the issue.