Return span context ref (#325)

Currently the `Span` trait in the API has its `span_context` method
return an owned `SpanContext`. In order to be more consistent with the
rest of the APIs, this patch updates the trait to instead return a
reference, and moves the `SpanContext` data outside of the mutex in the
SDK to implement the trait. This also allows #265 to be implemented
without having to lock.

Additionally the spec now states that the `on_start` method for
`SpanProcessor`s must be passed a span and the parent context so those
are updated as well.

opentelemetry/src/api/trace/context.rs

@@ -33,11 +33,11 @@ pub trait TraceContextExt {
     /// };
     ///
     /// // returns a reference to an empty span by default
-    /// assert_eq!(Context::current().span().span_context(), SpanContext::empty_context());
+    /// assert_eq!(Context::current().span().span_context(), &SpanContext::empty_context());
     ///
     /// sdktrace::TracerProvider::default().get_tracer("my-component", None).in_span("my-span", |cx| {
     ///     // Returns a reference to the current span if set
-    ///     assert_ne!(cx.span().span_context(), SpanContext::empty_context());
+    ///     assert_ne!(cx.span().span_context(), &SpanContext::empty_context());
     /// });
     /// ```
     fn span(&self) -> &dyn crate::trace::Span;

opentelemetry/src/api/trace/noop.rs

@@ -78,8 +78,8 @@ impl trace::Span for NoopSpan {
     }
 
     /// Returns an invalid `SpanContext`.
-    fn span_context(&self) -> trace::SpanContext {
-        self.span_context.clone()
+    fn span_context(&self) -> &trace::SpanContext {
+        &self.span_context
     }
 
     /// Returns false, signifying that this span is never recording.
@@ -131,7 +131,7 @@ impl trace::Tracer for NoopTracer {
 
     /// Starts a new `NoopSpan` in a given context.
     ///
-    /// If the context contains a valid span context, it is progagated.
+    /// If the context contains a valid span context, it is propagated.
     fn start_from_context(&self, name: &str, cx: &Context) -> Self::Span {
         let builder = self.span_builder(name);
         self.build_with_context(builder, cx)
@@ -144,13 +144,17 @@ impl trace::Tracer for NoopTracer {
 
     /// Builds a `NoopSpan` from a `SpanBuilder`.
     ///
-    /// If the span builder or context contains a valid span context, it is progagated.
+    /// If the span builder or context contains a valid span context, it is propagated.
     fn build_with_context(&self, mut builder: trace::SpanBuilder, cx: &Context) -> Self::Span {
         let parent_span_context = builder
             .parent_context
             .take()
-            .or_else(|| Some(cx.span().span_context()).filter(|cx| cx.is_valid()))
-            .or_else(|| cx.remote_span_context().cloned())
+            .or_else(|| {
+                Some(cx.span().span_context())
+                    .filter(|sc| sc.is_valid())
+                    .cloned()
+            })
+            .or_else(|| cx.remote_span_context().filter(|sc| sc.is_valid()).cloned())
             .filter(|cx| cx.is_valid());
         if let Some(span_context) = parent_span_context {
             trace::NoopSpan { span_context }

opentelemetry/src/api/trace/span.rs

@@ -87,7 +87,7 @@ pub trait Span: fmt::Debug + 'static + Send + Sync {
 
     /// Returns the `SpanContext` for the given `Span`. The returned value may be used even after
     /// the `Span is finished. The returned value MUST be the same for the entire `Span` lifetime.
-    fn span_context(&self) -> SpanContext;
+    fn span_context(&self) -> &SpanContext;
 
     /// Returns true if this `Span` is recording information like events with the `add_event`
     /// operation, attributes using `set_attributes`, status with `set_status`, etc.

opentelemetry/src/api/trace/span_processor.rs

@@ -35,11 +35,12 @@
 //! [`TracerProvider`]: ../provider/trait.TracerProvider.html
 
 use crate::exporter::trace::SpanData;
+use crate::{trace::Span, Context};
 
 /// `SpanProcessor`s allow finished spans to be processed.
 pub trait SpanProcessor: Send + Sync + std::fmt::Debug {
     /// `on_start` method is invoked when a `Span` is started.
-    fn on_start(&self, span: &SpanData);
+    fn on_start(&self, span: &dyn Span, cx: &Context);
     /// `on_end` method is invoked when a `Span` is ended.
     fn on_end(&self, span: SpanData);
     /// Shutdown is invoked when SDK shuts down. Use this call to cleanup any

opentelemetry/src/api/trace/tracer.rs

@@ -41,7 +41,7 @@ use std::time::SystemTime;
 ///
 /// let parent = tracer.start("foo");
 /// let child = tracer.span_builder("bar")
-///     .with_parent(parent.span_context())
+///     .with_parent(parent.span_context().clone())
 ///     .start(&tracer);
 ///
 /// // ...

opentelemetry/src/global/trace.rs

@@ -30,7 +30,7 @@ impl trace::Span for BoxedSpan {
     }
 
     /// Returns the `SpanContext` for the given `Span`.
-    fn span_context(&self) -> trace::SpanContext {
+    fn span_context(&self) -> &trace::SpanContext {
         self.0.span_context()
     }
 

opentelemetry/src/sdk/propagation/aws.rs

@@ -107,7 +107,7 @@ impl XrayPropagator {
 
 impl TextMapPropagator for XrayPropagator {
     fn inject_context(&self, cx: &Context, injector: &mut dyn Injector) {
-        let span_context: SpanContext = cx.span().span_context();
+        let span_context = cx.span().span_context();
         if span_context.is_valid() {
             let xray_trace_id: XrayTraceId = span_context.trace_id().into();
 

opentelemetry/src/sdk/trace/span.rs

@@ -8,51 +8,68 @@
 //! start time is set to the current time on span creation. After the `Span` is created, it
 //! is possible to change its name, set its `Attributes`, and add `Links` and `Events`.
 //! These cannot be changed after the `Span`'s end time has been set.
-use crate::trace::{Event, SpanContext, SpanId, StatusCode, TraceId, TraceState};
-use crate::{exporter::trace::SpanData, sdk, KeyValue};
+use crate::trace::{Event, SpanContext, SpanId, SpanKind, StatusCode};
+use crate::{api, exporter, sdk, KeyValue};
 use std::sync::{Arc, Mutex};
 use std::time::SystemTime;
 
 /// Single operation within a trace.
 #[derive(Clone, Debug)]
 pub struct Span {
-    id: SpanId,
     inner: Arc<SpanInner>,
 }
 
 /// Inner data, processed and exported on drop
 #[derive(Debug)]
 struct SpanInner {
+    span_context: SpanContext,
     data: Option<Mutex<Option<SpanData>>>,
     tracer: sdk::trace::Tracer,
 }
 
+#[derive(Clone, Debug, PartialEq)]
+pub(crate) struct SpanData {
+    /// Span parent id
+    pub(crate) parent_span_id: SpanId,
+    /// Span kind
+    pub(crate) span_kind: SpanKind,
+    /// Span name
+    pub(crate) name: String,
+    /// Span start time
+    pub(crate) start_time: SystemTime,
+    /// Span end time
+    pub(crate) end_time: SystemTime,
+    /// Span attributes
+    pub(crate) attributes: sdk::trace::EvictedHashMap,
+    /// Span Message events
+    pub(crate) message_events: sdk::trace::EvictedQueue<api::trace::Event>,
+    /// Span Links
+    pub(crate) links: sdk::trace::EvictedQueue<api::trace::Link>,
+    /// Span status code
+    pub(crate) status_code: StatusCode,
+    /// Span status message
+    pub(crate) status_message: String,
+    /// Resource contains attributes representing an entity that produced this span.
+    pub(crate) resource: Arc<sdk::Resource>,
+}
+
 impl Span {
-    pub(crate) fn new(id: SpanId, data: Option<SpanData>, tracer: sdk::trace::Tracer) -> Self {
+    pub(crate) fn new(
+        span_context: SpanContext,
+        data: Option<SpanData>,
+        tracer: sdk::trace::Tracer,
+    ) -> Self {
         Span {
-            id,
             inner: Arc::new(SpanInner {
+                span_context,
                 data: data.map(|data| Mutex::new(Some(data))),
                 tracer,
             }),
         }
     }
 
-    /// Operate on reference to span inner
+    /// Operate on a mutable reference to span data
     fn with_data<T, F>(&self, f: F) -> Option<T>
-    where
-        F: FnOnce(&SpanData) -> T,
-    {
-        self.inner.data.as_ref().and_then(|inner| {
-            inner
-                .lock()
-                .ok()
-                .and_then(|span_data| span_data.as_ref().map(f))
-        })
-    }
-
-    /// Operate on mutable reference to span inner
-    fn with_data_mut<T, F>(&self, f: F) -> Option<T>
     where
         F: FnOnce(&mut SpanData) -> T,
     {
@@ -77,24 +94,15 @@ impl crate::trace::Span for Span {
         timestamp: SystemTime,
         attributes: Vec<KeyValue>,
     ) {
-        self.with_data_mut(|data| {
+        self.with_data(|data| {
             data.message_events
                 .push_back(Event::new(name, timestamp, attributes))
         });
     }
 
     /// Returns the `SpanContext` for the given `Span`.
-    fn span_context(&self) -> SpanContext {
-        self.with_data(|data| data.span_context.clone())
-            .unwrap_or_else(|| {
-                SpanContext::new(
-                    TraceId::invalid(),
-                    SpanId::invalid(),
-                    0,
-                    false,
-                    TraceState::default(),
-                )
-            })
+    fn span_context(&self) -> &SpanContext {
+        &self.inner.span_context
     }
 
     /// Returns true if this `Span` is recording information like events with the `add_event`
@@ -109,30 +117,30 @@ impl crate::trace::Span for Span {
     /// attributes"](https://github.com/open-telemetry/opentelemetry-specification/tree/v0.5.0/specification/trace/semantic_conventions/README.md)
     /// that have prescribed semantic meanings.
     fn set_attribute(&self, attribute: KeyValue) {
-        self.with_data_mut(|data| {
+        self.with_data(|data| {
             data.attributes.insert(attribute);
         });
     }
 
     /// Sets the status of the `Span`. If used, this will override the default `Span`
     /// status, which is `Unset`.
     fn set_status(&self, code: StatusCode, message: String) {
-        self.with_data_mut(|data| {
+        self.with_data(|data| {
             data.status_code = code;
             data.status_message = message
         });
     }
 
     /// Updates the `Span`'s name.
     fn update_name(&self, new_name: String) {
-        self.with_data_mut(|data| {
+        self.with_data(|data| {
             data.name = new_name;
         });
     }
 
     /// Finishes the span with given timestamp.
     fn end_with_timestamp(&self, timestamp: SystemTime) {
-        self.with_data_mut(|data| {
+        self.with_data(|data| {
             data.end_time = timestamp;
         });
     }
@@ -161,7 +169,11 @@ impl Drop for SpanInner {
                         };
 
                         if let Some(span_data) = span_data {
-                            processor.on_end(span_data);
+                            processor.on_end(build_export_data(
+                                span_data,
+                                self.span_context.clone(),
+                                &self.tracer,
+                            ));
                         }
                     }
                 }
@@ -170,6 +182,28 @@ impl Drop for SpanInner {
     }
 }
 
+fn build_export_data(
+    data: SpanData,
+    span_context: SpanContext,
+    tracer: &sdk::trace::Tracer,
+) -> exporter::trace::SpanData {
+    exporter::trace::SpanData {
+        span_context,
+        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,
+        message_events: data.message_events,
+        links: data.links,
+        status_code: data.status_code,
+        status_message: data.status_message,
+        resource: data.resource,
+        instrumentation_lib: *tracer.instrumentation_library(),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -181,13 +215,6 @@ mod tests {
         let config = provider.config();
         let tracer = provider.get_tracer("opentelemetry", Some(env!("CARGO_PKG_VERSION")));
         let data = SpanData {
-            span_context: SpanContext::new(
-                TraceId::from_u128(0),
-                SpanId::from_u64(0),
-                api::trace::TRACE_FLAG_NOT_SAMPLED,
-                false,
-                TraceState::default(),
-            ),
             parent_span_id: SpanId::from_u64(0),
             span_kind: api::trace::SpanKind::Internal,
             name: "opentelemetry".to_string(),
@@ -199,27 +226,26 @@ mod tests {
             status_code: StatusCode::Unset,
             status_message: "".to_string(),
             resource: config.resource.clone(),
-            instrumentation_lib: *tracer.instrumentation_library(),
         };
         (tracer, data)
     }
 
     fn create_span() -> Span {
         let (tracer, data) = init();
-        Span::new(SpanId::from_u64(0), Some(data), tracer)
+        Span::new(SpanContext::empty_context(), Some(data), tracer)
     }
 
     #[test]
     fn create_span_without_data() {
         let (tracer, _) = init();
-        let span = Span::new(SpanId::from_u64(0), None, tracer);
+        let span = Span::new(SpanContext::empty_context(), None, tracer);
         span.with_data(|_data| panic!("there are data"));
     }
 
     #[test]
-    fn create_span_with_data() {
+    fn create_span_with_data_mut() {
         let (tracer, data) = init();
-        let span = Span::new(SpanId::from_u64(0), Some(data.clone()), tracer);
+        let span = Span::new(SpanContext::empty_context(), Some(data.clone()), tracer);
         span.with_data(|d| assert_eq!(*d, data));
     }
 

opentelemetry/src/sdk/trace/span_processor.rs

@@ -1,6 +1,7 @@
 use crate::{
     exporter::trace::{SpanData, SpanExporter},
-    trace::SpanProcessor,
+    trace::{Span, SpanProcessor},
+    Context,
 };
 use futures::{channel::mpsc, executor, future::BoxFuture, Future, FutureExt, Stream, StreamExt};
 use std::fmt;
@@ -57,7 +58,7 @@ impl SimpleSpanProcessor {
 }
 
 impl SpanProcessor for SimpleSpanProcessor {
-    fn on_start(&self, _span: &SpanData) {
+    fn on_start(&self, _span: &dyn Span, _cx: &Context) {
         // Ignored
     }
 
@@ -126,7 +127,7 @@ impl fmt::Debug for BatchSpanProcessor {
 }
 
 impl SpanProcessor for BatchSpanProcessor {
-    fn on_start(&self, _span: &SpanData) {
+    fn on_start(&self, _span: &dyn Span, _cx: &Context) {
         // Ignored
     }