-
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
Handle partial success responses in the OTLP exporter #6971
Handle partial success responses in the OTLP exporter #6971
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #6971 +/- ##
==========================================
+ Coverage 91.25% 91.27% +0.01%
==========================================
Files 296 296
Lines 14477 14498 +21
==========================================
+ Hits 13211 13233 +22
+ Misses 1002 1001 -1
Partials 264 264
☔ View full report in Codecov by Sentry. |
4a47822
to
0906ead
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
0906ead
to
749763f
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
749763f
to
60850ca
Compare
@bogdandrutu @jpkrohling could one of you please take a look? I will look into the decreased test coverage again today. Last time I checked, it wanted me to test what was essentially an impossible condition, but I will see if I can cover it. |
@bogdandrutu seems to have had concerns with the log statement. Perhaps @dmitryax or @codeboten could take a look as well, in case @bogdandrutu is not available? |
60850ca
to
a48075f
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
a48075f
to
d3a13da
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
d3a13da
to
476706c
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@evan-bradley do you think we need to merge this now or wait until we have a long-term plan as discussed in #7439 (comment)? |
@dmitryax I think we can merge this as-is, I don't expect any functional changes to how partial success responses are handled even after we change the errors. |
476706c
to
e27da35
Compare
@dmitryax Could you give this a look? I think it is ready to go, and would like to be able to use this. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Ok, maybe it'll change the messaging only |
I expect that to be the only thing that changes, since we will still be returning some form of permanent error. |
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 as a temporary solution until we have structured permanent errors
Description:
Handle partial success messages returned from OTLP gRPC backends.
Link to tracking Issue:
Fixes #6686
Testing:
Unit tests. I haven't been able to test this against any live backends, any recommendations would be welcome.