refactor: only return span instead of entire context for avoiding expensive cloning

Author: taisho6339

opentelemetry-sdk/CHANGELOG.md

@@ -5,8 +5,9 @@
 - Added `Resource::get_ref(&self, key: &Key) -> Option<&Value>` to allow retrieving a reference to a resource value without cloning.
 - **Breaking** Removed the following public hidden methods from the `SdkTracer` [#3227][3227]:
   - `id_generator`, `should_sample`
-- **Fix**: Fixed panic when `SpanProcessor::on_end` calls `Context::current()` ([#3262][3262]).
-  - Updated `SpanProcessor::on_end` documentation to clarify that `Context::current()` returns the parent context, not the span's context
+- **Fix**: Fixed panic when `SpanProcessor::on_end` calls `Context::current()` ([#2871][2871]).
+  - Updated `SpanProcessor::on_end` documentation to clarify that `Context::current()` should not be relied upon in `on_end`
+  - `Context::current()` returns whatever context happens to be active, which may be unrelated to the span being ended
   - Added best practice guidance: extract context information in `on_start` and store as span attributes
   - Documented that the panic fix in `opentelemetry` allows safe calls to `Context::current()` from `on_end`
 

opentelemetry-sdk/src/trace/span_processor.rs

@@ -86,9 +86,11 @@ pub trait SpanProcessor: Send + Sync + std::fmt::Debug {
     ///
     /// # Accessing Context
     ///
-    /// If you need to access context information (such as baggage) in `on_end`,
-    /// note that calling [`Context::current()`] will return the **parent context**,
-    /// not the span's context (which has already been removed from the context stack).
+    /// **Important**: Do not rely on [`Context::current()`] in `on_end`. When `on_end`
+    /// is called during span cleanup, `Context::current()` returns whatever context
+    /// happens to be active at that moment, which is typically unrelated to the span
+    /// being ended. Contexts can be activated in any order and are not necessarily
+    /// hierarchical.
     ///
     /// **Best Practice**: Extract any needed context information in [`on_start`]
     /// and store it as span attributes. This ensures the information is available

opentelemetry/CHANGELOG.md

@@ -8,9 +8,9 @@
   - `with_trace_id`, `with_span_id`, `with_end_time`, `with_status`, `with_sampling_result`
 - **Added** `#[must_use]` attribute to `opentelemetry::metrics::AsyncInstrumentBuilder` to add compile time warning when `.build()` is not called on observable instrument builders, preventing silent failures where callbacks are never registered and metrics are never reported.
 - **Fix**: Fixed panic when calling `Context::current()` from `Drop` implementations triggered by `ContextGuard` cleanup ([#2871][2871]).
-  - Modified `ContextGuard::drop` to extract the context outside of `borrow_mut()` scope before dropping it
+  - Modified `ContextGuard::drop` to extract only the span (not the entire context) outside of `borrow_mut()` scope before dropping it
   - This prevents "RefCell already mutably borrowed" panics when Span's `Drop` implementation calls `Context::current()` during context cleanup
-  - **Note**: `Context::current()` called from within a Drop triggered by context cleanup will return the parent context, not the context being dropped (documented behavior)
+  - **Important**: `Context::current()` called from within a Drop triggered by context cleanup returns whatever context happens to be current at that moment, which may be unrelated to the context being dropped
 
 [3227]: https://github.com/open-telemetry/opentelemetry-rust/pull/3227
 [2871]: https://github.com/open-telemetry/opentelemetry-rust/issues/2871

opentelemetry/src/context.rs

@@ -119,8 +119,12 @@ impl Context {
     ///
     /// When called from within a [`Drop`] implementation that is triggered by
     /// a [`ContextGuard`] being dropped (e.g., when a [`Span`] is dropped as part
-    /// of context cleanup), this function returns the **parent context**, not the
-    /// context being dropped.
+    /// of context cleanup), this function returns **whatever context happens to be
+    /// current after the guard is popped**, not the context being dropped.
+    ///
+    /// **Important**: The returned context may be completely unrelated to the
+    /// context being dropped, as contexts can be activated in any order and are
+    /// not necessarily hierarchical. Do not rely on any relationship between them.
     ///
     /// This behavior is by design and prevents panics that would otherwise occur
     /// from attempting to borrow the context while it's being mutably borrowed
@@ -478,11 +482,14 @@ impl Drop for ContextGuard {
     fn drop(&mut self) {
         let id = self.cx_pos;
         if id > ContextStack::BASE_POS && id < ContextStack::MAX_POS {
-            // Extract the context to drop outside of borrow_mut to avoid panic
-            // when the context's drop implementation (e.g., Span::drop) calls Context::current()
-            let _context_to_drop =
+            // Extract only the span to drop outside of borrow_mut to avoid panic
+            // when the span's drop implementation calls Context::current()
+            #[cfg(feature = "trace")]
+            let _to_drop =
                 CURRENT_CONTEXT.with(|context_stack| context_stack.borrow_mut().pop_id(id));
-            // Context is automatically dropped here, outside of borrow_mut scope
+            #[cfg(not(feature = "trace"))]
+            CURRENT_CONTEXT.with(|context_stack| context_stack.borrow_mut().pop_id(id));
+            // Span (if any) is automatically dropped here, outside of borrow_mut scope
         }
     }
 }
@@ -533,6 +540,12 @@ struct ContextStack {
     _marker: PhantomData<*const ()>,
 }
 
+// Type alias for what pop_id returns - only return the span when trace feature is enabled
+#[cfg(feature = "trace")]
+type PopIdReturn = Option<Arc<SynchronizedSpan>>;
+#[cfg(not(feature = "trace"))]
+type PopIdReturn = ();
+
 impl ContextStack {
     const BASE_POS: u16 = 0;
     const MAX_POS: u16 = u16::MAX;
@@ -561,7 +574,7 @@ impl ContextStack {
     }
 
     #[inline(always)]
-    fn pop_id(&mut self, pos: u16) -> Option<Context> {
+    fn pop_id(&mut self, pos: u16) -> PopIdReturn {
         if pos == ContextStack::BASE_POS || pos == ContextStack::MAX_POS {
             // The empty context is always at the bottom of the [`ContextStack`]
             // and cannot be popped, and the overflow position is invalid, so do
@@ -575,6 +588,7 @@ impl ContextStack {
                     "Attempted to pop the overflow position which is not allowed"
                 }
             );
+            #[cfg(feature = "trace")]
             return None;
         }
         let len: u16 = self.stack.len() as u16;
@@ -588,11 +602,19 @@ impl ContextStack {
             // empty context is always at the bottom of the stack if the
             // [`ContextStack`] is not empty.
             if let Some(Some(next_cx)) = self.stack.pop() {
-                // Return the old context to be dropped outside of borrow_mut
-                Some(std::mem::replace(&mut self.current_cx, next_cx))
-            } else {
-                None
+                // Extract and return only the span to avoid cloning the entire Context
+                #[cfg(feature = "trace")]
+                {
+                    let old_cx = std::mem::replace(&mut self.current_cx, next_cx);
+                    return old_cx.span;
+                }
+                #[cfg(not(feature = "trace"))]
+                {
+                    self.current_cx = next_cx;
+                }
             }
+            #[cfg(feature = "trace")]
+            return None;
         } else {
             // This is an out of order pop.
             if pos >= len {
@@ -603,10 +625,16 @@ impl ContextStack {
                     stack_length = len,
                     message = "Attempted to pop beyond the end of the context stack"
                 );
+                #[cfg(feature = "trace")]
                 return None;
             }
-            // Clear out the entry at the given id and return it
-            self.stack[pos as usize].take()
+            // Clear out the entry at the given id and extract its span
+            #[cfg(feature = "trace")]
+            return self.stack[pos as usize].take().and_then(|cx| cx.span);
+            #[cfg(not(feature = "trace"))]
+            {
+                self.stack[pos as usize] = None;
+            }
         }
     }