Annotation exception config (#662) #679

olovandersson · 2019-10-09T14:32:23Z

I've managed to find a solution that will work well even if annotations and circuit breakers created by code from the CircuitBreakerRegistry are mixed.

CircuitBreaker annotations now support both ignoreExceptions and recordExceptions. The spring boot config now pre-initializes the circuit breaker registry after the spring context has been scanned for annotations, which is merged with the properties configuration before initialization.

…#662)

Romeh · 2019-10-09T14:51:14Z

@olovandersson there is test failure in resilience4j-spring-boot:test FAILED , do u have the same locally ?

olovandersson · 2019-10-09T15:30:21Z

Yes, I'm more used to building with maven, so I didn't notice that no tests were run when I ran the publishToMavenLocal task. Anyway, I just pushed a fix.

RobWin · 2019-10-09T15:37:35Z

You can use gradlew check

RobWin · 2019-10-10T12:08:12Z

I review it when I'm back from vacation

Romeh · 2019-10-10T12:59:07Z

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerAnnotationConfigScanner.java

+	}
+
+	private void findAnnotationConfig(Class<?> beanClass) {
+		for (Method m : beanClass.getMethods()) {


here you assume the annotation will be present only only over methods , but it can be class level so you will miss the class level annotation in that case

I hadn't time to look at the PR yet. I just wonder how do we solve the case when there are multiple annotations for the same CircuitBreaker instance, but with different exceptions configured?

@Romeh Modified it to consider class-level annotations as well.

@RobWin The only reasonable behavior for conflicting annotation configurations would be to regard them as a programming error and throw an exception, so I implemented that. When it comes to external properties it is however consistent with spring in general to consider them as overrides.

For the multiple annotations case in the same class , i guess we need to merge the configured exceptions from multiple annotations for the same instance , as the user maybe would like to define common ignored exception over the class itself the he can define more specific exceptions to be ignored over the methods itself , example of what i mean :

@CircuitBreaker(name = DummyService.BACKEND,recordExceptions = {IOException.class}) @Component public class DummyServiceImpl { @CircuitBreaker(name = DummyService.BACKEND,recordExceptions = {BussinessExcp.class}) public void doSomething(boolean throwBackendTrouble){ } }

@RobWin what do u think ?

I think that is reasonable within a class, but I don't think it should be allowed between different classes, as that probably is more likely to be a programming error. Should we search the class hierarchy as well?

I think you can inject an ConfigurableEnvironment environment into the BeanFactoryPostProcessor

Interesting idea, I'll look into it.

@RobWin The PropertySource/ConfigurableEnvironment worked as expected, so I've pushed that change now. Also fixed so non-conflicting annotations (that declare the same exceptions for the same name) are allowed. Tested it in the resilience4j-spring-cloud2-demo project as well.

Question: It's a bit unclear to me why there are both CIrcuitBreakerConfigurationProperties.getConfigs() and CircuitBreakerConfigurationProperties.getInstances()?

The annotations only set the instances properties at this point.

I guess it's a hack and the Spring Boot guys will hate us for it ;)

getConfigs() returns a list of Configs was can be shared between instances.

For example:

resilience4j.circuitbreaker: configs: default: slidingWindowSize: 100 permittedNumberOfCallsInHalfOpenState: 10 waitDurationInOpenState: 10000 failureRateThreshold: 60 eventConsumerBufferSize: 10 registerHealthIndicator: true someShared: slidingWindowSize: 50 permittedNumberOfCallsInHalfOpenState: 10 instances: backendA: baseConfig: default waitDurationInOpenState: 5000 backendB: baseConfig: someShared

getInstances() returns the instance configurations which can also inherit from and overwrite shared configs

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerAnnotationConfigScanner.java

RobWin · 2019-10-14T07:26:21Z

@olovandersson Could you please check if the implementation also works when you use Spring Cloud Config and when you dynamically change a configuration property.

We do have a demo for Spring Cloud Config -> https://github.com/resilience4j/resilience4j-spring-cloud2-demo

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerAnnotationConfigScanner.java

Also fixed so non-conflicting annotations can appear multiple times.

dlsrb6342

Could you check indent config? tab and space are mix used.

...e4j/circuitbreaker/autoconfigure/CircuitBreakerAnnotationConfigScannerAutoConfiguration.java

dlsrb6342 · 2019-10-15T11:14:26Z

...va/io/github/resilience4j/bulkhead/autoconfigure/RefreshScopedBulkheadAutoConfiguration.java

@@ -11,7 +13,7 @@

 @Configuration
 @ConditionalOnClass({Bulkhead.class, RefreshScope.class})
-@AutoConfigureAfter(RefreshAutoConfiguration.class)
+@AutoConfigureAfter({CircuitBreakerAnnotationConfigScannerAutoConfiguration.class, RefreshAutoConfiguration.class})


This is BulkheadAutoConfiguration not for CircuitBreaker. Please remove CircuitBreakerAnnotationConfigScannerAutoConfiguration.

dlsrb6342 · 2019-10-15T11:15:56Z

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerAnnotationConfigScanner.java

+        this.configurableEnvironment = configurableEnvironment;
+	}
+
+	public void setOrder(int order) {


I think it is not needed. If we want to change order, it would be better to get from CircuitBreakerProperties

I think the Ordered inteface is needed so that this BeanFactoryPostProcessor is processed before others, or?

Yes it is needed. But I mean setOrder() is not needed. Like CircuitBreakerAspect, we can set order with properties.

resilience4j/resilience4j-spring/src/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfigurationProperties.java

Line 23 in c9a6fa2

private int circuitBreakerAspectOrder = Ordered.LOWEST_PRECEDENCE - 2;

A user should not be able to reconfigure it. Otherwise it doesn't work anymore.

yeah right. Then, we can change like below and remove setOrder() method.

@Override public int getOrder() { return LOWEST_PRECEDENCE; }

I added the setOrder to add flexibility to the library. It might not be necessary, but BeanFactoryPostProcessors can do all sorts of stuff to your application context , and we cannot anticipate how every single use-case might affect one another. Many exising BeanFactoryPostProcessors have this ability. This way you give library users a bit of flexibility for instance in case there is some other BeanFactoryPostProcessor that they would want to affect this one. There are several built-in factories in Spring that has this flexibility. I would guess that 99 out of 100 users won't bother with this. Note that the default order is lowest, so it will get processed last.

We can't set the order with CircuitBreakerConfigurationProperties because this factory is in fact used to help populate those properties.

We could skip the order though, if I remember correctly, it works correctly without it. People who want to customize could then choose to subclass instead.

Removed the Ordered interface as it wasn't really needed.

olovandersson · 2019-10-15T11:53:06Z

Could you check indent config? tab and space are mix used.

What's preferred in the project? I found existing source files with both tabs and spaces and mixed. So when I was trying to adapt, I didn't know what to choose. I usually use only spaces myself.

Removed Ordered interface from CircuitBreakerAnnotationConfigScanner, it isn't needed and can be added by subclassing if required. Removed circuitbreaker config that got added to RefreshScopedBulkheadAutoConfiguration by mistake. Fixed indentation to only use spaces in some classes.

olovandersson · 2019-10-15T12:14:39Z

Could you check indent config? tab and space are mix used.

What's preferred in the project? I found existing source files with both tabs and spaces and mixed. So when I was trying to adapt, I didn't know what to choose. I usually use only spaces myself.

Changed it to spaces now.

RobWin · 2019-10-15T14:30:39Z

A question from my side.
What happens now if the users configures a property in the config file and also in the annotation. Which value is used in the end?

olovandersson · 2019-10-15T14:57:32Z

A question from my side.
What happens now if the users configures a property in the config file and also in the annotation. Which value is used in the end?

It's like before, the config file takes precedence over the annotations. I must admit that I had not thoroughly looked into why but I've done that now and it's because the ConfigFileApplicationListener, which is an EnvironmentPostProcessor initializes them before the Spring has started to create the ApplicationContext. Spring-cloud do special tricks in the PropertySourceBootstrapConfiguration class to insert the remote configuration in the correct location for the local properties list. So it works correctly there as well.

olovandersson · 2019-10-15T15:04:30Z

@RobWin I've been thinking about one thing which isn't really an issue for this feature, but that's related. Currently the fallbackMethod gets invoked even for ignoreExceptions (which is sort of unexpected - this is not how the @HystrixCommand works, it won't call the fallback in this case). I would expect it to propagate instead. It is possible to re-throw it in a fallback method, but then it gets wrapped in an UndeclaredThrowableException.

I think the hystrix behaviour is more natural, but at the same time existing resilence4j users might have started to depend on it's current behavior.

As there probably are many users migrating from hystrix to resilience4j (especially since it's encouraged on the hystrix github page), it might be worth pointing out this difference in the documentation.

RobWin · 2019-10-15T15:31:27Z

Actually there are three type of exceptions.

Exceptions which are part of recordExceptions are counted as an error in the CircuitBreaker.
Exceptions which are part of ignoredExceptions are neither counted as an error nor failure.
Exceptions which are neither part of recordExceptions nor ignoreExceptions are counted as a success. This is a feature which is not supported by Hystrix.

A user can either define very fine granular which exceptions he wants to handle in fallback methods by defining very specific fallback method signatures. Or he can handle every exception in one fallback method. We thought it would be too confusing for the user to differiante between the three types.

RobWin · 2019-10-15T15:34:04Z

I thought we merged a PR which unwraps the cause from the InvocationException.

But I think I understand now. You are rethrowing a checked exception from the fallback method. We might have to invoke getUndeclaredThrowable in that case.

olovandersson · 2019-10-15T16:02:53Z

I thought we merged a PR which unwraps the cause from the InvocationException.

But I think I understand now. You are rethrowing a checked exception from the fallback method. We might have to invoke getUndeclaredThrowable in that case.

Yes, basically. Except I got an UndeclaredThrowableException even though my exception was extending RuntimeException. Which I thought was very odd. Anyway, I can create a separate issue for that one.

RobWin · 2019-10-15T16:18:04Z

We have to fix it in this class
https://github.com/resilience4j/resilience4j/blob/master/resilience4j-spring/src/main/java/io/github/resilience4j/fallback/FallbackMethod.java#L141

olovandersson · 2019-10-16T08:06:28Z

We have to fix it in this class
https://github.com/resilience4j/resilience4j/blob/master/resilience4j-spring/src/main/java/io/github/resilience4j/fallback/FallbackMethod.java#L141

Yeah, I understand now why I got UndeclaredThrowableException. It's the InvocationTargetException of the fallback method that causes this. Anyway it's really easy to write a junit test for and fix it (we just have to catch the InvocationTargetException, unwrap and re-throw it). I've already done it locally. Is it ok if I include that fix in this pull request?

RobWin · 2019-10-16T08:07:33Z

A separate bugfix-PR might be faster to merge.

RobWin · 2019-10-16T10:05:59Z

I wonder how we can solve this in Ratpack, since the annotations are used in Spring Boot and Ratpack.

olovandersson · 2019-10-16T11:25:14Z

I've been using Spring for more than a decade, but I haven't used Ratpack. Also I'll have a hard time justifying for my employer spending much time on it. I might be able to continue looking into this on my spare time, but it will be at a much lower pace.

Any help from anyone more familiar with Ratpack would be much appreciated.

Maybe some of the principles from the Spring solution can be re-used (like perhaps we could detect the annotations and merge the properties before the wiring starts)?

RobWin · 2019-10-16T11:25:47Z

@drmaas ?

RobWin · 2019-10-18T09:43:41Z

Hi,
I'm sorry to tell you that we made a vote where all core maintainers participated.
The result was that we would like to keep the Annotations only as a marker interface for our AOP Aspects. We all agreed, after talking to the Spring Boot guys, that Annotations shouldn't be used to extend the environment properties. Since the annotations are also used by other frameworks, we would need to find a solution for Ratpack and Micronaut as well.
Unfortunately the immutable and decorator design of resilience4j makes it difficult to implement such a feature properly.
But in order to support a compile-safe configuration of CircuitBreaker instances, we would like to add an alternative which is comparable to how Spring Cloud CircuitBreaker works. Please see their example
A user can define Customizer beans which allow to set CircuitBreakerConfig properties (besides the config file). This would allow also a mixture of configuration via code and config file.
The following is just a draft/suggestion:

Either:

@Bean
public CircuitBreakerConfigCustomizer customizer() {
        return (circuitBreakerName, builder) -> {
            if (circuitBreakerName.equals("bla")) {
                builder.recordExceptions(IOException.class);
                builder.ignoreExceptions(BusinessException.class);
            }
            return builder;
        };
}

or

@Bean
public CircuitBreakerConfigCustomizer blaCustomizer() {
        return builder -> {
            builder.recordExceptions(IOException.class);
            builder.ignoreExceptions(BusinessException.class);
            return new Tuple2<>("bla", builder);
        };
}

olovandersson · 2019-10-18T14:29:52Z

Ok, I can understand that. It would have been nice to get that feedback at an earlier stage though. But sometimes you have to dig into a problem to a problem to understand it's complexities.

I do believe that there are additional benefits of having the exceptions (or predicates) close to the breaker annotations besides compile-time safety. Much of the configuration is down to trimming in values, whereas the exceptions could be tightly coupled to your business logic. So it makes sense to have it close to the code.

But as I started thinking about ratpack issue, I also realized that it if the annotations should be able to do the things we talked about, they really need to be framework-specific and reside inside each framework module instead. Otherwise the development would always be held back by the maintenance of each framework (of which some could become unpopular after some time), as well as the abilities of the framework (since the framework provides the aspects for instance, not resilience4j).

The customization thing sounds like a really good idea. Because I could for instance develop my own annotations extend the current aspect and hook in the stuff I developed here as an add-on. But with the customization possibilities, it would be possible to do it in a much cleaner way, so I hope you proceed with that one.

RobWin · 2019-10-18T14:51:43Z

Yes, sorry that you have invested so much time. But as you said, without it we wouldn't be smarter now.
I agree. It might make sense to copy the annotations into the spring module.

olovandersson · 2019-10-18T15:43:42Z

I got an idea now. Spring has a pattern of extending annotations, the Service annotation for instance is annotated with Component. And SpringBootApplication is annotated with SpringBootConfiguration, EnableAutoConfiguration etc. I could imagine that a common use case for resilience4j is that you want a specific set of aspects at multiple locations (for instance, some combination of Bulkhead and CircuitBreaker).

What about adding support for such meta-annotations to the spring-module? They would then refer to the annotations in the existing resilience4j-annotations module. And maybe it would be possible to use that meta-annotation-support in combination with the Customizer functionality to enable custom configuration from such annotations?

And to clarify, the idea isn't that the module should provide the annotations itself, but provide the support for writing such annotations in your project and plug them in in an easy way.

olovandersson · 2019-10-18T15:55:19Z

I realize that #643 is about combining annotations. I do think it would make sense to consider whether you should put this in the generic annotation module because it will continue to lay a heavy burden on implementing the support in each supported framework.

Other stuff in the library is built on top of the core, whereas those annotations are rather useless without different framework implementations.

Olov Andersson (cbe836) added 5 commits October 8, 2019 15:56

Fixed various CircuitBreakerAnnotationConfigScanner init order issues (…

6377e6d

…#662)

Additional junit test and javadoc

a94361a

Fixed missing AutoConfigureAfter, code cleanup

69364fb

Javadoc clarification (#662)

d1ec69c

Romeh self-requested a review October 9, 2019 14:38

Fixed broken test

a414df5

Olov Andersson (cbe836) added 2 commits October 10, 2019 08:38

Added missing apache license texts

e91c27c

license text at wrong location

531b5c8

Romeh requested changes Oct 10, 2019

View reviewed changes

Class level config etc (#662)

80aabbb

olovandersson requested a review from Romeh October 11, 2019 08:29

RobWin mentioned this pull request Oct 14, 2019

Feature Request: @CircuitBreaker should support rich set of annotation attributes like threshold, sliding windowSize, waitDurationInOpenState #683

Closed

RobWin requested changes Oct 14, 2019

View reviewed changes

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerAnnotationConfigScanner.java Outdated Show resolved Hide resolved

Olov Andersson (cbe836) added 2 commits October 14, 2019 14:29

CircuitBreakerAnnotationConfigScanner checks class hierarchy (#662)

47626fc

CircuitBreakerAnnotationConfigScanner modifed to create a PropertySource

5269b46

Also fixed so non-conflicting annotations can appear multiple times.

dlsrb6342 requested changes Oct 15, 2019

View reviewed changes

olovandersson requested a review from dlsrb6342 October 15, 2019 12:13

olovandersson requested a review from RobWin October 15, 2019 12:17

CircuitBreakerAutoConfigurationTest verifies annotation config (#662)

8d0fe51

olovandersson closed this Oct 22, 2019

olovandersson deleted the annotation-exception-config branch October 22, 2019 06:46

dlsrb6342 mentioned this pull request Dec 13, 2019

Use named configuration with @RateLimiter annotation #773

Closed

Annotation exception config (#662) #679

Annotation exception config (#662) #679

Conversation

olovandersson commented Oct 9, 2019

Romeh commented Oct 9, 2019

olovandersson commented Oct 9, 2019

RobWin commented Oct 9, 2019 • edited

RobWin commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobWin Oct 15, 2019 • edited

Choose a reason for hiding this comment

RobWin commented Oct 14, 2019 • edited

dlsrb6342 left a comment

Choose a reason for hiding this comment

dlsrb6342 Oct 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlsrb6342 Oct 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olovandersson commented Oct 15, 2019

olovandersson commented Oct 15, 2019

RobWin commented Oct 15, 2019 • edited

olovandersson commented Oct 15, 2019

olovandersson commented Oct 15, 2019

RobWin commented Oct 15, 2019 • edited

RobWin commented Oct 15, 2019 • edited

olovandersson commented Oct 15, 2019

RobWin commented Oct 15, 2019

olovandersson commented Oct 16, 2019

RobWin commented Oct 16, 2019

RobWin commented Oct 16, 2019

olovandersson commented Oct 16, 2019

RobWin commented Oct 16, 2019

RobWin commented Oct 18, 2019 • edited

olovandersson commented Oct 18, 2019 • edited

RobWin commented Oct 18, 2019

olovandersson commented Oct 18, 2019

olovandersson commented Oct 18, 2019

RobWin commented Oct 9, 2019 •

edited

RobWin Oct 15, 2019 •

edited

RobWin commented Oct 14, 2019 •

edited

dlsrb6342 Oct 15, 2019 •

edited

dlsrb6342 Oct 15, 2019 •

edited

RobWin commented Oct 15, 2019 •

edited

RobWin commented Oct 15, 2019 •

edited

RobWin commented Oct 15, 2019 •

edited

RobWin commented Oct 18, 2019 •

edited

olovandersson commented Oct 18, 2019 •

edited