feat(appender-tracing): propagate span name to logs#3466
feat(appender-tracing): propagate span name to logs#3466SuperFluffy wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
changelog
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3466 +/- ##
=======================================
+ Coverage 83.2% 83.7% +0.4%
=======================================
Files 128 126 -2
Lines 25086 25502 +416
=======================================
+ Hits 20896 21370 +474
+ Misses 4190 4132 -58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
| if let Some(name) = current_span_name { | ||
| log_record.add_attribute(Key::new("span.name"), AnyValue::from(name)); |
There was a problem hiding this comment.
"span.name" - Its tricky! There is no established convention, so any name we pick can cause issues. Need to see if we can find any alternate solution.
Also, what happens when nesting of spans is there?
There was a problem hiding this comment.
Also, what happens when nesting of spans is there?
Related to your other comment for the test - this selects the inner-most span, so the span that immediately contains the event. I think that's the most natural span to select?
Alternatively we can follow tracing-subscriber's formatting subscriber which essentially contains a list of all spans.
"span.name"
I agree. None of this is great. Maybe just name?
There was a problem hiding this comment.
Given there is no way we can ensure conflict avoidance, could we avoid making that decision ourselves, and let user make that?
use_span_name("whatever name.user wants") ?
If user does not do the above, then we ignore the span-name. If the do it, then they gives us the attribute name.
WDYT?
There was a problem hiding this comment.
Yup, that simplifies this a lot. Added a OpenTelemetryTracingBridgeBuilder::with_span_name to toggle injecting spans into the attribute list.
| assert_eq!(logs.len(), 1); | ||
| let log = &logs[0]; | ||
|
|
||
| // The span.name should be the current (leaf/innermost) span |
There was a problem hiding this comment.
How do we determine what innermost is what user wants?
There was a problem hiding this comment.
Innermost is the most natural. Alternatively I can see collecting a list of all spans (probably not ideal).
There was a problem hiding this comment.
seems reasonable. Though I can see some users wanting ability to customize it. Probably we can start with this, and based on feedback, allow advanced capabilities.
There was a problem hiding this comment.
could you add this as a TODO
There was a problem hiding this comment.
Added this as an inline TODO in the loop under OpenTelemetryTracingBridge::on_event.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Agree we need to provide a way to attach span name, but need to also avoid picking an attribute name ourselves. See
#3466 (comment)
| /// (the default) disables it. | ||
| #[cfg(feature = "experimental_span_attributes")] | ||
| pub fn with_span_name(mut self, key: impl Into<Key>) -> Self { | ||
| self.span_name_key = Some(key.into()); |
There was a problem hiding this comment.
Note sure in how far this matters, but key here should be set once and be alive until the end of the program. Does it make sense to Box::leak::<'static> the key/string?
Did the changes. I guess this leaves open the question what to do if span and attribute names clash - but I don't think this crate should handle. Last time I checked the tracing crate itself just uses the last value it finds for a given key, so this is consistent. |
2 participants
Changes
Propagates the tracing span name to OTEL logs.
Similar to the experimental span attributes, the span containing the event is valuable to identify where and what happened.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes