-
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
OTLP HTTP Exporter: Propagate HTTP 429s #9905
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9905 +/- ##
=======================================
Coverage 91.79% 91.79%
=======================================
Files 358 359 +1
Lines 16576 16578 +2
=======================================
+ Hits 15216 15218 +2
Misses 1037 1037
Partials 323 323 ☔ 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.
Overall looks good to me.
NOTE! before merging this, I'm going to run some tests to be sure this does exactly what we expect! I'll report back |
…tor into fix-429-logic
…tor into fix-429-logic
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.
This looks good to me as an intermediary step towards #9041
c = codes.ResourceExhausted | ||
case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: | ||
c = codes.Unavailable | ||
default: |
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.
I would add 413 -> codes.FailedPrecondition, 408 -> codes.DeadlineExceeded etc. Not suggesting to do it in this PR though.
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.
yeah that should be done in a followup, I want this to be focused on the 429 bug
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
Description:
Changes otlphttp status code handling to propagate the error code as a grpc status code. This follows the logic that was implemented for the http receiver here.
Link to tracking Issue: #9892
Testing: local tests were updated, going to test a local build.
Documentation: none yet...