Enhancement/gather statistics #40

MALPI · 2017-09-27T06:56:51Z

Fixes #23

WIP
Extends Failsafe Actuator by the feature to provide failure/success metrics for the used Circuit Breakers.

Still open:

Add duration of a call
extend Readme.md.

coders-kitchen · 2017-09-27T20:23:59Z

src/main/java/org/zalando/failsafeactuator/domain/FailsafeBreaker.java

+    private final Meter overallMeter;
+    private String identifier;
+
+    public FailsafeBreaker(final MetricRegistry metricRegistry, final String identifier) {


I'm thinking if we should provide an abstraction layer around the metrics registry that would allow the users to plugin a different metrics collector, eg. prometheus.

For doing so we would need to introduce apart from the registry interface a Meter interface as well which would encapsulate the meter than.

As the default I would propose to provide a DropWizard Metrics implementation.

WDYT?

Spring supports PublicMetrics for this.

whiskeysierra · 2017-10-06T18:15:44Z

src/main/java/org/zalando/failsafeactuator/config/FailsafeAutoConfiguration.java

  public CircuitBreakerRegistry circuitBreakerRegistry() {
-    circuitBreakerRegistry = new CircuitBreakerRegistry();
+    circuitBreakerRegistry = new CircuitBreakerRegistry(this.metricRegistry);


You shouldn't assign to instance variables here, but rather take metricsRegistry as a parameter.

whiskeysierra · 2017-10-06T18:18:59Z

src/main/java/org/zalando/failsafeactuator/domain/FailsafeBreaker.java

+    }
+
+    @Override
+    public void recordResult(Object result) {


this should be done with event listeners rather than subclassing.

…cubator/failsafe-actuator into enhancement/gather-statistics

MALPI · 2017-10-11T19:16:14Z

Thanks for you input @whiskeysierra. Should all be considered in my last commits.

whiskeysierra · 2017-10-11T20:07:51Z

pom.xml

@@ -44,7 +46,12 @@
 			<scope>provided</scope>
            <version>1.16.18</version>
 		</dependency>
-        <dependency>
+		<dependency>


Why do you still need this?

Because Spring default metrics doesn't support Meter. But maybe you are right and we should rely on Spring defaults here.

The less dependencies the better. Gives your clients more freedom.

whiskeysierra · 2017-10-11T20:10:29Z

src/main/java/org/zalando/failsafeactuator/config/FailsafeAutoConfiguration.java

@@ -31,12 +34,24 @@
  private CircuitBreakerRegistry circuitBreakerRegistry;


Same here, shouldn't be needed to have this assigned to an instance field.

whiskeysierra · 2017-10-11T20:10:40Z

src/main/java/org/zalando/failsafeactuator/config/FailsafeAutoConfiguration.java

  public CircuitBreakerRegistry circuitBreakerRegistry() {
    circuitBreakerRegistry = new CircuitBreakerRegistry();
    return circuitBreakerRegistry;
  }

  @Bean
+  @DependsOn("metricRegistry")


Not needed.

whiskeysierra · 2017-10-11T20:10:53Z

src/main/java/org/zalando/failsafeactuator/config/FailsafeAutoConfiguration.java

+    return new DropwizardMetric(metricRegistry, circuitBreakerRegistry);
+  }
+
+  @Bean
  @DependsOn("circuitBreakerRegistry")


Replace with parameter.

whiskeysierra · 2017-10-11T20:12:09Z

src/main/java/org/zalando/failsafeactuator/metrics/DropwizardMetric.java

+      final Meter failureMeter = metricRegistry.meter(identifier + FAILURE);
+      metricMap.put(identifier, Arrays.asList(successMeter, failureMeter));
+      Failsafe.with(circuitBreakerRegistry.getConcurrentBreakerMap().get(identifier))
+          .with(new DropwizardListener(failureMeter, successMeter));


This probably won't work as expected.

whiskeysierra · 2017-10-11T20:13:42Z

src/main/java/org/zalando/failsafeactuator/config/FailsafeInjectionConfiguration.java

@@ -34,9 +32,9 @@

  @Bean
  @Scope(ConfigurableBeanFactory.SCOPE_PROTOTYPE)


Misleading because your registry actually implements a singleton pattern. Couldn't you delegate all of this work to spring?

Yes, we could do this. But I would prefer to do this change in another PR.

…cubator/failsafe-actuator into enhancement/gather-statistics

MALPI · 2017-10-24T12:46:06Z

@whiskeysierra @coders-kitchen can you have another look?

whiskeysierra · 2017-10-24T13:08:55Z

pom.xml


 	<modelVersion>4.0.0</modelVersion>

 	<groupId>org.zalando</groupId>
 	<artifactId>failsafe-actuator</artifactId>
-	<version>0.4.1</version>
+	<version>0.5.1</version>


Should a bump of the minor version set the patch back to 0, i.e. 0.5.0?

yes, will change that

whiskeysierra · 2017-10-24T13:09:49Z

src/main/java/org/zalando/failsafeactuator/aspect/Failsafe.java

@@ -1,29 +1,42 @@
 /**
- * The MIT License (MIT)
- * Copyright (c) 2016 Zalando SE
+ * The MIT License (MIT) Copyright (c) 2016 Zalando SE


I don't believe that you actually need to put the header in every file.

True, see #52

whiskeysierra · 2017-10-24T13:11:51Z

src/main/java/org/zalando/failsafeactuator/config/FailsafeAutoConfiguration.java

-  private CircuitBreakerRegistry circuitBreakerRegistry;
+  @Bean
+  @ConditionalOnMissingBean(MetricWriter.class)
+  public MetricWriter metricWriter() {


I wouldn't create those in here. The better approach would be to degrade and perform retries and manage circuit breakers without recording metrics.

👍 a noop implementation in this case makes more sense

whiskeysierra · 2017-10-24T13:13:40Z

src/main/java/org/zalando/failsafeactuator/aspect/FailsafeBreakerAspect.java

-                     });
-    } catch (RuntimeException e) {
+          .with(new CountingListener<>(counterService, breaker.value()))
+          .get(


This won't work for methods that return a Future.

We should file a new Issue for that since this logic wasn't introduced by this PR.

Hey @whiskeysierra now as I am looking into it, it's not really clear to me what you mean here. Could maybe explain a bit more in detail or extend the issue? Would be awesome!

There are two kinds of methods, sync and async. get is for synchronous methods. For something that returns a future, you should use future: https://github.com/jhalterman/failsafe#completablefuture-integration

Let's say you have a method that performs a remote request asynchronously and returns a Future<User>.

get would only retry the synchronous part of that, i.e. if anything failed during the creation of the future, but not during

future would be aware that the operation is asynchronous and it would correctly retry the asynchronous operation

whiskeysierra · 2017-10-24T13:13:59Z

src/main/java/org/zalando/failsafeactuator/aspect/FailsafeBreakerAspect.java

+                  try {
+                    return methodInvocation.proceed();
+                  } catch (final Throwable throwable) {
+                    throw new FailsafeBreakerWrappedException(throwable);


Why is this custom exception type needed?

@coders-kitchen should know. Depending on the outcome we should also tackle this on a separate issue as already mentioned before.

I prefer to have explicit exceptions in cases like this, to make it clear what the cause in case anything unexpected happen.

@coders-kitchen But you're deviating from the normal behaviour of failsafe. If a user would migrate from manually using failsafe to this annotation+advice, then he/she would experience a different behaviour. I believe it would be in the best interest of everybody if both ways are 100% compatible.

MALPI · 2017-10-30T06:45:02Z

@whiskeysierra @coders-kitchen I would consider this PR as done. Any further feedback?

coders-kitchen · 2017-11-06T21:39:18Z

@MALPI Fine from my side

MALPI · 2017-11-07T11:45:53Z

👍

Malte Pickhan and others added 3 commits September 6, 2017 21:36

Issue #23 Start implementation for gathering statistics.

6cc0af2

Add Metric Registry

7a6b464

Fix unit test, Autowire Metrics

63f519c

MALPI changed the base branch from master to release/0.5.0 September 27, 2017 06:57

Fix MetricRegistry autowiring

f874517

coders-kitchen reviewed Sep 27, 2017

View reviewed changes

MALPI mentioned this pull request Oct 5, 2017

Gather statistics #23

Closed

whiskeysierra reviewed Oct 6, 2017

View reviewed changes

Malte Pickhan added 2 commits October 6, 2017 20:59

Fix unit test, Fix imports

e78e271

Merge branch 'enhancement/gather-statistics' of github.com:zalando-in…

61359eb

…cubator/failsafe-actuator into enhancement/gather-statistics

zalando deleted a comment Oct 6, 2017

Drop unused imports

d3dda80

zalando deleted a comment Oct 6, 2017

Malte Pickhan added 6 commits October 6, 2017 21:05

Take metricregistry as argument

a0c00a9

Remove metric registry again

220b99a

Implement Springs PublicMetric Interface to gather statistics

57b0f7f

Merge release

4ed228e

Fix Autoconfiguration

93d8863

Fix identifier for metric reporting

b84925f

whiskeysierra reviewed Oct 11, 2017

View reviewed changes

Drop unnecessary dependsOn

6ca0515

MALPI and others added 7 commits October 17, 2017 21:20

Make registry an Argument

8a79b5e

Add creation of Bean

76684a5

Remove dropwizard and use spring defaults

a4e977b

Drop dependency to dropwizard and implement with spring defaults

8357a59

Merge branch 'enhancement/gather-statistics' of github.com:zalando-in…

ec218db

…cubator/failsafe-actuator into enhancement/gather-statistics

Drop merge mistake

082a9c4

Fix double counting of failures/success

3b6b530

zalando deleted a comment Oct 23, 2017

MALPI force-pushed the enhancement/gather-statistics branch from a058618 to 3b6b530 Compare October 23, 2017 19:01

Add some javadoc

520d326

zalando deleted a comment Oct 23, 2017

Create MetricWriter and CounterService in case they're missing.

f519f52

whiskeysierra reviewed Oct 24, 2017

View reviewed changes

Malte Pickhan added 2 commits October 29, 2017 20:12

Fix autoconfiguration in Tests

36959f7

Fix version

4e226c7

MALPI merged commit 7b606e0 into release/0.5.0 Nov 7, 2017

MALPI deleted the enhancement/gather-statistics branch December 19, 2017 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/gather statistics #40

Enhancement/gather statistics #40

MALPI commented Sep 27, 2017 •

edited

coders-kitchen Sep 27, 2017

whiskeysierra Oct 6, 2017

whiskeysierra Oct 6, 2017

whiskeysierra Oct 6, 2017

MALPI commented Oct 11, 2017

whiskeysierra Oct 11, 2017

MALPI Oct 12, 2017

whiskeysierra Oct 12, 2017

whiskeysierra Oct 11, 2017

whiskeysierra Oct 11, 2017

whiskeysierra Oct 11, 2017

whiskeysierra Oct 11, 2017

whiskeysierra Oct 11, 2017

MALPI Oct 17, 2017

MALPI Oct 17, 2017

MALPI commented Oct 24, 2017

whiskeysierra Oct 24, 2017

MALPI Oct 24, 2017

whiskeysierra Oct 24, 2017

MALPI Oct 24, 2017

whiskeysierra Oct 24, 2017

coders-kitchen Oct 24, 2017

whiskeysierra Oct 24, 2017

MALPI Oct 24, 2017

MALPI Oct 30, 2017

MALPI Nov 15, 2017

whiskeysierra Nov 15, 2017

whiskeysierra Oct 24, 2017

MALPI Oct 24, 2017

coders-kitchen Oct 24, 2017

whiskeysierra Nov 15, 2017

MALPI commented Oct 30, 2017

coders-kitchen commented Nov 6, 2017

MALPI commented Nov 7, 2017

		@@ -31,12 +34,24 @@
		private CircuitBreakerRegistry circuitBreakerRegistry;

		@@ -34,9 +32,9 @@

		@Bean
		@Scope(ConfigurableBeanFactory.SCOPE_PROTOTYPE)

Enhancement/gather statistics #40

Enhancement/gather statistics #40

Conversation

MALPI commented Sep 27, 2017 • 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

MALPI commented Oct 11, 2017

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

Choose a reason for hiding this comment

MALPI commented Oct 24, 2017

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

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

MALPI commented Oct 30, 2017

coders-kitchen commented Nov 6, 2017

MALPI commented Nov 7, 2017

MALPI commented Sep 27, 2017 •

edited