-
Notifications
You must be signed in to change notification settings - Fork 453
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
Global error handler cleanup - Jaeger Propagator #2250
Global error handler cleanup - Jaeger Propagator #2250
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2250 +/- ##
=====================================
Coverage 79.5% 79.5%
=====================================
Files 121 121
Lines 20944 20950 +6
=====================================
+ Hits 16667 16672 +5
- Misses 4277 4278 +1 ☔ 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.
Use Warning/Info instead of Error severity.
https://github.com/open-telemetry/opentelemetry-rust/pull/2250/files#r1817682423
"invalid jaeger header format", | ||
"JaegerPropagator", | ||
))); | ||
otel_warn!( |
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 haven't followed the entire code path, so unclear what is the impact of this Warning. For eg: if an end users see this log entry, what should they be doing?
For all user facing logs, we should be very clear about the impact of this - will a span be dropped? will a span be using different parent ? will a span be created as root span? will we just ignore the header for tracestate and baggage too? or just ignore the traceparent header? something else?
I am not saying this should be fixed in this PR, as these are existing issues, nothing newly introduced in this PR. Since Metrics, Logs are where we are actively working towards Stable, we can probably merge this now, and come back when we start working towards Tracing Stable release?
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 do have some concerns about whether the logs are actionable/informative enough for end users - https://github.com/open-telemetry/opentelemetry-rust/pull/2250/files#r1817692142
But I don't think this should be a blocker now, as Tracing signal is unlikely to be in stable state soon. (For Metrics, Logs, we need to make sure every log is informative/actionable, as it is getting closer to stable release now)
Fixes #
Design discussion issue (if applicable) #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes