fix: LoggingSpanExporter.flush() should preserve flush failures#8361
fix: LoggingSpanExporter.flush() should preserve flush failures#8361jack-berg merged 7 commits intoopen-telemetry:mainfrom
Conversation
…urns success LoggingSpanExporter.flush() and LoggingMetricExporter.flush() unconditionally return success even if underlying handlers fail. This masks errors from the caller. Fixes open-telemetry#8360
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8361 +/- ##
============================================
+ Coverage 90.82% 90.87% +0.05%
- Complexity 7927 7954 +27
============================================
Files 895 896 +1
Lines 23872 24010 +138
Branches 2378 2390 +12
============================================
+ Hits 21681 21819 +138
- Misses 1446 1453 +7
+ Partials 745 738 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@psx95 Thanks for the review. You were right that the production bug is only in |
ADITYA-CODE-SOURCE
changed the title
There was a problem hiding this comment.
I am not sure why this file required a change. Could you explain the reasoning behind making this change?
There was a problem hiding this comment.
You're right — that change was only a CI workaround for an unrelated configuration-cache failure, and it broadened the scope of this PR. I removed it in commit 49a8703 so this PR stays focused on the LoggingSpanExporter fix.
There was a problem hiding this comment.
Nit: This file still has a change - a line was removed. This file should not be in the diff for this PR.
There was a problem hiding this comment.
There was still a whitespace-only leftover in . I restored it to match exactly in commit 0e034ab, so it should no longer appear in the diff.\n\nCould you please take another look? I believe all requested changes are addressed now.
There was a problem hiding this comment.
There was still a whitespace-only leftover in integration-tests/graal-incubating/build.gradle.kts. I restored it to match main exactly in commit 0e034ab, so it should no longer appear in the diff.\n\nCould you please take another look? I believe all requested changes are addressed now.
There was a problem hiding this comment.
Now that there are two tests in this file relying on the behavior of logger handler, I think it will be better if this test is also updated to remove the logger handler towards the end of the test.
This way both the tests can run in isolation in terms of LoggingHandler behavior.
I think the same would apply for the other test file as well.
There was a problem hiding this comment.
Good point. I updated both success-path flush tests in commit 49a8703 to remove the temporary handler in a finally block so the logger state is isolated between tests.
|
@psx95 Thanks again for the review. I believe all the requested changes are addressed now the scope is narrowed to LoggingSpanExporter the unrelated graal-incubating change is removed and both flush tests clean up their handlers properly. All checks are passing. |
There was a problem hiding this comment.
Nit: This file still has a change - a line was removed. This file should not be in the diff for this PR.
|
@psx95 Thanks again for the review. The leftover whitespace-only change in is now removed, the PR diff is back to the three intended logging files, and all checks are passing.\n\nCould you please take another look when you have a chance? |
|
Thank you for your contribution @ADITYA-CODE-SOURCE! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
3 participants
Fixes a bug where LoggingSpanExporter.flush() unconditionally returned success, even if an underlying logger handler failed during flushing. This masked flush failures from the caller.
Changes:
Fixes #8360