refactor(sdk): change SpanProcessor::on_end to take &mut FinishedSpan#3410
refactor(sdk): change SpanProcessor::on_end to take &mut FinishedSpan#3410bryantbiggs wants to merge 5 commits into
Conversation
7d25461 to
633ab55
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3410 +/- ##
=======================================
- Coverage 82.8% 82.8% -0.1%
=======================================
Files 130 130
Lines 27289 27595 +306
=======================================
+ Hits 22622 22865 +243
- Misses 4667 4730 +63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e14ea7d to
c03abe2
Compare
bantonsson
left a comment
There was a problem hiding this comment.
Thanks for moving this forward @bryantbiggs. Only some minor things.
c03abe2 to
4d36680
Compare
|
Can we get some maintainer 👀 on this? |
4d36680 to
5863cc6
Compare
|
Any of the maintainers have some time for this? |
0b095b1 to
d9b67f1
Compare
|
Thanks for the PR! Let's hold off on this one until after the current release ships — that way we can look at it together with the other pending tracing changes and batch all the breaking changes into the next release. Will pick it up right after — appreciate the patience! |
Refactors the SpanProcessor API to avoid unnecessary cloning of span data: 1. Introduces `ReadableSpan` trait with getters for span fields, allowing processors to inspect spans without cloning. 2. Introduces `FinishedSpan` wrapper passed to `on_end`. Processors that only read span data pay zero cost. Processors that need ownership call `consume()` — the last processor gets the data via move (zero-copy), earlier processors get a clone. 3. Changes `on_end(&self, span: SpanData)` to `on_end(&self, span: &mut FinishedSpan)`. This is a breaking change to the SpanProcessor trait. 4. Implements `ReadableSpan` for both `Span` (live spans) and `FinishedSpan`. Benchmarks show span lifecycle cost becomes constant regardless of processor count (previously scaled linearly due to cloning). Original work by paullegranddc in PR open-telemetry#2962. Supersedes open-telemetry#2962. Relates to open-telemetry#2940, open-telemetry#2726, open-telemetry#2939.
- Fix end_with_timestamp bug: use explicit end_time_set flag instead of sentinel comparison (start_time == end_time), which silently overwrote user-provided timestamps equal to start time - Restore mem::replace for span_context in end_and_export to avoid unnecessary clone of TraceState (span is dead after data.take()) - Rewrite end_and_export to take data before shutdown check, preventing potential data leak on re-entrant Drop - Add FinishedSpan::is_consumed() method and document post-consume behavior - Add instrumentation_scope() to ReadableSpan trait (OnceLock for MSRV 1.75) - Fix FinishedSpan::Debug to always show consumed=true when span data is gone - Add ReadableSpan trait doc explaining post-end behavior on Span - Use InMemorySpanExporter in test to verify end_time preservation - Normalize formatting in ReadableSpan impl for FinishedSpan - Remove redundant imports, fix doc typo, remove double blank line - Update benchmark results for Apple M4 Max
- Change `ReadableSpan::span_kind()` to return `&SpanKind` for consistency with `status() -> &Status` - Remove leftover debug `println!` in tests
d9b67f1 to
f2c4e7b
Compare
Summary
Supersedes #2962 (original work by @paullegranddc). Refactors
SpanProcessor::on_endfromfn on_end(&self, span: SpanData)tofn on_end(&self, span: &mut FinishedSpan), enabling zero-copy span inspection and ownership semantics for multi-processor chains.Breaking Change
SpanProcessor::on_endnow takes&mut FinishedSpaninstead ofSpanData.Key Changes
ReadableSpantrait: New trait implemented by bothSpan(live, foron_start) andFinishedSpan(ended, foron_end). Allows processors to inspect span fields (name(),attributes(),span_kind(),instrumentation_scope(), etc.) without cloning.FinishedSpantype: Wraps finished span data with consume semantics.consume()gives the last processor zero-copy ownership viatake(); earlier processors receive aclone().is_consumed()lets processors check post-consume state.end_time_setflag: Replaces the fragilestart_time == end_timesentinel comparison.end_with_timestamp()now correctly preserves user-provided timestamps even when equal to start time.span_context: Usesmem::replaceinstead ofclone()inend_and_exportto avoid heap allocation forTraceState.end_and_exportrewrite: Takes span data before shutdown check (fixes potential data leak on re-entrant Drop), sets end time via explicit flag.Migration Guide
Files Changed
opentelemetry-sdk/src/trace/span.rs—FinishedSpan,ReadableSpantrait,end_and_exportrewrite,end_time_setflagopentelemetry-sdk/src/trace/span_processor.rs— UpdatedSpanProcessortrait + built-in implsopentelemetry-sdk/src/trace/span_processor_with_async_runtime.rs— Async batch processor updatedopentelemetry-sdk/src/trace/tracer.rs—end_time_set: falsein span constructionexamples/tracing-http-propagator/src/server.rs— NewRouteConcurrencyCounterSpanProcessorexample demonstratingReadableSpanin bothon_startandon_endopentelemetry-sdk/benches/span_processor_api.rs— New benchmark (0/1/2/4 processors, ~191 ns uniform on M4 Max)stress/src/traces.rs— UpdatedNoOpSpanProcessorBenchmark Results (Apple M4 Max)
Uniform across processor counts — the new API adds negligible overhead.
Test Plan
test_readable_span_after_consume_returns_defaults,test_end_with_timestamp_equal_to_start_timetracing-http-propagator) and stress test compilert-tokio) compilesCloses #2726, #2939. Relates to #2940.