Remove metrics dependency from fault-tolerance extension #4987

loicmathieu · 2019-10-29T13:01:00Z

Remove the dependency between quarkus-smallrye-fault-tolerance and quarkus-smallrye-metics.

With this PR, the metrics extension will no longuer be included when the fault-tolerance extension is used.

The fault-tolerance extension now have a dependency to microprofile-fault-tolerance instead and provides a NOOP MetricsRegistry when the metrics extension is not present.

This fixes #4857

loicmathieu · 2019-10-29T13:07:31Z

@Ladicek @jmartisk can you review the implementation ?
I'm not proud of the NoopMetricsRegistry but I don't think there is a better way.

Also the way I switch off metrics for smallrye-fault-tolerance is via a startup event listening methods, I don't know if there is a better way to do this.

@gsmet I added the noteworthy label as with this PR quarkus-smallrye-metrics will no longuer be integrated when we add quarkus-smallrye-fault-tolerance. If some people rely of it they could be surprised.

gsmet · 2019-10-29T13:12:35Z

I'm cancelling CI for now, we need CI resources for other PRs. Sorry about that.

Please do not force push until tomorrow afternoon to avoid CI load.

Thanks!

bom/runtime/pom.xml

...ployment/src/main/java/io/quarkus/smallrye/faulttolerance/deployment/NoopMetricRegistry.java

gsmet · 2019-10-30T22:17:20Z

Let's not push this in 0.28.0. Please wait.

...eployment/src/main/java/io/quarkus/smallrye/metrics/deployment/SmallRyeMetricsProcessor.java

loicmathieu · 2019-11-12T09:58:41Z

@jmartisk raises concern that this implementation should fails on https://github.com/smallrye/smallrye-fault-tolerance/blob/2.1.2/implementation/src/main/java/io/smallrye/faulttolerance/metrics/BulkheadWaitRecorder.java#L55

As there is not a lot of tests inside Quarkus for fault-tolerence, I will add new ones so we will be able to decide if the implementation is OK or not.

Ladicek · 2019-11-12T10:14:36Z

I don't think it should fail, because that code can only be reached when metrics are enabled. See https://github.com/smallrye/smallrye-fault-tolerance/blob/2.1.2/implementation/src/main/java/io/smallrye/faulttolerance/HystrixCommandInterceptor.java#L202-L209

jmartisk · 2019-11-12T10:34:16Z

Hmm, perhaps this case will work, but we need to really check that SmallRye FT doesn't do anything that will break if we have the no-op registry trick in place, and fix any cases where it could break.. This trick is not a general purpose solution :(

Ladicek · 2019-11-12T11:09:47Z

I don't think it's a trick. I certainly don't want to introduce metrics abstraction in SmallRye FT just to make Metrics optional. MP Metrics is that abstraction. That said, it's probably true that the SmallRye FT code was written with the assumption that an implementation of MP Metrics is always present (and we need to do better with the reimplementation). I think I checked the SmallRye FT codebase when this issue was raised and I believe this approach should work well. I'll also gladly accept any PR in SmallRye FT that would help with this.

loicmathieu · 2019-11-12T11:36:56Z

@jmartisk @Ladicek I add tests for all the annotations of MP fault-tolerance inside quarkus-smallrye-fault-tolerance and all tests works.
As the extension tests uses the no-op impl, it means it works :)

So, is it OK for you guys to merge this ?

Meanwhile I saw that the fault tolerance guide was missing documentation on @Bulkhead and @Asynchronous. Is it worth opening an issue to covers them in the guide ?

jmartisk · 2019-11-12T12:34:58Z

I just gave this a round of manual experimentation and it worked well for me. So +1

loicmathieu · 2019-11-19T10:09:39Z

@jmartisk if all is OK for you can you approve it ?
@Ladicek is it OK for you also ot is there still some concern about it ?

I'll rebase the PR as there is some conflict

Fixes quarkusio#4857

Ladicek · 2019-11-19T11:05:14Z

...nce/deployment/src/test/java/io/quarkus/smallrye/faulttolerance/test/FaultToleranceTest.java

+
+    @Test
+    public void testBulkhead() {
+        ExecutorService executorService = Executors.newFixedThreadPool(10);


Perhaps shutdown() the executor service at the end of the test?

Actually the asserts here happen on different threads, so they don't really count (at least that's how JUnit 4 behaves; I don't know about JUnit 5, but I'd expect the same). Probably easiest would be something like this (writing from memory, some tweaks might be necessary, but the general idea should be clear):

AtomicInteger success = new AtomicInteger(); ExecutorService executorService = Executors.newFixedThreadPool(10); for (int i = 0; i < 20; i++) { executorService.submit(() -> { int bulk = bulkhead.bulkhead(); if (bulk <= 5) { success.incrementAndGet(); } }); } executorService.shutdown(); executorService.awaitTermination(5, TimeUnit.SECOND); assertEquals(20, success.get());

Assertions are done on the same thread (I check the code) so my impl is correct. I assert 20 times that the bulk is less or equals to 5 and it's pretty the same as you code but with an AtomicInteger to count. I prefere mine as it carry the intent better.

I added the closing of the executorService, thanks for pointing this to me :)

Well, the assertTrue call is in the lambda that you pass to executorService.submit -- unless I'm seriously mistaken, that means the assert will happen on the thread from the executor thread pool, not on the main thread on which the test runs.

@Ladicek you're right, the assert exception is thrown inside the task and ignored ...

But Bulkhead also throws an exception when too many parallel queries run so the test should assert that an exception was thrown or that bulkhead was never more than five.

Is the new test implementation OK ?

...src/test/java/io/quarkus/smallrye/faulttolerance/test/circuitbreaker/CircuitBreakerBean.java

Ladicek · 2019-11-19T11:12:36Z

Two little nitpicks in the tests, otherwise LGTM.

Ladicek · 2019-11-19T12:57:19Z

...nce/deployment/src/test/java/io/quarkus/smallrye/faulttolerance/test/FaultToleranceTest.java

+        executorService.shutdown();
+        executorService.awaitTermination(5, TimeUnit.SECONDS);
+        assertTrue(success.get());
+        assertTrue(bulkheadFailures.get());


Should this be assertFalse?

Otherwise it looks good.

No, this proove that Bulkhead works by checking that a BulkheadException has been thrown at least once.
I hope that this test will always succeed but as it's a concurency test it can be possible to have some times false negative, maybe I should add a comment on this ?

Ah ah, true, true, thanks!

loicmathieu · 2019-11-19T13:37:09Z

@gsmet ready to be merged and needed #5272, can we merge it ?

geoand · 2019-11-25T07:34:46Z

...main/java/io/quarkus/smallrye/faulttolerance/deployment/SmallRyeFaultToleranceProcessor.java

+        if (!capabilities.isCapabilityPresent(Capabilities.METRICS)) {
+            //disable fault tolerance metrics with the MP sys props and provides a No-op metric registry.
+            additionalBean.produce(new AdditionalBeanBuildItem(NoopMetricRegistry.class));
+            systemProperty.produce(new SystemPropertyBuildItem("MP_Fault_Tolerance_Metrics_Enabled", "false"));


Is this property necessary? I don't see it being used anywhere

This is a configuration option specified by MicroProfile Fault Tolerance to switch off metrics collection. SmallRye Fault Tolerance reads it.

Got it, thanks!

geoand

Looks good to me and more importantly it's been verified by the experts

loicmathieu force-pushed the fix/smallrye-metrics-depdendency branch from 97e827a to 5da955c Compare October 29, 2019 13:04

loicmathieu added the release/noteworthy-feature label Oct 29, 2019

gsmet added this to the 0.28.0 milestone Oct 29, 2019

Ladicek reviewed Oct 29, 2019

View reviewed changes

bom/runtime/pom.xml Outdated Show resolved Hide resolved

Ladicek reviewed Oct 29, 2019

View reviewed changes

...ployment/src/main/java/io/quarkus/smallrye/faulttolerance/deployment/NoopMetricRegistry.java Outdated Show resolved Hide resolved

loicmathieu force-pushed the fix/smallrye-metrics-depdendency branch from 5da955c to ab9c779 Compare October 30, 2019 14:08

loicmathieu changed the title ~~Remove metrics depdendency from fault-tolerance extension~~ Remove metrics dependency from fault-tolerance extension Oct 30, 2019

gsmet modified the milestones: 0.28.0, 1+ Oct 30, 2019

jmartisk approved these changes Oct 31, 2019

View reviewed changes

jmartisk requested changes Nov 7, 2019

View reviewed changes

...eployment/src/main/java/io/quarkus/smallrye/metrics/deployment/SmallRyeMetricsProcessor.java Outdated Show resolved Hide resolved

loicmathieu force-pushed the fix/smallrye-metrics-depdendency branch from ab9c779 to def4285 Compare November 7, 2019 11:00

loicmathieu changed the title ~~Remove metrics dependency from fault-tolerance extension~~ WIP - Remove metrics dependency from fault-tolerance extension Nov 12, 2019

loicmathieu changed the title ~~WIP - Remove metrics dependency from fault-tolerance extension~~ Remove metrics dependency from fault-tolerance extension Nov 12, 2019

feat: make fault tolerance not dependents of metrics

453924c

Fixes quarkusio#4857

loicmathieu force-pushed the fix/smallrye-metrics-depdendency branch from 7a58054 to 4f3989a Compare November 19, 2019 10:17

Ladicek reviewed Nov 19, 2019

View reviewed changes

...src/test/java/io/quarkus/smallrye/faulttolerance/test/circuitbreaker/CircuitBreakerBean.java Outdated Show resolved Hide resolved

loicmathieu force-pushed the fix/smallrye-metrics-depdendency branch from 4f3989a to 039d894 Compare November 19, 2019 11:26

jmartisk approved these changes Nov 19, 2019

View reviewed changes

jmartisk mentioned this pull request Nov 19, 2019

Unified way for extensions to register metrics #5272

Merged

Add more test to smallrye-faultolerance

7d327b6

loicmathieu force-pushed the fix/smallrye-metrics-depdendency branch from 039d894 to 7d327b6 Compare November 19, 2019 11:57

Ladicek reviewed Nov 19, 2019

View reviewed changes

Ladicek approved these changes Nov 19, 2019

View reviewed changes

loicmathieu added this to the 1.1.0 milestone Nov 19, 2019

geoand reviewed Nov 25, 2019

View reviewed changes

geoand approved these changes Nov 25, 2019

View reviewed changes

geoand merged commit 3892023 into quarkusio:master Nov 25, 2019

gsmet mentioned this pull request Nov 25, 2019

MicroProfile Health quickstart is failing #5748

Closed

loicmathieu deleted the fix/smallrye-metrics-depdendency branch November 26, 2019 09:26

Remove metrics dependency from fault-tolerance extension #4987

Remove metrics dependency from fault-tolerance extension #4987

Uh oh!

Conversation

loicmathieu commented Oct 29, 2019

Uh oh!

loicmathieu commented Oct 29, 2019

Uh oh!

gsmet commented Oct 29, 2019

Uh oh!

Uh oh!

Uh oh!

gsmet commented Oct 30, 2019

Uh oh!

Uh oh!

loicmathieu commented Nov 12, 2019

Uh oh!

Ladicek commented Nov 12, 2019

Uh oh!

jmartisk commented Nov 12, 2019

Uh oh!

Ladicek commented Nov 12, 2019

Uh oh!

loicmathieu commented Nov 12, 2019

Uh oh!

jmartisk commented Nov 12, 2019

Uh oh!

loicmathieu commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ladicek Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ladicek commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loicmathieu commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoand left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ladicek Nov 19, 2019 •

edited

Loading