Added mutable reference parameter for SpanData in on_end trait function and replaced where necessary

Puffy1215 · Puffy1215 · commit b7efe129bb3e · 2025-04-02T20:57:26.000+08:00
diff --git a/opentelemetry-sdk/benches/batch_span_processor.rs b/opentelemetry-sdk/benches/batch_span_processor.rs
@@ -61,8 +61,8 @@ fn criterion_benchmark(c: &mut Criterion) {
                             let span_processor = shared_span_processor.clone();
                             let spans = get_span_data();
                             handles.push(tokio::spawn(async move {
-                                for span in spans {
-                                    span_processor.on_end(span);
+                                for mut span in spans {
+                                    span_processor.on_end(&mut span);
                                     tokio::task::yield_now().await;
                                 }
                             }));
diff --git a/opentelemetry-sdk/src/trace/export.rs b/opentelemetry-sdk/src/trace/export.rs
@@ -73,30 +73,30 @@ pub trait SpanExporter: Send + Sync + Debug {
 /// `SpanData` contains all the information collected by a `Span` and can be used
 /// by exporters as a standard input.
 #[derive(Clone, Debug, PartialEq)]
-pub struct SpanData<'a> {
+pub struct SpanData {
     /// Exportable `SpanContext`
     pub span_context: SpanContext,
     /// Span parent id
     pub parent_span_id: SpanId,
     /// Span kind
-    pub span_kind: &'a SpanKind,
+    pub span_kind: SpanKind,
     /// Span name
-    pub name: &'a Cow<'static, str>,
+    pub name: Cow<'static, str>,
     /// Span start time
     pub start_time: SystemTime,
     /// Span end time
     pub end_time: SystemTime,
     /// Span attributes
-    pub attributes: &'a Vec<KeyValue>,
+    pub attributes: Vec<KeyValue>,
     /// The number of attributes that were above the configured limit, and thus
     /// dropped.
     pub dropped_attributes_count: u32,
     /// Span events
-    pub events: &'a crate::trace::SpanEvents,
+    pub events: crate::trace::SpanEvents,
     /// Span Links
-    pub links: &'a crate::trace::SpanLinks,
+    pub links: crate::trace::SpanLinks,
     /// Span status
-    pub status: &'a Status,
+    pub status: Status,
     /// Instrumentation scope that produced this span
     pub instrumentation_scope: InstrumentationScope,
 }
diff --git a/opentelemetry-sdk/src/trace/mod.rs b/opentelemetry-sdk/src/trace/mod.rs
@@ -136,7 +136,7 @@ mod tests {
             }
         }
 
-        fn on_end(&self, _span: SpanData) {}
+        fn on_end(&self, _span: &mut SpanData) {}
 
         fn force_flush(&self) -> crate::error::OTelSdkResult {
             Ok(())
diff --git a/opentelemetry-sdk/src/trace/provider.rs b/opentelemetry-sdk/src/trace/provider.rs
@@ -516,7 +516,7 @@ mod tests {
                 .fetch_add(1, Ordering::SeqCst);
         }
 
-        fn on_end(&self, _span: SpanData) {
+        fn on_end(&self, _span: &mut SpanData) {
             // ignore
         }
 
@@ -779,7 +779,7 @@ mod tests {
             // No operation needed for this processor
         }
 
-        fn on_end(&self, _span: SpanData) {
+        fn on_end(&self, _span: &mut SpanData) {
             // No operation needed for this processor
         }
 
diff --git a/opentelemetry-sdk/src/trace/span.rs b/opentelemetry-sdk/src/trace/span.rs
@@ -74,12 +74,12 @@ impl Span {
     /// Convert information in this span into `exporter::trace::SpanData`.
     /// This function copies all data from the current span, which will create a
     /// overhead.
-    pub fn exported_data<'a>(&'a mut self) -> Option<crate::trace::SpanData<'a>> {
+    pub fn exported_data(&self) -> Option<crate::trace::SpanData> {
         let (span_context, tracer) = (self.span_context.clone(), &self.tracer);
 
         self.data
-            .as_mut()
-            .map(|data| build_export_data(data, span_context, tracer))
+            .as_ref()
+            .map(|data| build_export_data(data.clone(), span_context, tracer))
     }
 }
 
@@ -199,8 +199,23 @@ impl opentelemetry::trace::Span for Span {
 impl Span {
     fn ensure_ended_and_exported(&mut self, timestamp: Option<SystemTime>) {
         // skip if data has already been exported
-        let data = &mut match self.data.take() {
-            Some(data) => data,
+        let mut data = match self.data.take() {
+            Some(data) => {
+                crate::trace::SpanData {
+                    span_context: self.span_context.clone(),
+                    parent_span_id: data.parent_span_id,
+                    span_kind: data.span_kind,
+                    name: data.name,
+                    start_time: data.start_time,
+                    end_time: data.end_time,
+                    attributes: data.attributes,
+                    dropped_attributes_count: data.dropped_attributes_count,
+                    events: data.events,
+                    links: data.links,
+                    status: data.status,
+                    instrumentation_scope: self.tracer.instrumentation_scope().clone(),
+                }
+            },
             None => return,
         };
 
@@ -220,11 +235,11 @@ impl Span {
         match provider.span_processors() {
             [] => {}
             [processor] => {
-                processor.on_end();
+                processor.on_end(&mut data)
             }
             processors => {
                 for processor in processors {
-                    processor.on_end();
+                    processor.on_end(&mut data);
                 }
             }
         }
@@ -238,23 +253,23 @@ impl Drop for Span {
     }
 }
 
-fn build_export_data<'a>(
-    data: &'a mut SpanData,
+fn build_export_data(
+    data: SpanData,
     span_context: SpanContext,
-    tracer: &'a crate::trace::SdkTracer,
-) -> crate::trace::SpanData<'a> {
+    tracer: &crate::trace::SdkTracer,
+) -> crate::trace::SpanData {
     crate::trace::SpanData {
         span_context,
         parent_span_id: data.parent_span_id,
-        span_kind: &data.span_kind,
-        name: &data.name,
+        span_kind: data.span_kind,
+        name: data.name,
         start_time: data.start_time,
         end_time: data.end_time,
-        attributes: &data.attributes,
+        attributes: data.attributes,
         dropped_attributes_count: data.dropped_attributes_count,
-        events: &data.events,
-        links: &data.links,
-        status: &data.status,
+        events: data.events,
+        links: data.links,
+        status: data.status,
         instrumentation_scope: tracer.instrumentation_scope().clone(),
     }
 }
@@ -713,7 +728,7 @@ mod tests {
         let res = provider.shutdown();
         println!("{:?}", res);
         assert!(res.is_ok());
-        let mut dropped_span = tracer.start("span_with_dropped_provider");
+        let dropped_span = tracer.start("span_with_dropped_provider");
         // return none if the provider has already been dropped
         assert!(dropped_span.exported_data().is_none());
     }
diff --git a/opentelemetry-sdk/src/trace/span_processor.rs b/opentelemetry-sdk/src/trace/span_processor.rs
@@ -82,8 +82,7 @@ pub trait SpanProcessor: Send + Sync + std::fmt::Debug {
     /// `on_end` is called after a `Span` is ended (i.e., the end timestamp is
     /// already set). This method is called synchronously within the `Span::end`
     /// API, therefore it should not block or throw an exception.
-    /// TODO - This method should take reference to `SpanData`
-    fn on_end(&self, span: SpanData);
+    fn on_end(&self, span: &mut SpanData);
     /// Force the spans lying in the cache to be exported.
     fn force_flush(&self) -> OTelSdkResult;
     /// Shuts down the processor. Called when SDK is shut down. This is an
@@ -129,7 +128,7 @@ impl<T: SpanExporter> SpanProcessor for SimpleSpanProcessor<T> {
         // Ignored
     }
 
-    fn on_end(&self, span: SpanData) {
+    fn on_end(&self, span: &mut SpanData) {
         if !span.span_context.is_sampled() {
             return;
         }
@@ -138,7 +137,7 @@ impl<T: SpanExporter> SpanProcessor for SimpleSpanProcessor<T> {
             .exporter
             .lock()
             .map_err(|_| OTelSdkError::InternalFailure("SimpleSpanProcessor mutex poison".into()))
-            .and_then(|exporter| futures_executor::block_on(exporter.export(vec![span])));
+            .and_then(|exporter| futures_executor::block_on(exporter.export(vec![span.clone()])));
 
         if let Err(err) = result {
             // TODO: check error type, and log `error` only if the error is user-actionable, else log `debug`
@@ -460,7 +459,7 @@ impl BatchSpanProcessor {
         E: SpanExporter + Send + Sync + 'static,
     {
         // Get upto `max_export_batch_size` amount of spans from the channel and push them to the span vec
-        while let Ok(span) = spans_receiver.try_recv() {
+        while let Ok(span)  = spans_receiver.try_recv() {
             spans.push(span);
             if spans.len() == config.max_export_batch_size {
                 break;
@@ -512,7 +511,7 @@ impl SpanProcessor for BatchSpanProcessor {
     }
 
     /// Handles span end.
-    fn on_end(&self, span: SpanData) {
+    fn on_end(&self, span: &mut SpanData) {
         if self.is_shutdown.load(Ordering::Relaxed) {
             // this is a warning, as the user is trying to emit after the processor has been shutdown
             otel_warn!(
@@ -521,7 +520,7 @@ impl SpanProcessor for BatchSpanProcessor {
             );
             return;
         }
-        let result = self.span_sender.try_send(span);
+        let result = self.span_sender.try_send(span.clone());
 
         if result.is_err() {
             // Increment dropped span count. The first time we have to drop a span,
@@ -875,8 +874,8 @@ mod tests {
     fn simple_span_processor_on_end_calls_export() {
         let exporter = InMemorySpanExporterBuilder::new().build();
         let processor = SimpleSpanProcessor::new(exporter.clone());
-        let span_data = new_test_export_span_data();
-        processor.on_end(span_data.clone());
+        let mut span_data = new_test_export_span_data();
+        processor.on_end(&mut span_data);
         assert_eq!(exporter.get_finished_spans().unwrap()[0], span_data);
         let _result = processor.shutdown();
     }
@@ -885,7 +884,7 @@ mod tests {
     fn simple_span_processor_on_end_skips_export_if_not_sampled() {
         let exporter = InMemorySpanExporterBuilder::new().build();
         let processor = SimpleSpanProcessor::new(exporter.clone());
-        let unsampled = SpanData {
+        let mut unsampled = SpanData {
             span_context: SpanContext::empty_context(),
             parent_span_id: SpanId::INVALID,
             span_kind: SpanKind::Internal,
@@ -899,16 +898,16 @@ mod tests {
             status: Status::Unset,
             instrumentation_scope: Default::default(),
         };
-        processor.on_end(unsampled);
+        processor.on_end(&mut unsampled);
         assert!(exporter.get_finished_spans().unwrap().is_empty());
     }
 
     #[test]
     fn simple_span_processor_shutdown_calls_shutdown() {
         let exporter = InMemorySpanExporterBuilder::new().build();
         let processor = SimpleSpanProcessor::new(exporter.clone());
-        let span_data = new_test_export_span_data();
-        processor.on_end(span_data.clone());
+        let mut span_data = new_test_export_span_data();
+        processor.on_end(&mut span_data);
         assert!(!exporter.get_finished_spans().unwrap().is_empty());
         let _result = processor.shutdown();
         // Assume shutdown is called by ensuring spans are empty in the exporter
@@ -1109,8 +1108,8 @@ mod tests {
             .build();
         let processor = BatchSpanProcessor::new(exporter, config);
 
-        let test_span = create_test_span("test_span");
-        processor.on_end(test_span.clone());
+        let mut test_span = create_test_span("test_span");
+        processor.on_end(&mut test_span);
 
         // Wait for flush interval to ensure the span is processed
         std::thread::sleep(Duration::from_secs(6));
@@ -1132,8 +1131,8 @@ mod tests {
         let processor = BatchSpanProcessor::new(exporter, config);
 
         // Create a test span and send it to the processor
-        let test_span = create_test_span("force_flush_span");
-        processor.on_end(test_span.clone());
+        let mut test_span = create_test_span("force_flush_span");
+        processor.on_end(&mut test_span);
 
         // Call force_flush to immediately export the spans
         let flush_result = processor.force_flush();
@@ -1161,8 +1160,8 @@ mod tests {
         let processor = BatchSpanProcessor::new(exporter, config);
 
         // Create a test span and send it to the processor
-        let test_span = create_test_span("shutdown_span");
-        processor.on_end(test_span.clone());
+        let mut test_span = create_test_span("shutdown_span");
+        processor.on_end(&mut test_span);
 
         // Call shutdown to flush and export all pending spans
         let shutdown_result = processor.shutdown();
@@ -1196,13 +1195,13 @@ mod tests {
         let processor = BatchSpanProcessor::new(exporter, config);
 
         // Create test spans and send them to the processor
-        let span1 = create_test_span("span1");
-        let span2 = create_test_span("span2");
-        let span3 = create_test_span("span3"); // This span should be dropped
+        let mut span1 = create_test_span("span1");
+        let mut span2 = create_test_span("span2");
+        let mut span3 = create_test_span("span3"); // This span should be dropped
 
-        processor.on_end(span1.clone());
-        processor.on_end(span2.clone());
-        processor.on_end(span3.clone()); // This span exceeds the queue size
+        processor.on_end(&mut span1);
+        processor.on_end(&mut span2);
+        processor.on_end(&mut span3); // This span exceeds the queue size
 
         // Wait for the scheduled delay to expire
         std::thread::sleep(Duration::from_secs(3));
@@ -1242,7 +1241,7 @@ mod tests {
             KeyValue::new("key1", "value1"),
             KeyValue::new("key2", "value2"),
         ];
-        processor.on_end(span_data.clone());
+        processor.on_end(&mut span_data);
 
         // Force flush to export the span
         let _ = processor.force_flush();
@@ -1272,8 +1271,8 @@ mod tests {
         processor.set_resource(&resource);
 
         // Create a span and send it to the processor
-        let test_span = create_test_span("resource_test");
-        processor.on_end(test_span.clone());
+        let mut test_span = create_test_span("resource_test");
+        processor.on_end(&mut test_span);
 
         // Force flush to ensure the span is exported
         let _ = processor.force_flush();
@@ -1307,8 +1306,8 @@ mod tests {
         let processor = BatchSpanProcessor::new(exporter, config);
 
         for _ in 0..4 {
-            let span = new_test_export_span_data();
-            processor.on_end(span);
+            let mut span = new_test_export_span_data();
+            processor.on_end(&mut span);
         }
 
         processor.force_flush().unwrap();
@@ -1330,8 +1329,8 @@ mod tests {
         let processor = BatchSpanProcessor::new(exporter, config);
 
         for _ in 0..4 {
-            let span = new_test_export_span_data();
-            processor.on_end(span);
+            let mut span = new_test_export_span_data();
+            processor.on_end(&mut span);
         }
 
         processor.force_flush().unwrap();
@@ -1357,8 +1356,8 @@ mod tests {
         for _ in 0..10 {
             let processor_clone = Arc::clone(&processor);
             let handle = tokio::spawn(async move {
-                let span = new_test_export_span_data();
-                processor_clone.on_end(span);
+                let mut span = new_test_export_span_data();
+                processor_clone.on_end(&mut span);
             });
             handles.push(handle);
         }
diff --git a/opentelemetry-sdk/src/trace/span_processor_with_async_runtime.rs b/opentelemetry-sdk/src/trace/span_processor_with_async_runtime.rs
diff --git a/stress/src/traces.rs b/stress/src/traces.rs

Original file line number	Diff line number	Diff line change
`@@ -136,7 +136,7 @@ mod tests {`
`136`	`136`	`}`
`137`	`137`	`}`
`138`	`138`
`139`		`- fn on_end(&self, _span: SpanData) {}`
	`139`	`+ fn on_end(&self, _span: &mut SpanData) {}`
`140`	`140`
`141`	`141`	`fn force_flush(&self) -> crate::error::OTelSdkResult {`
`142`	`142`	`Ok(())`
Original file line number	Diff line number	Diff line change
`@@ -516,7 +516,7 @@ mod tests {`
`516`	`516`	`.fetch_add(1, Ordering::SeqCst);`
`517`	`517`	`}`
`518`	`518`
`519`		`- fn on_end(&self, _span: SpanData) {`
	`519`	`+ fn on_end(&self, _span: &mut SpanData) {`
`520`	`520`	`// ignore`
`521`	`521`	`}`
`522`	`522`
`@@ -779,7 +779,7 @@ mod tests {`
`779`	`779`	`// No operation needed for this processor`
`780`	`780`	`}`
`781`	`781`
`782`		`- fn on_end(&self, _span: SpanData) {`
	`782`	`+ fn on_end(&self, _span: &mut SpanData) {`
`783`	`783`	`// No operation needed for this processor`
`784`	`784`	`}`
`785`	`785`