-
Notifications
You must be signed in to change notification settings - Fork 586
chore(otlp): Handle partial success #3206
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
chore(otlp): Handle partial success #3206
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3206 +/- ##
=======================================
- Coverage 80.8% 80.7% -0.2%
=======================================
Files 128 128
Lines 23090 23220 +130
=======================================
+ Hits 18676 18754 +78
- Misses 4414 4466 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
585fa89 to
91a6e1e
Compare
opentelemetry-otlp/tests/smoke.rs
Outdated
| assert_eq!("my-test-event", first_event.name); | ||
| } | ||
|
|
||
| // Test for partial success response handling |
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.
not sure if we are actually validating anything in this test about partial-success.
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.
@cijothomas
It's just to make sure things don't catch fire, but you are right, there is not much in the way of asserts - apart from it doesn't panic, and spans come out the end.
I changed the comment to reflect this, but if your feeling is it is overwrought it may well be and I can happily remove it. The use case here in general is straightforward enough - de-serialize and log partial success - that I don't think this adds much beyond the 'fail path' unit tests tbh.
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.
LGTM.
91a6e1e to
51cb999
Compare
Fixes #865, handling partial success for HTTP and tonic OTLP responses by logging the portion of the request that was rejected by the server and not retrying.
The relevant bit of the spec is here.
Changes
Straightforward enough! The testing is rather limited because all we're doing is parsing and logging responses; let's make sure we don't panic for invalid or empty data, as well as a minimal smoke test.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes