-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[receiver/otlp] Switch from InvalidArgument to Internal #9415
[receiver/otlp] Switch from InvalidArgument to Internal #9415
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9415 +/- ##
=======================================
Coverage 90.24% 90.24%
=======================================
Files 344 344
Lines 17932 17933 +1
=======================================
+ Hits 16182 16183 +1
Misses 1421 1421
Partials 329 329 ☔ View full report in Codecov by Sentry. |
Could you explain a little more? I think I probably agree, but it would be good if we made it clear why we think this is accurate, especially because the spec recommends the 400-equivalent. My reasons for agreeing here would be:
|
@evan-bradley my reasoning for switching to My thinking was that If a consumer is messing up but we return The spec does say
I'll add that for the case of propagating errors back up the consumer chain, the otlpreceiver is only making these code assumptions for errors that are not already For the sake of argument, I'll provide a counter-argument to this change: it is possible there is a processor that expects specific data to be present in the payload and errors otherwise. IDT anything like that exists in contrib, but in that theoretical scenario an InvalidArgument would be more appropriate. In my opinion this would be handled by the component returning an appropriate |
I think that's a good point. For the cases where we can say this is an error with the data sent to the Collector, we can have the component with this context make that decision. Otherwise we can assume it's an issue originating within the Collector. |
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Description:
The otlp receiver was recently updated via #8080 to properly propagate consumer errors back to clients as either permanent or retriable. The code we're using to indicate a non-retriable error is
codes.InvalidArgument
, which is the equivalent of400
in HTTP.While 100% correct according to the OTLP specification to indicate a non-retriable error, I think
codes.Internal
(which is equivalent to HTTP500
), better conveys the actual state of the collector in these situations.Link to tracking Issue:
Related to #9357 (comment)
Testing:
Updated tests