feat(appender-tracing): add experimental span attributes enrichment #3282
Conversation
…s with span context
…y-rust into feature/include-span-attributes-in-logrecords
…://github.com/leghadjeu-christian/opentelemetry-rust into feature/include-span-attributes-in-logrecords
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3282 +/- ##
======================================
Coverage 80.5% 80.5%
======================================
Files 129 129
Lines 23286 23456 +170
======================================
+ Hits 18748 18894 +146
- Misses 4538 4562 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
leghadjeu-christian
changed the title
| self.logger.emit(log_record); | ||
| } | ||
| #[cfg(feature = "experimental_span_attributes")] | ||
| fn on_new_span( |
There was a problem hiding this comment.
do we need to do anything to remove this from the span extensions, when span is being exited? Or its auto removed by tracing itself?
| id: &tracing::span::Id, | ||
| ctx: tracing_subscriber::layer::Context<'_, S>, | ||
| ) { | ||
| let span = ctx.span(id).expect("Span not found; this is a bug"); |
There was a problem hiding this comment.
is it a bug in OTel side? or tracing ?
|
|
||
| #[test] | ||
| #[cfg(feature = "experimental_span_attributes")] | ||
| fn test_span_context_enrichment_enabled() { |
There was a problem hiding this comment.
can you add a test for nested spans? I am curious on how that works as well...
| if let Some(span) = ctx.event_span(event) { | ||
| let extensions = span.extensions(); | ||
| if let Some(stored) = extensions.get::<StoredSpanAttributes>() { | ||
| // Add span attributes first (event attributes will overwrite on conflict). TODO: Consider conflict resolution strategies. |
There was a problem hiding this comment.
Can you point me to where is the overwrite logic? OTel SDK itself does not do any overwrite...
| ); | ||
| } | ||
|
|
||
| fn record_i64(&mut self, field: &tracing::field::Field, value: i64) { |
There was a problem hiding this comment.
are the field names &'static str ? Can we leverage that and avoid String which forces a heap allocation for each attribute?
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry/src/common.rs#L16-L49
We'd avoid allocation if &'static str is used. Let's do it. Its already done in the event visitor.
| /// Visitor to extract fields from a tracing span | ||
| #[cfg(feature = "experimental_span_attributes")] | ||
| struct SpanFieldVisitor { | ||
| fields: HashMap<String, AnyValue>, |
There was a problem hiding this comment.
Instead of using String as the key, you should use OTel's Key.. that should avoid the heap allocations.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Thanks for working on this. I left comments. We need to do this with high performance, avoiding allocations wherever feasible.
Also, we need to have tests for nested spans.
please check comments.
2 participants
Fixes #3221
Design discussion issue (if applicable) #
Changes
Add experimental
experimental_span_attributesfeature to enrich log records with attributes from active tracing spans.When enabled, span attributes are automatically included in log records. Event attributes take precedence on conflicts.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes