-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 traces export errors use a consistent error message prefix #3516
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3516 +/- ##
=======================================
+ Coverage 79.4% 79.7% +0.2%
=======================================
Files 170 171 +1
Lines 12654 12673 +19
=======================================
+ Hits 10056 10101 +45
+ Misses 2388 2359 -29
- Partials 210 213 +3
|
Please fix the lint issues and add a changelog entry. |
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 seems like a lot of code to add to ensure the two client error messages have a prefix. Why not just add the prefix to the client response?
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, I think adding information to the the error message is helpful, and worth the extra branch.
This point is well taken. This PR and #3515 can be done better and I will give it a try. My biggest concern among these three is actually #3514, which conceals the gRPC or HTTP error message when the final error was retryable. I'll close this and #3515 for now and reopen when it's in better shape.
There are cases where the retry mechanism returns a local error that would not be prefixed, that's what led me to this in the first place. When you get the full gRPC error message you're likely to have enough context to determine whether it's a tracing SDK problem or a metrics SDK problem. |
…o jmacd/traces_errmsg
…go into jmacd/traces_errmsg
I changed this PR to use a wrapped error and restored the old tests w/ a call to |
If the approach taken here is acceptable, I would restore #3515 with the same (using the new internal |
When a user configures both a tracing SDK and metrics SDK the, existing error-handling and logging mechanism do not indicate whether it is a tracing or a metrics failure. For SDKs and vendors that route metrics and traces to different services, it is important to indicate which of the two paths is experiencing problems.
Part of #3513.