From 11dae40d54abc7f7e61d62d7b3cfe40e0e018d7c Mon Sep 17 00:00:00 2001 From: Julian Tescher Date: Sat, 12 Mar 2022 15:45:47 -0500 Subject: [PATCH] api: replace `StatusCode` and message with `Status` The spec defines status as unset, ok, or error with a description. The current api allows for confusion and misuse by providing invalid combinations (e.g. Ok with a description). This change simplifies the api and removes these illegal states by moving the status to be an enum where only the `Status::Error` case accepts a message. --- examples/zpages/src/main.rs | 4 +- opentelemetry-api/src/global/trace.rs | 15 ++- opentelemetry-api/src/testing/trace.rs | 8 +- opentelemetry-api/src/trace/context.rs | 13 +-- opentelemetry-api/src/trace/mod.rs | 6 +- opentelemetry-api/src/trace/noop.rs | 5 +- opentelemetry-api/src/trace/span.rs | 83 ++++++++++---- opentelemetry-api/src/trace/tracer.rs | 35 +++--- .../src/exporter/model/mod.rs | 5 +- .../src/exporter/model/v03.rs | 6 +- .../src/exporter/model/v05.rs | 6 +- opentelemetry-jaeger/src/exporter/mod.rs | 92 +++++++--------- .../tests/integration_test.rs | 4 +- opentelemetry-proto/src/transform/traces.rs | 42 +++++--- opentelemetry-proto/src/transform/tracez.rs | 37 ++++--- .../benches/batch_span_processor.rs | 5 +- opentelemetry-sdk/src/export/trace/mod.rs | 10 +- opentelemetry-sdk/src/testing/trace.rs | 5 +- opentelemetry-sdk/src/trace/span.rs | 102 ++++++------------ opentelemetry-sdk/src/trace/tracer.rs | 11 +- .../src/exporter/model/mod.rs | 32 +++--- .../src/exporter/model/span.rs | 29 ++--- opentelemetry-zpages/src/trace/aggregator.rs | 14 +-- 23 files changed, 256 insertions(+), 313 deletions(-) diff --git a/examples/zpages/src/main.rs b/examples/zpages/src/main.rs index b9251f8fc5..0053b3891e 100644 --- a/examples/zpages/src/main.rs +++ b/examples/zpages/src/main.rs @@ -1,7 +1,7 @@ use hyper::http::{Request, Response}; use hyper::service::{make_service_fn, service_fn}; use hyper::{Body, Server}; -use opentelemetry::trace::{Span, StatusCode}; +use opentelemetry::trace::{Span, Status}; use opentelemetry::{global, runtime::Tokio, sdk::trace, trace::Tracer}; use opentelemetry_zpages::{tracez, TracezError, TracezQuerier, TracezResponse}; use rand::Rng; @@ -66,7 +66,7 @@ async fn handler( "/running" => { let span_duration = Duration::from_millis(rand::thread_rng().gen_range(1..6000)); let mut spans = global::tracer("zpages-test").start("running-spans"); - spans.set_status(StatusCode::Ok, "".to_string()); + spans.set_status(Status::Ok); tokio::time::sleep(span_duration).await; println!("The span slept for {} ms", span_duration.as_millis()); Response::new(Body::empty()) diff --git a/opentelemetry-api/src/global/trace.rs b/opentelemetry-api/src/global/trace.rs index c368b68078..2d4ecffa95 100644 --- a/opentelemetry-api/src/global/trace.rs +++ b/opentelemetry-api/src/global/trace.rs @@ -1,5 +1,5 @@ use crate::global::handle_error; -use crate::trace::{noop::NoopTracerProvider, SpanContext, StatusCode, TraceResult}; +use crate::trace::{noop::NoopTracerProvider, SpanContext, Status, TraceResult}; use crate::{trace, trace::TracerProvider, Context, KeyValue}; use std::borrow::Cow; use std::fmt; @@ -67,7 +67,7 @@ pub trait ObjectSafeSpan { /// to `Unset` will always be ignore, set the status to `Error` only works when current /// status is `Unset`, set the status to `Ok` will be consider final and any further call /// to this function will be ignore. - fn set_status(&mut self, code: StatusCode, message: Cow<'static, str>); + fn set_status(&mut self, status: Status); /// Updates the `Span`'s name. After this update, any sampling behavior based on the /// name will depend on the implementation. @@ -127,8 +127,8 @@ impl ObjectSafeSpan for T { self.set_attribute(attribute) } - fn set_status(&mut self, code: StatusCode, message: Cow<'static, str>) { - self.set_status(code, message) + fn set_status(&mut self, status: Status) { + self.set_status(status) } fn update_name(&mut self, new_name: Cow<'static, str>) { @@ -201,11 +201,8 @@ impl trace::Span for BoxedSpan { /// Sets the status of the `Span`. If used, this will override the default `Span` /// status, which is `Unset`. - fn set_status(&mut self, code: trace::StatusCode, message: T) - where - T: Into>, - { - self.0.set_status(code, message.into()) + fn set_status(&mut self, status: trace::Status) { + self.0.set_status(status) } /// Updates the `Span`'s name. diff --git a/opentelemetry-api/src/testing/trace.rs b/opentelemetry-api/src/testing/trace.rs index 0cdbf0a037..16d5e14f0b 100644 --- a/opentelemetry-api/src/testing/trace.rs +++ b/opentelemetry-api/src/testing/trace.rs @@ -1,5 +1,5 @@ use crate::{ - trace::{Span, SpanContext, SpanId, StatusCode, TraceId}, + trace::{Span, SpanContext, SpanId, Status, TraceId}, KeyValue, }; use std::borrow::Cow; @@ -24,11 +24,7 @@ impl Span for TestSpan { false } fn set_attribute(&mut self, _attribute: KeyValue) {} - fn set_status(&mut self, _code: StatusCode, _message: T) - where - T: Into>, - { - } + fn set_status(&mut self, _status: Status) {} fn update_name(&mut self, _new_name: T) where T: Into>, diff --git a/opentelemetry-api/src/trace/context.rs b/opentelemetry-api/src/trace/context.rs index 0f19c5511d..7dc553a00e 100644 --- a/opentelemetry-api/src/trace/context.rs +++ b/opentelemetry-api/src/trace/context.rs @@ -1,7 +1,7 @@ //! Context extensions for tracing use crate::{ global, - trace::{Span, SpanContext}, + trace::{Span, SpanContext, Status}, Context, ContextGuard, KeyValue, }; use futures_util::{sink::Sink, stream::Stream}; @@ -131,14 +131,9 @@ impl SpanRef<'_> { /// Sets the status of this `Span`. /// - /// If used, this will override the default span status, which is [`StatusCode::Unset`]. - /// - /// [`StatusCode::Unset`]: super::StatusCode::Unset - pub fn set_status(&self, code: super::StatusCode, message: T) - where - T: Into>, - { - self.with_inner_mut(move |inner| inner.set_status(code, message)) + /// If used, this will override the default span status, which is [`Status::Unset`]. + pub fn set_status(&self, status: Status) { + self.with_inner_mut(move |inner| inner.set_status(status)) } /// Updates the span's name. diff --git a/opentelemetry-api/src/trace/mod.rs b/opentelemetry-api/src/trace/mod.rs index d11215ac01..da7e530745 100644 --- a/opentelemetry-api/src/trace/mod.rs +++ b/opentelemetry-api/src/trace/mod.rs @@ -68,14 +68,14 @@ //! [`Context`]: crate::Context //! //! ``` -//! use opentelemetry_api::{global, trace::{self, Span, StatusCode, Tracer, TracerProvider}}; +//! use opentelemetry_api::{global, trace::{self, Span, Status, Tracer, TracerProvider}}; //! //! fn may_error(rand: f32) { //! if rand < 0.5 { //! // Get the currently active span to record additional attributes, //! // status, etc. //! trace::get_active_span(|span| { -//! span.set_status(StatusCode::Error, "value too small"); +//! span.set_status(Status::error("value too small")); //! }); //! } //! } @@ -144,7 +144,7 @@ mod tracer_provider; pub use self::{ context::{get_active_span, mark_span_as_active, FutureExt, SpanRef, TraceContextExt}, - span::{Span, SpanKind, StatusCode}, + span::{Span, SpanKind, Status}, span_context::{SpanContext, SpanId, TraceFlags, TraceId, TraceState}, tracer::{SamplingDecision, SamplingResult, SpanBuilder, Tracer}, tracer_provider::TracerProvider, diff --git a/opentelemetry-api/src/trace/noop.rs b/opentelemetry-api/src/trace/noop.rs index 648e332638..f5f620a124 100644 --- a/opentelemetry-api/src/trace/noop.rs +++ b/opentelemetry-api/src/trace/noop.rs @@ -109,10 +109,7 @@ impl trace::Span for NoopSpan { } /// Ignores status - fn set_status(&mut self, _code: trace::StatusCode, _message: T) - where - T: Into>, - { + fn set_status(&mut self, _status: trace::Status) { // Ignored } diff --git a/opentelemetry-api/src/trace/span.rs b/opentelemetry-api/src/trace/span.rs index 4e11d6fcb1..0028596022 100644 --- a/opentelemetry-api/src/trace/span.rs +++ b/opentelemetry-api/src/trace/span.rs @@ -122,10 +122,8 @@ pub trait Span { /// Sets the status of this `Span`. /// - /// If used, this will override the default span status, which is [`StatusCode::Unset`]. - fn set_status(&mut self, code: StatusCode, message: T) - where - T: Into>; + /// If used, this will override the default span status, which is [`Status::Unset`]. + fn set_status(&mut self, status: Status); /// Updates the span's name. /// @@ -219,38 +217,81 @@ pub enum SpanKind { Internal, } -/// The code representation of the status of a [`Span`]. +/// The status of a [`Span`]. /// /// These values form a total order: Ok > Error > Unset. This means that setting -/// `StatusCode::Ok` will override any prior or future attempts to set a -/// status with `StatusCode::Error` or `StatusCode::Unset`. +/// `Status::Ok` will override any prior or future attempts to set a status with +/// `Status::Error` or `Status::Unset`. /// -/// The status code should remain unset, except for the following circumstances: +/// The status should remain unset, except for the following circumstances: /// -/// Generally, instrumentation libraries should not set the status code to -/// `StatusCode::Ok`, unless explicitly configured to do so. Instrumentation +/// Generally, instrumentation libraries should not set the code to +/// `Status::Ok`, unless explicitly configured to do so. Instrumentation /// libraries should leave the status code as unset unless there is an error. /// -/// Application developers and operators may set the status code to `StatusCode::Ok`. +/// Application developers and operators may set the status code to +/// `Status::Ok`. /// -/// When span status is set to `StatusCode::Ok` it should be considered final and +/// When span status is set to `Status::Ok` it should be considered final and /// any further attempts to change it should be ignored. /// -/// Analysis tools should respond to a `StatusCode::Ok` status by suppressing any -/// errors they would otherwise generate. For example, to suppress noisy errors such -/// as 404s. +/// Analysis tools should respond to a `Status::Ok` status by suppressing any +/// errors they would otherwise generate. For example, to suppress noisy errors +/// such as 404s. /// -/// Only the value of the last call will be recorded, and implementations are free -/// to ignore previous calls. -#[derive(Clone, Debug, PartialEq, Copy)] -pub enum StatusCode { +/// Only the value of the last call will be recorded, and implementations are +/// free to ignore previous calls. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd)] +pub enum Status { /// The default status. Unset, + /// The operation contains an error. + Error { + /// The description of the error + description: Cow<'static, str>, + }, + /// The operation has been validated by an application developer or operator to /// have completed successfully. Ok, +} - /// The operation contains an error. - Error, +impl Status { + /// Create a new error status with a given description. + /// + /// # Examples + /// + /// ``` + /// use opentelemetry_api::trace::Status; + /// + /// // record error with `str` description + /// let error_status = Status::error("something went wrong"); + /// + /// // or with `String` description + /// let error_status = Status::error(format!("too many foos: {}", 42)); + /// # drop(error_status); + /// ``` + pub fn error(description: impl Into>) -> Self { + Status::Error { + description: description.into(), + } + } +} + +impl Default for Status { + fn default() -> Self { + Status::Unset + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn status_order() { + assert!(Status::Ok > Status::error("")); + assert!(Status::error("") > Status::Unset); + } } diff --git a/opentelemetry-api/src/trace/tracer.rs b/opentelemetry-api/src/trace/tracer.rs index 177390429a..3d9ad1a9f5 100644 --- a/opentelemetry-api/src/trace/tracer.rs +++ b/opentelemetry-api/src/trace/tracer.rs @@ -1,7 +1,5 @@ use crate::{ - trace::{ - Event, Link, Span, SpanId, SpanKind, StatusCode, TraceContextExt, TraceId, TraceState, - }, + trace::{Event, Link, Span, SpanId, SpanKind, Status, TraceContextExt, TraceId, TraceState}, Context, KeyValue, }; use std::borrow::Cow; @@ -244,26 +242,34 @@ pub trait Tracer { pub struct SpanBuilder { /// Trace id, useful for integrations with external tracing systems. pub trace_id: Option, + /// Span id, useful for integrations with external tracing systems. pub span_id: Option, + /// Span kind pub span_kind: Option, + /// Span name pub name: Cow<'static, str>, + /// Span start time pub start_time: Option, + /// Span end time pub end_time: Option, + /// Span attributes pub attributes: Option>, + /// Span events pub events: Option>, + /// Span Links pub links: Option>, - /// Span status code - pub status_code: Option, - /// Span status message - pub status_message: Option>, + + /// Span status + pub status: Status, + /// Sampling result pub sampling_result: Option, } @@ -344,19 +350,8 @@ impl SpanBuilder { } /// Assign status code - pub fn with_status_code(self, code: StatusCode) -> Self { - SpanBuilder { - status_code: Some(code), - ..self - } - } - - /// Assign status message - pub fn with_status_message>>(self, message: T) -> Self { - SpanBuilder { - status_message: Some(message.into()), - ..self - } + pub fn with_status(self, status: Status) -> Self { + SpanBuilder { status, ..self } } /// Assign sampling result diff --git a/opentelemetry-datadog/src/exporter/model/mod.rs b/opentelemetry-datadog/src/exporter/model/mod.rs index cc99378698..d0a3d7fd18 100644 --- a/opentelemetry-datadog/src/exporter/model/mod.rs +++ b/opentelemetry-datadog/src/exporter/model/mod.rs @@ -77,7 +77,7 @@ pub(crate) mod tests { use opentelemetry::sdk; use opentelemetry::sdk::InstrumentationLibrary; use opentelemetry::{ - trace::{SpanContext, SpanId, SpanKind, StatusCode, TraceFlags, TraceId, TraceState}, + trace::{SpanContext, SpanId, SpanKind, Status, TraceFlags, TraceId, TraceState}, Key, }; use std::time::{Duration, SystemTime}; @@ -115,8 +115,7 @@ pub(crate) mod tests { attributes, events, links, - status_code: StatusCode::Ok, - status_message: "".into(), + status: Status::Ok, resource: None, instrumentation_lib: InstrumentationLibrary::new("component", None, None), } diff --git a/opentelemetry-datadog/src/exporter/model/v03.rs b/opentelemetry-datadog/src/exporter/model/v03.rs index 855b7c704c..60e332a0cd 100644 --- a/opentelemetry-datadog/src/exporter/model/v03.rs +++ b/opentelemetry-datadog/src/exporter/model/v03.rs @@ -1,6 +1,6 @@ use crate::exporter::model::Error; use opentelemetry::sdk::export::trace; -use opentelemetry::trace::StatusCode; +use opentelemetry::trace::Status; use opentelemetry::{Key, Value}; use std::time::SystemTime; @@ -73,8 +73,8 @@ pub(crate) fn encode( rmp::encode::write_str(&mut encoded, "error")?; rmp::encode::write_i32( &mut encoded, - match span.status_code { - StatusCode::Error => 1, + match span.status { + Status::Error { .. } => 1, _ => 0, }, )?; diff --git a/opentelemetry-datadog/src/exporter/model/v05.rs b/opentelemetry-datadog/src/exporter/model/v05.rs index 2b1d968670..ff55bc3916 100644 --- a/opentelemetry-datadog/src/exporter/model/v05.rs +++ b/opentelemetry-datadog/src/exporter/model/v05.rs @@ -1,7 +1,7 @@ use crate::exporter::intern::StringInterner; use crate::exporter::Error; use opentelemetry::sdk::export::trace; -use opentelemetry::trace::StatusCode; +use opentelemetry::trace::Status; use opentelemetry::{Key, Value}; use std::time::SystemTime; @@ -126,8 +126,8 @@ fn encode_traces( rmp::encode::write_i64(&mut encoded, duration)?; rmp::encode::write_i32( &mut encoded, - match span.status_code { - StatusCode::Error => 1, + match span.status { + Status::Error { .. } => 1, _ => 0, }, )?; diff --git a/opentelemetry-jaeger/src/exporter/mod.rs b/opentelemetry-jaeger/src/exporter/mod.rs index 8422a7a7a3..a35767b31c 100644 --- a/opentelemetry-jaeger/src/exporter/mod.rs +++ b/opentelemetry-jaeger/src/exporter/mod.rs @@ -34,7 +34,7 @@ use opentelemetry::trace::TraceError; use opentelemetry::{ global, sdk, sdk::export::trace, - trace::{Event, Link, SpanKind, StatusCode, TracerProvider}, + trace::{Event, Link, SpanKind, Status, TracerProvider}, Key, KeyValue, }; #[cfg(feature = "collector_client")] @@ -730,8 +730,7 @@ fn convert_otel_span_into_jaeger_span( } else { None }, - span.status_code, - span.status_message.into_owned(), + span.status, span.span_kind, )), logs: events_to_logs(span.events), @@ -741,8 +740,7 @@ fn convert_otel_span_into_jaeger_span( fn build_span_tags( attrs: sdk::trace::EvictedHashMap, instrumentation_lib: Option, - status_code: StatusCode, - status_description: String, + status: Status, kind: SpanKind, ) -> Vec { let mut user_overrides = UserOverrides::default(); @@ -767,25 +765,27 @@ fn build_span_tags( tags.push(Key::new(SPAN_KIND).string(format_span_kind(kind)).into()); } - if status_code != StatusCode::Unset { - // Ensure error status is set unless user has already overrided it - if status_code == StatusCode::Error && !user_overrides.error { - tags.push(Key::new(ERROR).bool(true).into()); - } - if !user_overrides.status_code { - if status_code == StatusCode::Ok { + match status { + Status::Unset => {} + Status::Ok => { + if !user_overrides.status_code { tags.push(KeyValue::new(OTEL_STATUS_CODE, "OK").into()); - } else if status_code == StatusCode::Error { - tags.push(KeyValue::new(OTEL_STATUS_CODE, "ERROR").into()); } } - // set status message if there is one - if !status_description.is_empty() && !user_overrides.status_description { - tags.push( - Key::new(OTEL_STATUS_DESCRIPTION) - .string(status_description) - .into(), - ); + Status::Error { + description: message, + } => { + if !user_overrides.error { + tags.push(Key::new(ERROR).bool(true).into()); + } + + if !user_overrides.status_code { + tags.push(KeyValue::new(OTEL_STATUS_CODE, "ERROR").into()); + } + + if !message.is_empty() && !user_overrides.status_description { + tags.push(Key::new(OTEL_STATUS_DESCRIPTION).string(message).into()); + } } } @@ -983,7 +983,7 @@ mod tests { use crate::exporter::{build_span_tags, OTEL_STATUS_CODE, OTEL_STATUS_DESCRIPTION}; use opentelemetry::sdk::trace::{Config, EvictedHashMap}; use opentelemetry::sdk::Resource; - use opentelemetry::trace::{SpanKind, StatusCode}; + use opentelemetry::trace::{SpanKind, Status}; use opentelemetry::KeyValue; use std::env; use std::sync::Arc; @@ -1012,38 +1012,24 @@ mod tests { ); } - 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 + #[rustfmt::skip] + fn get_error_tag_test_data() -> Vec<(Status, Option<&'static str>, Option<&'static str>)> + { + // Status, OTEL_STATUS_CODE tag value, OTEL_STATUS_DESCRIPTION tag value vec![ - (StatusCode::Error, "".into(), Some("ERROR"), None), - (StatusCode::Unset, "".into(), None, None), + (Status::error(""), Some("ERROR"), None), + (Status::Unset, 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), + (Status::Ok, Some("OK"), None), + (Status::error("have message"), Some("ERROR"), Some("have message")), + (Status::Unset, None, None), ] } #[test] fn test_set_status() { - 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, - ); + for (status, status_tag_val, msg_tag_val) in get_error_tag_test_data() { + let tags = build_span_tags(EvictedHashMap::new(20, 20), None, status, SpanKind::Client); if let Some(val) = status_tag_val { assert_tag_contains(tags.clone(), OTEL_STATUS_CODE, val); } else { @@ -1063,8 +1049,10 @@ mod tests { let mut attributes = EvictedHashMap::new(20, 20); let user_error = true; let user_kind = "server"; - let user_status_code = StatusCode::Error; let user_status_description = "Something bad happened"; + let user_status = Status::Error { + description: user_status_description.into(), + }; attributes.insert(KeyValue::new("error", user_error)); attributes.insert(KeyValue::new(SPAN_KIND, user_kind)); attributes.insert(KeyValue::new(OTEL_STATUS_CODE, "ERROR")); @@ -1072,13 +1060,7 @@ mod tests { OTEL_STATUS_DESCRIPTION, user_status_description, )); - let tags = build_span_tags( - attributes, - None, - user_status_code, - user_status_description.to_string(), - SpanKind::Client, - ); + let tags = build_span_tags(attributes, None, user_status, SpanKind::Client); assert!(tags .iter() diff --git a/opentelemetry-jaeger/tests/integration_test.rs b/opentelemetry-jaeger/tests/integration_test.rs index 251833c93e..2f300cd545 100644 --- a/opentelemetry-jaeger/tests/integration_test.rs +++ b/opentelemetry-jaeger/tests/integration_test.rs @@ -1,7 +1,7 @@ #[cfg(feature = "integration_test")] mod tests { use opentelemetry::sdk::trace::Tracer as SdkTracer; - use opentelemetry::trace::{StatusCode, TraceContextExt, Tracer, TracerProvider}; + use opentelemetry::trace::{Status, TraceContextExt, Tracer, TracerProvider}; use opentelemetry::KeyValue; use opentelemetry_jaeger::testing::{ jaeger_api_v2 as jaeger_api, jaeger_client::JaegerTestClient, @@ -28,7 +28,7 @@ mod tests { tracer.in_span("step-2-2", |_cx| { tracer.in_span("step-3-1", |cx| { let span = cx.span(); - span.set_status(StatusCode::Error, "") + span.set_status(Status::error("some err")) }); tracer.in_span("step-3-2", |cx| { cx.span() diff --git a/opentelemetry-proto/src/transform/traces.rs b/opentelemetry-proto/src/transform/traces.rs index 1015f6310f..6046e83b4a 100644 --- a/opentelemetry-proto/src/transform/traces.rs +++ b/opentelemetry-proto/src/transform/traces.rs @@ -1,6 +1,6 @@ use crate::transform::common::to_nanos; use opentelemetry::sdk::{self, export::trace::SpanData}; -use opentelemetry::trace::{Link, SpanId, SpanKind, StatusCode}; +use opentelemetry::trace::{Link, SpanId, SpanKind}; #[cfg(feature = "gen-tonic")] pub mod tonic { @@ -10,6 +10,7 @@ pub mod tonic { span, status, InstrumentationLibrarySpans, ResourceSpans, Span, Status, }; use crate::transform::common::tonic::Attributes; + use opentelemetry::trace; impl From for span::SpanKind { fn from(span_kind: SpanKind) -> Self { @@ -23,12 +24,12 @@ pub mod tonic { } } - impl From for status::StatusCode { - fn from(status_code: StatusCode) -> Self { - match status_code { - StatusCode::Ok => status::StatusCode::Ok, - StatusCode::Unset => status::StatusCode::Unset, - StatusCode::Error => status::StatusCode::Error, + impl From<&trace::Status> for status::StatusCode { + fn from(status: &trace::Status) -> Self { + match status { + trace::Status::Ok => status::StatusCode::Ok, + trace::Status::Unset => status::StatusCode::Unset, + trace::Status::Error { .. } => status::StatusCode::Error, } } } @@ -96,8 +97,11 @@ pub mod tonic { dropped_links_count: source_span.links.dropped_count(), links: source_span.links.into_iter().map(Into::into).collect(), status: Some(Status { - code: status::StatusCode::from(source_span.status_code).into(), - message: source_span.status_message.into_owned(), + code: status::StatusCode::from(&source_span.status).into(), + message: match source_span.status { + trace::Status::Error { description } => description.to_string(), + _ => Default::default(), + }, ..Default::default() }), }], @@ -127,6 +131,7 @@ pub mod grpcio { Status, Status_StatusCode, }; use crate::transform::common::grpcio::Attributes; + use opentelemetry::trace; use protobuf::{RepeatedField, SingularPtrField}; impl From for Span_SpanKind { @@ -141,12 +146,12 @@ pub mod grpcio { } } - impl From for Status_StatusCode { - fn from(status_code: StatusCode) -> Self { - match status_code { - StatusCode::Ok => Status_StatusCode::STATUS_CODE_OK, - StatusCode::Unset => Status_StatusCode::STATUS_CODE_UNSET, - StatusCode::Error => Status_StatusCode::STATUS_CODE_ERROR, + impl From<&trace::Status> for Status_StatusCode { + fn from(status: &trace::Status) -> Self { + match status { + trace::Status::Ok => Status_StatusCode::STATUS_CODE_OK, + trace::Status::Unset => Status_StatusCode::STATUS_CODE_UNSET, + trace::Status::Error { .. } => Status_StatusCode::STATUS_CODE_ERROR, } } } @@ -222,8 +227,11 @@ pub mod grpcio { source_span.links.into_iter().map(Into::into).collect(), ), status: SingularPtrField::some(Status { - code: Status_StatusCode::from(source_span.status_code), - message: source_span.status_message.into_owned(), + code: Status_StatusCode::from(&source_span.status), + message: match source_span.status { + trace::Status::Error { description } => description.to_string(), + _ => Default::default(), + }, ..Default::default() }), ..Default::default() diff --git a/opentelemetry-proto/src/transform/tracez.rs b/opentelemetry-proto/src/transform/tracez.rs index 9b31979ce1..d2ff72da93 100644 --- a/opentelemetry-proto/src/transform/tracez.rs +++ b/opentelemetry-proto/src/transform/tracez.rs @@ -2,14 +2,17 @@ mod grpcio { use opentelemetry::{ sdk::export::trace::SpanData, - trace::{Event, StatusCode}, + trace::{self, Event}, }; - use crate::proto::grpcio::{ - trace::{Span_Event, Status}, - tracez::{ErrorData, LatencyData, RunningData}, - }; use crate::transform::common::{grpcio::Attributes, to_nanos}; + use crate::{ + grpcio::trace::Status_StatusCode, + proto::grpcio::{ + trace::{Span_Event, Status}, + tracez::{ErrorData, LatencyData, RunningData}, + }, + }; impl From for LatencyData { fn from(span_data: SpanData) -> Self { @@ -37,10 +40,16 @@ mod grpcio { attributes: Attributes::from(span_data.attributes).0, events: span_data.events.iter().cloned().map(Into::into).collect(), links: span_data.links.iter().cloned().map(Into::into).collect(), - status: ::protobuf::SingularPtrField::from(Some(Status::from(( - span_data.status_code, - span_data.status_message.to_string(), - )))), + status: ::protobuf::SingularPtrField::from(match span_data.status { + trace::Status::Error { + description: message, + } => Some(Status { + message: message.to_string(), + code: Status_StatusCode::STATUS_CODE_ERROR, + ..Default::default() + }), + _ => None, + }), ..Default::default() } } @@ -61,16 +70,6 @@ mod grpcio { } } - impl From<(StatusCode, String)> for Status { - fn from((status_code, status_message): (StatusCode, String)) -> Self { - Status { - message: status_message, - code: status_code.into(), - ..Default::default() - } - } - } - impl From for Span_Event { fn from(event: Event) -> Self { Span_Event { diff --git a/opentelemetry-sdk/benches/batch_span_processor.rs b/opentelemetry-sdk/benches/batch_span_processor.rs index 2fb6a26cad..1d872739cb 100644 --- a/opentelemetry-sdk/benches/batch_span_processor.rs +++ b/opentelemetry-sdk/benches/batch_span_processor.rs @@ -1,6 +1,6 @@ use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; use opentelemetry_api::trace::{ - SpanContext, SpanId, SpanKind, StatusCode, TraceFlags, TraceId, TraceState, + SpanContext, SpanId, SpanKind, Status, TraceFlags, TraceId, TraceState, }; use opentelemetry_sdk::export::trace::SpanData; use opentelemetry_sdk::runtime::Tokio; @@ -29,8 +29,7 @@ fn get_span_data() -> Vec { attributes: EvictedHashMap::new(12, 12), events: EvictedQueue::new(12), links: EvictedQueue::new(12), - status_code: StatusCode::Unset, - status_message: Default::default(), + status: Status::Unset, resource: None, instrumentation_lib: Default::default(), }) diff --git a/opentelemetry-sdk/src/export/trace/mod.rs b/opentelemetry-sdk/src/export/trace/mod.rs index 2fe904419c..5460afe017 100644 --- a/opentelemetry-sdk/src/export/trace/mod.rs +++ b/opentelemetry-sdk/src/export/trace/mod.rs @@ -1,8 +1,6 @@ //! Trace exporters use async_trait::async_trait; -use opentelemetry_api::trace::{ - Event, Link, SpanContext, SpanId, SpanKind, StatusCode, TraceError, -}; +use opentelemetry_api::trace::{Event, Link, SpanContext, SpanId, SpanKind, Status, TraceError}; use std::borrow::Cow; use std::fmt::Debug; use std::sync::Arc; @@ -72,10 +70,8 @@ pub struct SpanData { pub events: crate::trace::EvictedQueue, /// Span Links pub links: crate::trace::EvictedQueue, - /// Span status code - pub status_code: StatusCode, - /// Span status message - pub status_message: Cow<'static, str>, + /// Span status + pub status: Status, /// Resource contains attributes representing an entity that produced this span. pub resource: Option>, /// Instrumentation library that produced this span diff --git a/opentelemetry-sdk/src/testing/trace.rs b/opentelemetry-sdk/src/testing/trace.rs index bfe3bf07d0..03781fe66d 100644 --- a/opentelemetry-sdk/src/testing/trace.rs +++ b/opentelemetry-sdk/src/testing/trace.rs @@ -8,7 +8,7 @@ use crate::{ }; use async_trait::async_trait; pub use opentelemetry_api::testing::trace::TestSpan; -use opentelemetry_api::trace::{SpanContext, SpanId, SpanKind, StatusCode}; +use opentelemetry_api::trace::{SpanContext, SpanId, SpanKind, Status}; use std::fmt::{Display, Formatter}; use std::sync::mpsc::{channel, Receiver, Sender}; @@ -24,8 +24,7 @@ pub fn new_test_export_span_data() -> SpanData { attributes: EvictedHashMap::new(config.span_limits.max_attributes_per_span, 0), events: EvictedQueue::new(config.span_limits.max_events_per_span), links: EvictedQueue::new(config.span_limits.max_links_per_span), - status_code: StatusCode::Unset, - status_message: "".into(), + status: Status::Unset, resource: config.resource, instrumentation_lib: InstrumentationLibrary::default(), } diff --git a/opentelemetry-sdk/src/trace/span.rs b/opentelemetry-sdk/src/trace/span.rs index f65da9496f..1c008d2e88 100644 --- a/opentelemetry-sdk/src/trace/span.rs +++ b/opentelemetry-sdk/src/trace/span.rs @@ -9,7 +9,7 @@ //! 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::SpanLimits; -use opentelemetry_api::trace::{Event, SpanContext, SpanId, SpanKind, StatusCode}; +use opentelemetry_api::trace::{Event, SpanContext, SpanId, SpanKind, Status}; use opentelemetry_api::{trace, KeyValue}; use std::borrow::Cow; use std::sync::Arc; @@ -42,10 +42,8 @@ pub(crate) struct SpanData { pub(crate) events: crate::trace::EvictedQueue, /// Span Links pub(crate) links: crate::trace::EvictedQueue, - /// Span status code - pub(crate) status_code: StatusCode, - /// Span status message - pub(crate) status_message: Cow<'static, str>, + /// Span status + pub(crate) status: Status, } impl Span { @@ -138,22 +136,16 @@ impl opentelemetry_api::trace::Span for Span { }); } - /// Sets the status of the `Span`. If used, this will override the default `Span` - /// status, which is `Unset`. `message` MUST be ignored when the status is `OK` or `Unset` - fn set_status(&mut self, code: StatusCode, message: T) - where - T: Into>, - { + /// Sets the status of this `Span`. + /// + /// If used, this will override the default span status, which is [`Status::Unset`]. + fn set_status(&mut self, status: Status) { self.with_data(|data| { - // check if we should ignore - if code_priority(code) < code_priority(data.status_code) { - // ignore if the current code has higher priority. - return; + // check if we should update the status + // These values form a total order: Ok > Error > Unset. + if status > data.status { + data.status = status; } - if code == StatusCode::Error { - data.status_message = message.into(); - } - data.status_code = code; }); } @@ -242,21 +234,12 @@ fn build_export_data( attributes: data.attributes, events: data.events, links: data.links, - status_code: data.status_code, - status_message: data.status_message, + status: data.status, resource, instrumentation_lib: tracer.instrumentation_library().clone(), } } -fn code_priority(code: StatusCode) -> i32 { - match code { - StatusCode::Unset => 0, - StatusCode::Error => 1, - StatusCode::Ok => 2, - } -} - #[cfg(all(test, feature = "testing"))] mod tests { use super::*; @@ -284,8 +267,7 @@ mod tests { ), events: crate::trace::EvictedQueue::new(config.span_limits.max_events_per_span), links: crate::trace::EvictedQueue::new(config.span_limits.max_links_per_span), - status_code: StatusCode::Unset, - status_message: "".into(), + status: Status::Unset, }; (tracer, data) } @@ -394,52 +376,35 @@ mod tests { fn set_status() { { let mut 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 status = Status::Ok; + span.set_status(status.clone()); + span.with_data(|data| assert_eq!(data.status, status)); } { let mut 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 status = Status::Unset; + span.set_status(status.clone()); + span.with_data(|data| assert_eq!(data.status, status)); } { let mut 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"); - }); + let status = Status::error("error"); + span.set_status(status.clone()); + span.with_data(|data| assert_eq!(data.status, status)); } { let mut span = create_span(); - // ok status code should be final - span.set_status(StatusCode::Ok, "".to_string()); - span.set_status(StatusCode::Error, "error".to_string()); - span.with_data(|data| { - assert_eq!(data.status_code, StatusCode::Ok); - assert_eq!(data.status_message, ""); - }); + // ok status should be final + span.set_status(Status::Ok); + span.set_status(Status::error("error")); + span.with_data(|data| assert_eq!(data.status, Status::Ok)); } { let mut span = create_span(); - // error status code should be able to override unset - span.set_status(StatusCode::Error, "error".to_string()); - span.with_data(|data| { - assert_eq!(data.status_code, StatusCode::Error); - assert_eq!(data.status_message, "error"); - }); + // error status should be able to override unset + span.set_status(Status::Unset); + span.set_status(Status::error("error")); + span.with_data(|data| assert_ne!(data.status, Status::Ok)); } } @@ -497,13 +462,12 @@ mod tests { let err = std::io::Error::from(std::io::ErrorKind::Other); span.record_error(&err); span.set_attribute(KeyValue::new("k", "v")); - span.set_status(StatusCode::Error, "ERROR".to_string()); + span.set_status(Status::error("ERROR")); span.update_name("new_name".to_string()); span.with_data(|data| { assert_eq!(data.events, initial.events); assert_eq!(data.attributes, initial.attributes); - assert_eq!(data.status_code, initial.status_code); - assert_eq!(data.status_message, initial.status_message); + assert_eq!(data.status, initial.status); assert_eq!(data.name, initial.name); }); } @@ -600,7 +564,7 @@ mod tests { let mut span = tracer.start("test_span"); span.add_event("test_event".to_string(), vec![]); - span.set_status(StatusCode::Error, "".to_string()); + span.set_status(Status::error("")); let exported_data = span.exported_data(); assert!(exported_data.is_some()); diff --git a/opentelemetry-sdk/src/trace/tracer.rs b/opentelemetry-sdk/src/trace/tracer.rs index 652eac2b71..ace67a34e9 100644 --- a/opentelemetry-sdk/src/trace/tracer.rs +++ b/opentelemetry-sdk/src/trace/tracer.rs @@ -17,11 +17,10 @@ use crate::{ InstrumentationLibrary, }; use opentelemetry_api::trace::{ - Link, SamplingDecision, SamplingResult, SpanBuilder, SpanContext, SpanId, SpanKind, StatusCode, + Link, SamplingDecision, SamplingResult, SpanBuilder, SpanContext, SpanId, SpanKind, TraceContextExt, TraceFlags, TraceId, TraceState, }; use opentelemetry_api::{Context, KeyValue}; -use std::borrow::Cow; use std::fmt; use std::sync::Weak; @@ -214,8 +213,7 @@ impl opentelemetry_api::trace::Tracer for Tracer { start_time, end_time, events, - status_code, - status_message, + status, .. } = builder; @@ -255,8 +253,6 @@ impl opentelemetry_api::trace::Tracer for Tracer { } events_queue.append_vec(&mut events); } - let status_code = status_code.unwrap_or(StatusCode::Unset); - let status_message = status_message.unwrap_or(Cow::Borrowed("")); let span_context = SpanContext::new(trace_id, span_id, flags, false, trace_state); Span::new( @@ -270,8 +266,7 @@ impl opentelemetry_api::trace::Tracer for Tracer { attributes, events: events_queue, links, - status_code, - status_message, + status, }), self.clone(), span_limits, diff --git a/opentelemetry-zipkin/src/exporter/model/mod.rs b/opentelemetry-zipkin/src/exporter/model/mod.rs index fe5ba66677..39b916803d 100644 --- a/opentelemetry-zipkin/src/exporter/model/mod.rs +++ b/opentelemetry-zipkin/src/exporter/model/mod.rs @@ -1,6 +1,6 @@ use opentelemetry::{ sdk::export::trace, - trace::{SpanKind, StatusCode}, + trace::{SpanKind, Status}, Key, KeyValue, }; use std::collections::HashMap; @@ -17,16 +17,6 @@ const INSTRUMENTATION_LIBRARY_VERSION: &str = "otel.library.version"; const OTEL_ERROR_DESCRIPTION: &str = "error"; const OTEL_STATUS_CODE: &str = "otel.status_code"; -/// Converts StatusCode to Option<&'static str> -/// `Unset` status code is unused. -fn from_statuscode_to_str(status_code: StatusCode) -> Option<&'static str> { - match status_code { - StatusCode::Ok => Some("OK"), - StatusCode::Unset => None, - StatusCode::Error => Some("ERROR"), - } -} - /// Converts `SpanKind` into an `Option` fn into_zipkin_span_kind(kind: SpanKind) -> Option { match kind { @@ -71,15 +61,19 @@ pub(crate) fn into_zipkin_span(local_endpoint: Endpoint, span_data: trace::SpanD ) .filter(|kv| kv.key.as_str() != "error"), ); - if let Some(status_code) = from_statuscode_to_str(span_data.status_code) { - if status_code == "ERROR" { - tags.insert( - OTEL_ERROR_DESCRIPTION.into(), - span_data.status_message.into_owned(), - ); + + match span_data.status { + Status::Unset => {} + Status::Ok => { + tags.insert(OTEL_STATUS_CODE.into(), "OK".into()); } - tags.insert(OTEL_STATUS_CODE.into(), status_code.into()); - } + Status::Error { + description: message, + } => { + tags.insert(OTEL_STATUS_CODE.into(), "ERROR".into()); + tags.insert(OTEL_ERROR_DESCRIPTION.into(), message.into_owned()); + } + }; span::Span::builder() .trace_id(span_data.span_context.trace_id().to_string()) diff --git a/opentelemetry-zipkin/src/exporter/model/span.rs b/opentelemetry-zipkin/src/exporter/model/span.rs index e1eea8b7eb..2360a6f531 100644 --- a/opentelemetry-zipkin/src/exporter/model/span.rs +++ b/opentelemetry-zipkin/src/exporter/model/span.rs @@ -61,7 +61,7 @@ mod tests { 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, TraceFlags, TraceId}; + use opentelemetry::trace::{SpanContext, SpanId, SpanKind, Status, TraceFlags, TraceId}; use std::collections::HashMap; use std::net::Ipv4Addr; use std::time::SystemTime; @@ -134,31 +134,19 @@ mod tests { ); } - fn get_set_status_test_data() -> Vec<( - StatusCode, - String, - Option<&'static str>, - Option<&'static str>, - )> { + fn get_set_status_test_data() -> Vec<(Status, 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), + (Status::Ok, Some("OK"), None), + (Status::error(""), Some("ERROR"), Some("")), + (Status::error("error msg"), Some("ERROR"), Some("error msg")), + (Status::Unset, None, None), ] } #[test] fn test_set_status() { - for (status_code, status_msg, status_tag_val, status_msg_tag_val) in - get_set_status_test_data() - { + for (status, 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), @@ -175,8 +163,7 @@ mod tests { attributes: EvictedHashMap::new(20, 20), events: EvictedQueue::new(20), links: EvictedQueue::new(20), - status_code, - status_message: status_msg.into(), + status, resource: None, instrumentation_lib: Default::default(), }; diff --git a/opentelemetry-zpages/src/trace/aggregator.rs b/opentelemetry-zpages/src/trace/aggregator.rs index 1ea9b56e81..efba7ba4bb 100644 --- a/opentelemetry-zpages/src/trace/aggregator.rs +++ b/opentelemetry-zpages/src/trace/aggregator.rs @@ -9,7 +9,7 @@ use async_channel::Receiver; use futures_util::StreamExt as _; -use opentelemetry::trace::StatusCode; +use opentelemetry::trace::Status; use crate::trace::{TracezError, TracezMessage, TracezQuery, TracezResponse}; use crate::SpanQueue; @@ -74,7 +74,7 @@ impl SpanAggregator { summary.running.remove(span.span_context.clone()); - if span.status_code == StatusCode::Error { + if matches!(span.status, Status::Error { .. }) { summary.error.push_back(span); } else { let latency_idx = latency_bucket(span.start_time, span.end_time); @@ -201,7 +201,7 @@ impl> From for Vec { mod tests { use std::time::{Duration, SystemTime}; - use opentelemetry::trace::{SpanContext, SpanId, StatusCode, TraceFlags, TraceId, TraceState}; + use opentelemetry::trace::{SpanContext, SpanId, Status, TraceFlags, TraceId, TraceState}; use crate::trace::aggregator::{SpanAggregator, LATENCY_BUCKET_COUNT}; use crate::trace::span_queue::SpanQueue; @@ -292,11 +292,11 @@ mod tests { TraceState::default(), ); span_data.name = Cow::from("test-service"); - span_data.status_code = { + span_data.status = { if is_error { - StatusCode::Error + Status::error("") } else { - StatusCode::Ok + Status::Ok } }; span_data @@ -380,7 +380,7 @@ mod tests { .iter() .any(|expected_span| collected_span.span_context == expected_span.span_context - && collected_span.status_code == expected_span.status_code), + && collected_span.status == expected_span.status), "{}", msg )