-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Treat PartialSuccess as Success #9260
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9260 +/- ##
=========================================
+ Coverage 0 90.47% +90.47%
=========================================
Files 0 344 +344
Lines 0 18042 +18042
=========================================
+ Hits 0 16323 +16323
- Misses 0 1390 +1390
- Partials 0 329 +329 ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers, this is ready for review. |
Test failure looks unrelated:
|
Description: Changes the treatment of PartialSuccess, making them successful and logging a warning instead of returning an error to the caller. These responses are meant to convey successful receipt of valid data which could not be accepted for other reasons, specifically to cover situations where the OpenTelemetry SDK and Collector have done nothing wrong, specifically to avoid retries. While the existing OTLP exporter returns a permanent error (also avoids retries), it makes the situation look like a total failure when in fact it is more nuanced.
As discussed in the tracking issue, it is a lot of work to propagate these "partial" successes backwards in a pipeline, so the appropriate simple way to handle these items is to return success.
In this PR, we log a warning. In a future PR, (IMO) as discussed in open-telemetry/oteps#238, we should count the spans/metrics/logs that are rejected in this way using a dedicated outcome label.
Link to tracking Issue:
Part of #9243
Testing: Tests for the "partial success" warning have been added.
Documentation: PartialSuccess behavior was not documented. Given the level of detail in the README, it feels appropriate to continue not documenting, otherwise lots of new details should be added.