search

spring-cloud / spring-cloud-circuitbreaker

Spring Cloud Circuit Breaker API and Implementations
Apache License 2.0
328 stars 109 forks source link

TimeLimiter instance properties configuration not working #141

Closed eduardolbueno closed 2 years ago

eduardolbueno commented 2 years ago

Describe the bug Properties configuration for instances of the resilience4j's TimeLimiter is not working.

This is an extension of #122. The above issue was partially fixed in the Cloud version 2020.0.5 - the TimeLimiter configuration is still not working.

If you run the code below you will get a timeout exception, whereas if you un-comment the default TimeLimiter configuration in application.yml you will get the result printed in the console.

Sample demo-tl.zip

ryanjbaxter commented 2 years ago

So it is just the specific time limiter configuration that is not working?

eduardolbueno commented 2 years ago

Correct

ryanjbaxter commented 2 years ago

The problem is that the circuit breaker id created by OpenFeign is DemoClient#getDemo() however # ( ) , are not allowed in property source names so what ends up being the property source name in your configuration properties is DemoClientgetDemo and of course we don't find that configuration name when we look for any kind of configuration for DemoClient#getDemo(). There are two possible solutions:

  1. We change the circuit breaker id created by OpenFeign so it does not have these characters
  2. In Spring Cloud CircuitBreaker we first check for the id thats passed and if no configuration is found we strip the characters from the name and check again.

The advantage to solution number 2 is that is less of a breaking change.

@spencergibb @OlgaMaciaszek what do you think?

As an interim solution would be define this bean and it should make things work.

    @Bean
    CircuitBreakerNameResolver circuitBreakerNameResolver() {
        return (feignClientName, target, method) -> Feign.configKey(target.type(), method).replace("#","").replace("(","").replace(")", "").replace(",","");
    }
eduardolbueno commented 2 years ago

Then this means that the circuit breaker configuration itself is not working as well. Because I have defined it as demo-client.

As a side question, is it the case that a circuit breaker instance is created for every feign operation (method)?

Why can't the curcuit breaker name be the same as the feign client name? demo-client in this case. Additionally, I would never guess that the CB instance name is ClassName#method() as this is not documented at all.

ryanjbaxter commented 2 years ago

Then this means that the circuit breaker configuration itself is not working as well. Because I have defined it as demo-client.

The feign client name has no effect on the CB name

As a side question, is it the case that a circuit breaker instance is created for every feign operation (method)?

Yes each operation gets its own CB

Why can't the curcuit breaker name be the same as the feign client name? demo-client in this case.

See above, because each method gets its own CB, the names must be unique

Additionally, I would never guess that the CB instance name is ClassName#method() as this is not documented at all.

You can open an issue in spring-cloud-openfeign for this

OlgaMaciaszek commented 2 years ago

@ryanjbaxter replacement would probably work fine for 3.1.x, however, as we're working on a major for 4.x now, we could consider changing the naming implementation in OF. wdyt? what naming convention would be the best for naming those circuits?

ryanjbaxter commented 2 years ago

Fixed in Spring Cloud OpenFeign https://github.com/spring-cloud/spring-cloud-openfeign/pull/687