Set status tag in Zipkin & Jaeger (#383)

Author: TommyCpp

opentelemetry-jaeger/src/exporter/mod.rs

@@ -623,11 +623,6 @@ fn build_span_tags(
         }
     }
 
-    // Ensure error status is set
-    if status_code == StatusCode::Error && !user_overrides.error {
-        tags.push(Key::new(ERROR).bool(true).into())
-    }
-
     if !user_overrides.span_kind {
         tags.push(Key::new(SPAN_KIND).string(kind.to_string()).into());
     }
@@ -636,8 +631,31 @@ fn build_span_tags(
         tags.push(KeyValue::new(STATUS_CODE, status_code as i64).into());
     }
 
-    if !user_overrides.status_message {
-        tags.push(Key::new(STATUS_MESSAGE).string(status_message).into());
+    if status_code != StatusCode::Unset {
+        // Ensure error status is set
+        if status_code == StatusCode::Error {
+            tags.push(Key::new(ERROR).bool(true).into());
+        }
+        tags.push(
+            Key::new(OTEL_STATUS_CODE)
+                .string::<&'static str>(status_code.as_str())
+                .into(),
+        );
+        // set status message if there is one
+        if !status_message.is_empty() {
+            if !user_overrides.status_message {
+                tags.push(
+                    Key::new(STATUS_MESSAGE)
+                        .string(status_message.clone())
+                        .into(),
+                );
+            }
+            tags.push(
+                Key::new(OTEL_STATUS_DESCRIPTION)
+                    .string(status_message)
+                    .into(),
+            );
+        }
     }
 
     Some(tags)
@@ -647,6 +665,8 @@ const ERROR: &str = "error";
 const SPAN_KIND: &str = "span.kind";
 const STATUS_CODE: &str = "status.code";
 const STATUS_MESSAGE: &str = "status.message";
+const OTEL_STATUS_CODE: &str = "otel.status_code";
+const OTEL_STATUS_DESCRIPTION: &str = "otel.status_description";
 
 #[derive(Default)]
 struct UserOverrides {
@@ -708,7 +728,7 @@ impl ExportError for Error {
 
 #[cfg(test)]
 #[cfg(feature = "collector_client")]
-mod tests {
+mod collector_client_tests {
     use crate::exporter::thrift::jaeger::Batch;
     use crate::new_pipeline;
     use opentelemetry::trace::TraceError;
@@ -755,3 +775,83 @@ mod tests {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::exporter::thrift::jaeger::Tag;
+    use crate::exporter::{build_span_tags, OTEL_STATUS_CODE, OTEL_STATUS_DESCRIPTION};
+    use opentelemetry::sdk::trace::EvictedHashMap;
+    use opentelemetry::trace::{SpanKind, StatusCode};
+
+    fn assert_tag_contains(tags: Vec<Tag>, key: &'static str, expect_val: &'static str) {
+        assert_eq!(
+            tags.into_iter()
+                .filter(|tag| tag.key.as_str() == key
+                    && tag.v_str.as_deref().unwrap_or("") == expect_val)
+                .count(),
+            1,
+            "Expect a tag {} with value {} but found nothing",
+            key,
+            expect_val
+        );
+    }
+
+    fn assert_tag_not_contains(tags: Vec<Tag>, key: &'static str) {
+        assert_eq!(
+            tags.into_iter()
+                .filter(|tag| tag.key.as_str() == key)
+                .count(),
+            0,
+            "Not expect tag {}, but found something",
+            key
+        );
+    }
+
+    fn get_error_tag_test_data() -> Vec<(
+        StatusCode,
+        String,
+        Option<&'static str>,
+        Option<&'static str>,
+    )> {
+        // StatusCode, error message, OTEL_STATUS_CODE tag value, OTEL_STATUS_DESCRIPTION tag value
+        vec![
+            (StatusCode::Error, "".into(), Some("ERROR"), None),
+            (StatusCode::Unset, "".into(), None, None),
+            // When status is ok, no description should be in span data. This should be ensured by Otel API
+            (StatusCode::Ok, "".into(), Some("OK"), None),
+            (
+                StatusCode::Error,
+                "have message".into(),
+                Some("ERROR"),
+                Some("have message"),
+            ),
+            (StatusCode::Unset, "have message".into(), None, None),
+        ]
+    }
+
+    #[test]
+    fn test_set_status() -> Result<(), Box<dyn std::error::Error>> {
+        for (status_code, error_msg, status_tag_val, msg_tag_val) in get_error_tag_test_data() {
+            let tags = build_span_tags(
+                EvictedHashMap::new(20, 20),
+                None,
+                status_code,
+                error_msg,
+                SpanKind::Client,
+            )
+            .unwrap_or_default();
+            if let Some(val) = status_tag_val {
+                assert_tag_contains(tags.clone(), OTEL_STATUS_CODE, val);
+            } else {
+                assert_tag_not_contains(tags.clone(), OTEL_STATUS_CODE);
+            }
+
+            if let Some(val) = msg_tag_val {
+                assert_tag_contains(tags.clone(), OTEL_STATUS_DESCRIPTION, val);
+            } else {
+                assert_tag_not_contains(tags.clone(), OTEL_STATUS_DESCRIPTION);
+            }
+        }
+        Ok(())
+    }
+}

opentelemetry-zipkin/src/exporter/model/mod.rs

@@ -12,11 +12,10 @@ pub(crate) mod span;
 
 use endpoint::Endpoint;
 
-/// Instrument Library name MUST be reported in Jaeger Span tags with the following key
 const INSTRUMENTATION_LIBRARY_NAME: &str = "otel.library.name";
-
-/// Instrument Library version MUST be reported in Jaeger Span tags with the following key
 const INSTRUMENTATION_LIBRARY_VERSION: &str = "otel.library.version";
+const OTEL_ERROR_DESCRIPTION: &str = "error";
+const OTEL_STATUS_CODE: &str = "otel.status_code";
 
 /// Converts `Event` into an `annotation::Annotation`
 impl Into<annotation::Annotation> for Event {
@@ -58,6 +57,7 @@ fn into_zipkin_span_kind(kind: SpanKind) -> Option<span::Kind> {
 /// Converts a `trace::SpanData` to a `span::SpanData` for a given `ExporterConfig`, which can then
 /// be ingested into a Zipkin collector.
 pub(crate) fn into_zipkin_span(local_endpoint: Endpoint, span_data: trace::SpanData) -> span::Span {
+    // see tests in create/exporter/model/span.rs
     let mut user_defined_span_kind = false;
     let mut tags = map_from_kvs(
         span_data
@@ -88,15 +88,16 @@ pub(crate) fn into_zipkin_span(local_endpoint: Endpoint, span_data: trace::SpanD
                 ]
                 .iter()
                 .filter_map(|(key, val)| val.map(|val| KeyValue::new(*key, val))),
-            ),
+            )
+            .filter(|kv| kv.key.as_str() != "error"),
     );
-
     if let Some(status_code) = from_statuscode_to_str(span_data.status_code) {
-        tags.insert("otel.status_code".into(), status_code.into());
+        if status_code == "ERROR" {
+            tags.insert(OTEL_ERROR_DESCRIPTION.into(), span_data.status_message);
+        }
+        tags.insert(OTEL_STATUS_CODE.into(), status_code.into());
     }
 
-    tags.insert("otel.status_description".into(), span_data.status_message);
-
     span::Span::builder()
         .trace_id(span_data.span_context.trace_id().to_hex())
         .parent_id(span_data.parent_span_id.to_hex())

opentelemetry-zipkin/src/exporter/model/span.rs

@@ -58,8 +58,14 @@ mod tests {
     use crate::exporter::model::annotation::Annotation;
     use crate::exporter::model::endpoint::Endpoint;
     use crate::exporter::model::span::{Kind, Span};
+    use crate::exporter::model::{into_zipkin_span, OTEL_ERROR_DESCRIPTION, OTEL_STATUS_CODE};
+    use opentelemetry::sdk::export::trace::SpanData;
+    use opentelemetry::sdk::trace::{EvictedHashMap, EvictedQueue};
+    use opentelemetry::trace::{SpanContext, SpanId, SpanKind, StatusCode, TraceId};
     use std::collections::HashMap;
     use std::net::Ipv4Addr;
+    use std::sync::Arc;
+    use std::time::SystemTime;
 
     #[test]
     fn test_empty() {
@@ -112,4 +118,77 @@ mod tests {
         let result = serde_json::to_string(&span).unwrap();
         assert_eq!(result, desired.to_owned());
     }
+
+    fn assert_tag_contains(
+        tags: &HashMap<String, String>,
+        key: &'static str,
+        expected_val: Option<&'static str>,
+    ) {
+        let val = tags.get::<String>(&key.to_string()).map(|s| s.as_str());
+        assert_eq!(
+            val,
+            expected_val,
+            "expect value of key {} to be {}, but got {}",
+            key,
+            expected_val.unwrap_or("none"),
+            val.unwrap_or("none")
+        );
+    }
+
+    fn get_set_status_test_data() -> Vec<(
+        StatusCode,
+        String,
+        Option<&'static str>,
+        Option<&'static str>,
+    )> {
+        // status code, status message, whether OTEL_STATUS_CODE is set, whether OTEL_ERROR_DESCRIPTION is set, whether error tag is set
+        vec![
+            (StatusCode::Ok, "".into(), Some("OK"), None),
+            (StatusCode::Error, "".into(), Some("ERROR"), Some("")),
+            (
+                StatusCode::Error,
+                "error msg".into(),
+                Some("ERROR"),
+                Some("error msg"),
+            ),
+            (StatusCode::Unset, "whatever".into(), None, None),
+        ]
+    }
+
+    #[test]
+    fn test_set_status() -> Result<(), Box<dyn std::error::Error>> {
+        for (status_code, status_msg, status_tag_val, status_msg_tag_val) in
+            get_set_status_test_data()
+        {
+            let span_data = SpanData {
+                span_context: SpanContext::new(
+                    TraceId::from_u128(1),
+                    SpanId::from_u64(1),
+                    0,
+                    false,
+                    Default::default(),
+                ),
+                parent_span_id: SpanId::from_u64(1),
+                span_kind: SpanKind::Client,
+                name: "".to_string(),
+                start_time: SystemTime::now(),
+                end_time: SystemTime::now(),
+                attributes: EvictedHashMap::new(20, 20),
+                message_events: EvictedQueue::new(20),
+                links: EvictedQueue::new(20),
+                status_code,
+                status_message: status_msg,
+                resource: Arc::new(Default::default()),
+                instrumentation_lib: Default::default(),
+            };
+            let local_endpoint = Endpoint::new("test".into(), None);
+            let span = into_zipkin_span(local_endpoint, span_data);
+            if let Some(tags) = span.tags.as_ref() {
+                assert_tag_contains(tags, OTEL_STATUS_CODE, status_tag_val);
+                assert_tag_contains(tags, OTEL_ERROR_DESCRIPTION, status_msg_tag_val);
+            };
+        }
+
+        Ok(())
+    }
 }

opentelemetry/src/api/trace/span.rs

@@ -123,7 +123,7 @@ pub trait Span: fmt::Debug + 'static + Send + Sync {
     fn set_attribute(&self, attribute: KeyValue);
 
     /// Sets the status of the `Span`. If used, this will override the default `Span`
-    /// status, which is `OK`.
+    /// status, which is `Unset`. `message` MUST be ignored when the status is `OK` or `Unset`
     ///
     /// Only the value of the last call will be recorded, and implementations are free
     /// to ignore previous calls.
@@ -241,7 +241,7 @@ impl fmt::Display for SpanKind {
 /// It's composed of a canonical code in conjunction with an optional
 /// descriptive message.
 #[cfg_attr(feature = "serialize", derive(Deserialize, Serialize))]
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Copy)]
 pub enum StatusCode {
     /// The default status.
     Unset = 0,
@@ -250,3 +250,14 @@ pub enum StatusCode {
     /// The operation contains an error.
     Error = 2,
 }
+
+impl StatusCode {
+    /// Return a static str that represent the status code
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            StatusCode::Unset => "",
+            StatusCode::Ok => "OK",
+            StatusCode::Error => "ERROR",
+        }
+    }
+}

opentelemetry/src/sdk/trace/span.rs

@@ -129,11 +129,13 @@ impl crate::trace::Span for Span {
     }
 
     /// Sets the status of the `Span`. If used, this will override the default `Span`
-    /// status, which is `Unset`.
+    /// status, which is `Unset`. `message` MUST be ignored when the status is `OK` or `Unset`
     fn set_status(&self, code: StatusCode, message: String) {
         self.with_data(|data| {
+            if code == StatusCode::Error {
+                data.status_message = message;
+            }
             data.status_code = code;
-            data.status_message = message
         });
     }
 
@@ -353,14 +355,36 @@ mod tests {
 
     #[test]
     fn set_status() {
-        let span = create_span();
-        let status = StatusCode::Ok;
-        let message = "OK".to_string();
-        span.set_status(status.clone(), message.clone());
-        span.with_data(|data| {
-            assert_eq!(data.status_code, status);
-            assert_eq!(data.status_message, message);
-        });
+        {
+            let span = create_span();
+            let status = StatusCode::Ok;
+            let message = "OK".to_string();
+            span.set_status(status, message);
+            span.with_data(|data| {
+                assert_eq!(data.status_code, status);
+                assert_eq!(data.status_message, "");
+            });
+        }
+        {
+            let span = create_span();
+            let status = StatusCode::Unset;
+            let message = "OK".to_string();
+            span.set_status(status, message);
+            span.with_data(|data| {
+                assert_eq!(data.status_code, status);
+                assert_eq!(data.status_message, "");
+            });
+        }
+        {
+            let span = create_span();
+            let status = StatusCode::Error;
+            let message = "Error".to_string();
+            span.set_status(status, message);
+            span.with_data(|data| {
+                assert_eq!(data.status_code, status);
+                assert_eq!(data.status_message, "Error");
+            });
+        }
     }
 
     #[test]