Update the spec with the new metrics for 3.0 #536

Azquelt · 2020-04-23T17:25:22Z

Includes spec and TCKs.

We can still make tweaks to the spec if required.

Fixes #401
Fixes #499

Azquelt · 2020-04-29T17:08:52Z

I've now added new TCKs, though I don't have an implementation to run the TCKs against yet.

Mostly I've just adapted the existing tests to use the new metrics, though the new retry metrics resulted in several new test cases.

Azquelt · 2020-05-04T17:01:30Z

I've now made a few fixes and got my implementation running against these tests.

Emily-Jiang · 2020-05-13T14:47:05Z

spec/src/main/asciidoc/metrics.asciidoc

+| Tags
+a| * `method` - the fully qualified method name
+ * `result` = `[valueReturned\|exceptionThrown]` - whether the invocation returned a value or threw an exception
+ * `fallbackUsed` = `[true\|false\|none]` - `true` if fallback was used, `false` if a fallback is configured but was not used, `none` if a fallback is not configured


Had a further thought on this:
fallbackUsed with the choice of true|false|none is odd. How about fallbackStatus - true|false|none

I reviewed the whole section and I think it is better to change none to notApplicable

fallbackUsed =[ true|false|notApplicable]

A fallback that is not applicable is a fallback that has been defined, but configured to not apply for particular situation. It's an interesting case that we don't cover, but it's distinct from fallback that is not defined (which is what none means now).

What we have now is, essentially: fallback=[applied|notApplied|notDefined].

We could expand that to fallback=[applied|notAppliedBecauseNotNeeded|notAppliedBecauseOfConfiguration|notDefined]. Whether we need to distinguish the two cases of why fallback wasn't applied, I don't know.

We already know whether it was not applied because it wasn't needed or was not applied because the exception didn't match the configuration by the value of result

We currently have

result = valueReturned, fallbackUsed = false - method returned successfully, fallback was not needed

result = valueReturned, fallbackUsed = true - method threw an exception, fallback was invoked and returned a value

result = exceptionThrown, fallbackUsed = false - method threw an exception, but fallback was not invoked because fallback configuration didn't include the thrown exception

result = exceptionThrown, fallbackUsed = true - method threw an exception, fallback was invoked and also threw an exception

Ah, good point, thanks!

I propose we stay with none. We use some excessively long names elsewhere, and I'm not really a big fan of that.

EDIT: alternatively, I propose we go with long names here as well, and use something like fallback=[applied|notApplied|notDefined] :-)

I like the long names you specified @Ladicek , as it is self explainary 👍 .

notEnabled - does not mean the annotation does not exist. It could not the exception does not enable the fallback.

I don't think we ever use the word "enabled" to talk about applyOn or skipOn, so I don't think it would be reasonable to think that it relates to that. We do use "enabled" to talk about enabling and disabling an annotation via config.

EDIT: alternatively, I propose we go with long names here as well, and use something like fallback=[applied|notApplied|notDefined] :-)

I still don't really like notSpecified or notDefined as it sounds to me too much like a value which should exist is missing, but I can live with it. fallback=notDefined is at least better than fallbackUsed=notDefined.

I can see how undefined could sound like "something is supposed to be here, but some programmer made a mistake somewhere and so you're seeing this placeholder" (especially for people who have seen JavaScript), but notDefined is pretty unique IMHO.

I've changed it to fallback=[applied|notApplied|notDefined]

spec/src/main/asciidoc/metrics.asciidoc

Emily-Jiang · 2020-05-15T10:27:59Z

spec/src/main/asciidoc/metrics.asciidoc

+ft.timeout.calls.total{method="com.example.MyClass.doWork", timedOut="true"}
+ft.timeout.calls.total{method="com.example.MyClass.doWork", timedOut="false"}
+ft.timeout.executionDuration{method="com.example.MyClass.doWork"}
+```

 Now imagine the `doWork()` method is called and the invocation goes like this:

 * On the first attempt, the invocation takes more than 1000ms and times out
 * On the second attempt, something goes wrong and the method throws an `IOException`
 * On the third attempt, the method returns successfully and the result of this attempt is returned to the user


Is this attempt from the Retry operation?

Yes, I'll see if I can reword this to make it clearer that it's doing retries.

I've updated this example to make it explicit that it's doing retries.

Emily-Jiang · 2020-05-15T10:29:13Z

tck/src/main/java/org/eclipse/microprofile/fault/tolerance/tck/metrics/FallbackMetricBean.java

@@ -1,6 +1,6 @@
 /*
 *******************************************************************************
- * Copyright (c) 2018 Contributors to the Eclipse Foundation
+ * Copyright (c) 2018-2020 Contributors to the Eclipse Foundation


Will do this under #545

Emily-Jiang · 2020-05-15T10:29:40Z

...rc/main/java/org/eclipse/microprofile/fault/tolerance/tck/metrics/FallbackMetricHandler.java

@@ -1,6 +1,6 @@
 /*
 *******************************************************************************
- * Copyright (c) 2018 Contributors to the Eclipse Foundation
+ * Copyright (c) 2018-2020 Contributors to the Eclipse Foundation


Emily-Jiang · 2020-05-15T10:30:14Z

tck/src/main/java/org/eclipse/microprofile/fault/tolerance/tck/metrics/MetricsDisabledTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018 Contributors to the Eclipse Foundation
+ * Copyright (c) 2018-2020 Contributors to the Eclipse Foundation


Emily-Jiang · 2020-05-15T10:33:51Z

Can you also add a test scenario when configuring both failOn and skipOn on CircuitBreaker to measure the success counter for an exception excluded by CircuitBreaker?

Azquelt · 2020-05-15T13:37:36Z

Can you also add a test scenario when configuring both failOn and skipOn on CircuitBreaker to measure the success counter for an exception excluded by CircuitBreaker?

Yes, good spot. We test with an exception which isn't in failOn but we don't test with an exception which is in skipOn.

Update: expanded test to include skipOn

Azquelt · 2020-05-15T16:56:17Z

I think I've resolved all the comments on this PR, apart from the fallbackUsed tag. I'll have a further think on that.

carlosdlr

LGTM

Ladicek · 2020-05-19T13:33:01Z

spec/src/main/asciidoc/metrics.asciidoc

-* `ft.com.example.MyClass.doWork.timeout.callsNotTimedOut.total`
+```
+ft.invocations.total{method="com.example.MyClass.doWork", result="valueReturned", fallbackUsed="none"}
+ft.invocations.total{method="com.example.MyClass.doWork", result="exceptionThrown", fallbackUsed="none"}


Old name fallbackUsed.

ah rats, thank you for spotting this, I completely forgot to update the example.

Ladicek · 2020-05-19T13:33:07Z

spec/src/main/asciidoc/metrics.asciidoc

-`ft.com.example.MyClass.doWork.invocations.failed = 0` +
-No exceptions were propagated back to the caller.
+```
+ft.invocations.total{method="com.example.MyClass.doWork", result="valueReturned", fallbackUsed="none"} = 1


Old name fallbackUsed.

Ladicek · 2020-05-19T13:37:24Z

Quickly skimmed through it and found a few occurences of the old fallbackUsed name, otherwise LGTM.

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>

Azquelt · 2020-05-20T08:34:19Z

Squashing all the review changes

Azquelt force-pushed the ft-30-metrics branch 2 times, most recently from 134b56c to 573fbd8 Compare April 29, 2020 17:05

Azquelt force-pushed the ft-30-metrics branch 2 times, most recently from 4b1babf to dad075b Compare May 4, 2020 16:19

Azquelt marked this pull request as ready for review May 4, 2020 17:01

Emily-Jiang reviewed May 13, 2020

View reviewed changes

Emily-Jiang reviewed May 15, 2020

View reviewed changes

spec/src/main/asciidoc/metrics.asciidoc Outdated Show resolved Hide resolved

Emily-Jiang reviewed May 15, 2020

View reviewed changes

Azquelt force-pushed the ft-30-metrics branch from 961c832 to 41ded1e Compare May 15, 2020 15:40

Emily-Jiang approved these changes May 19, 2020

View reviewed changes

carlosdlr self-requested a review May 19, 2020 13:07

carlosdlr approved these changes May 19, 2020

View reviewed changes

Ladicek reviewed May 19, 2020

View reviewed changes

Ladicek approved these changes May 19, 2020

View reviewed changes

Azquelt added 2 commits May 20, 2020 09:32

Update the spec with the new metrics for 3.0

69e17f9

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>

Add TCKs for updated metrics

7999177

Signed-off-by: Andrew Rouse <anrouse@uk.ibm.com>

Azquelt force-pushed the ft-30-metrics branch from aa2aeed to 7999177 Compare May 20, 2020 08:33

Azquelt merged commit b817646 into eclipse:master May 20, 2020

Azquelt mentioned this pull request Aug 25, 2020

Fault Tolerance 3.0 Feature Test Summary OpenLiberty/open-liberty#13335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the spec with the new metrics for 3.0 #536

Update the spec with the new metrics for 3.0 #536

Azquelt commented Apr 23, 2020 •

edited

Azquelt commented Apr 29, 2020

Azquelt commented May 4, 2020

Emily-Jiang May 13, 2020

Emily-Jiang May 15, 2020

Ladicek May 15, 2020

Azquelt May 15, 2020 •

edited

Ladicek May 18, 2020

Ladicek May 18, 2020 •

edited

Emily-Jiang May 18, 2020

Azquelt May 18, 2020

Ladicek May 19, 2020

Azquelt May 19, 2020

Emily-Jiang May 15, 2020

Azquelt May 15, 2020

Azquelt May 15, 2020

Emily-Jiang May 15, 2020

Azquelt May 15, 2020

Emily-Jiang May 15, 2020

Emily-Jiang May 15, 2020

Emily-Jiang commented May 15, 2020

Azquelt commented May 15, 2020 •

edited

Azquelt commented May 15, 2020

carlosdlr left a comment

Ladicek May 19, 2020

Azquelt May 19, 2020

Ladicek May 19, 2020

Ladicek commented May 19, 2020 •

edited

Azquelt commented May 20, 2020

Update the spec with the new metrics for 3.0 #536

Update the spec with the new metrics for 3.0 #536

Conversation

Azquelt commented Apr 23, 2020 • edited

Azquelt commented Apr 29, 2020

Azquelt commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azquelt May 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek May 18, 2020 • 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

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

Emily-Jiang commented May 15, 2020

Azquelt commented May 15, 2020 • edited

Azquelt commented May 15, 2020

carlosdlr left a 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

Ladicek commented May 19, 2020 • edited

Azquelt commented May 20, 2020

Azquelt commented Apr 23, 2020 •

edited

Azquelt May 15, 2020 •

edited

Ladicek May 18, 2020 •

edited

Azquelt commented May 15, 2020 •

edited

Ladicek commented May 19, 2020 •

edited