Use failExceptionally in PeriodicMetricReader when exporter is busy#8525
Conversation
|
|
Fix indentation in PeriodicMetricReader.java
|
As it says next to the spotless violations:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8525 +/- ##
============================================
+ Coverage 90.93% 90.94% +0.01%
Complexity 10209 10209
============================================
Files 1013 1013
Lines 27175 27175
Branches 3184 3184
============================================
+ Hits 24712 24715 +3
+ Misses 1735 1733 -2
+ Partials 728 727 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Build is now passing. I noticed the patch coverage is at 50% — happy to add a test covering the forceFlush failure path if that would help get this merged. |
|
@vivekp14 Yes, please add/modify tests. Make sure to test the tests by breaking it to see the test fail. |
Add test for forceFlush behavior during in-flight export
| } else { | ||
| result.fail(); | ||
| result.failExceptionally( | ||
| new IllegalStateException("Exporter busy. Dropping metrics.")); |
There was a problem hiding this comment.
Meh I don't love the "Dropping metrics" because its its not true - metrics are actively being exported! I know you're just continuing the existing text used in the log, but now that its an exception that callers might key off of I want to improve the language. Maybe something like: "Export is already in progress, skipping flush". Would want to update the log line on 295 to match.
Also, extract this message to a constant so it can be used across this exception, the one line line 296, and the log line on 295.
Fixes #8433
When forceFlush() is called while a periodic export is already in
progress, PeriodicMetricReader was silently failing with plain fail().
Updated to use failExceptionally(IllegalStateException) so callers
can inspect the cause via CompletableResultCode#getFailureThrowable().
Also added JavaDoc to forceFlush() documenting this edge case.