From 1a6d493b0f1ef4084f431eb36883499cff3ca9ee Mon Sep 17 00:00:00 2001 From: Zhongyang Wu Date: Sun, 17 Mar 2024 21:02:58 -0700 Subject: [PATCH 1/3] To be finished --- .../src/propagator.rs | 11 +++++-- opentelemetry-sdk/src/propagation/baggage.rs | 24 +++++++++----- opentelemetry-sdk/src/propagation/mod.rs | 3 ++ opentelemetry/src/global/error_handler.rs | 5 +++ opentelemetry/src/propagation/mod.rs | 31 +++++++++++++++++++ 5 files changed, 63 insertions(+), 11 deletions(-) diff --git a/opentelemetry-jaeger-propagator/src/propagator.rs b/opentelemetry-jaeger-propagator/src/propagator.rs index f6dcc86453..f4b9c0b40f 100644 --- a/opentelemetry-jaeger-propagator/src/propagator.rs +++ b/opentelemetry-jaeger-propagator/src/propagator.rs @@ -6,6 +6,7 @@ use opentelemetry::{ }; use std::borrow::Cow; use std::str::FromStr; +use opentelemetry::propagation::PropagationError; const JAEGER_HEADER: &str = "uber-trace-id"; const JAEGER_BAGGAGE_PREFIX: &str = "uberctx-"; @@ -69,7 +70,7 @@ impl Propagator { } /// Extract span context from header value - fn extract_span_context(&self, extractor: &dyn Extractor) -> Result { + fn extract_span_context(&self, extractor: &dyn Extractor) -> Option { let mut header_value = Cow::from(extractor.get(self.header_name).unwrap_or("")); // if there is no :, it means header_value could be encoded as url, try decode first if !header_value.contains(':') { @@ -78,7 +79,11 @@ impl Propagator { let parts = header_value.split_terminator(':').collect::>(); if parts.len() != 4 { - return Err(()); + global::handle_error(Error::Propagation(PropagationError::extract( + "invalid jaeger header format", + "JaegerPropagator", + ))); + return None; } // extract trace id @@ -88,7 +93,7 @@ impl Propagator { let flags = self.extract_trace_flags(parts[3])?; let state = self.extract_trace_state(extractor)?; - Ok(SpanContext::new(trace_id, span_id, flags, true, state)) + Some(SpanContext::new(trace_id, span_id, flags, true, state)) } /// Extract trace id from the header. diff --git a/opentelemetry-sdk/src/propagation/baggage.rs b/opentelemetry-sdk/src/propagation/baggage.rs index 4e42e722f6..e8e901ec89 100644 --- a/opentelemetry-sdk/src/propagation/baggage.rs +++ b/opentelemetry-sdk/src/propagation/baggage.rs @@ -1,6 +1,8 @@ use once_cell::sync::Lazy; +use opentelemetry::propagation::PropagationError; use opentelemetry::{ baggage::{BaggageExt, KeyValueMetadata}, + global, propagation::{text_map_propagator::FieldIter, Extractor, Injector, TextMapPropagator}, Context, }; @@ -111,18 +113,24 @@ impl TextMapPropagator for BaggagePropagator { .collect::>() .join(";"); // join with ; because we deleted all ; when calling split above - Ok(KeyValueMetadata::new( + Some(KeyValueMetadata::new( name.trim().to_owned(), value.trim().to_string(), decoded_props.as_str(), )) } else { - // Invalid name-value format - Err(()) + global::handle_error(PropagationError::extract( + "invalid baggage key-value format", + "BaggagePropagator", + )); + None } } else { - // Invalid baggage value format - Err(()) + global::handle_error(PropagationError::extract( + "invalid baggage format", + "BaggagePropagator", + )); + None } }); cx.with_baggage(baggage) @@ -172,7 +180,7 @@ mod tests { vec![ (Key::new("key1"), (Value::from("value1"), BaggageMetadata::from("property1;property2"))), (Key::new("key2"), (Value::from("value2"), BaggageMetadata::default())), - (Key::new("key3"), (Value::from("value3"), BaggageMetadata::from("propertyKey=propertyValue"))) + (Key::new("key3"), (Value::from("value3"), BaggageMetadata::from("propertyKey=propertyValue"))), ].into_iter().collect()), ] } @@ -220,12 +228,12 @@ mod tests { vec![ KeyValueMetadata::new("key1", "val1", "prop1"), KeyValue::new("key2", "val2").into(), - KeyValueMetadata::new("key3", "val3", "anykey=anyvalue") + KeyValueMetadata::new("key3", "val3", "anykey=anyvalue"), ], vec![ "key1=val1;prop1", "key2=val2", - "key3=val3;anykey=anyvalue" + "key3=val3;anykey=anyvalue", ], ) ] diff --git a/opentelemetry-sdk/src/propagation/mod.rs b/opentelemetry-sdk/src/propagation/mod.rs index 6cfb5d07bd..e7ed4058cd 100644 --- a/opentelemetry-sdk/src/propagation/mod.rs +++ b/opentelemetry-sdk/src/propagation/mod.rs @@ -3,4 +3,7 @@ mod baggage; mod trace_context; pub use baggage::BaggagePropagator; +use std::fmt::Display; pub use trace_context::TraceContextPropagator; + + diff --git a/opentelemetry/src/global/error_handler.rs b/opentelemetry/src/global/error_handler.rs index 290a8a2043..8bcecf465f 100644 --- a/opentelemetry/src/global/error_handler.rs +++ b/opentelemetry/src/global/error_handler.rs @@ -8,6 +8,7 @@ use crate::metrics::MetricsError; #[cfg(feature = "trace")] use crate::trace::TraceError; use once_cell::sync::Lazy; +use crate::propagation::PropagationError; static GLOBAL_ERROR_HANDLER: Lazy>> = Lazy::new(|| RwLock::new(None)); @@ -31,6 +32,9 @@ pub enum Error { #[error(transparent)] /// Failed to export logs. Log(#[from] LogError), + + #[error(transparent)] + Propagation(#[from] PropagationError), #[error("{0}")] /// Other types of failures not covered by the variants above. @@ -61,6 +65,7 @@ pub fn handle_error>(err: T) { #[cfg(feature = "logs")] #[cfg_attr(docsrs, doc(cfg(feature = "logs")))] Error::Log(err) => eprintln!("OpenTelemetry log error occurred. {}", err), + Error::Propagation(err) => eprintln!("OpenTelemetry propagation error occurred. {}", err), Error::Other(err_msg) => eprintln!("OpenTelemetry error occurred. {}", err_msg), }, } diff --git a/opentelemetry/src/propagation/mod.rs b/opentelemetry/src/propagation/mod.rs index 7701b3a75a..8885c1cffc 100644 --- a/opentelemetry/src/propagation/mod.rs +++ b/opentelemetry/src/propagation/mod.rs @@ -20,6 +20,8 @@ //! [`Context`]: crate::Context use std::collections::HashMap; +use std::fmt::Display; +use thiserror::Error; pub mod composite; pub mod text_map_propagator; @@ -61,6 +63,35 @@ impl Extractor for HashMap { } } +/// Error when extracting or injecting context data(i.e propagating) across application boundaries. +#[derive(Error, Debug)] +#[error("Cannot {} from {}, {}", ops, message, propagator_name)] +pub struct PropagationError { + message: &'static str, + // which propagator does this error comes from + propagator_name: &'static str, + // are we extracting or injecting information across application boundaries + ops: &'static str, +} + +impl PropagationError{ + pub fn extract(message: &'static str, propagator_name: &'static str) -> Self { + PropagationError { + message, + propagator_name, + ops: "extract", + } + } + + pub fn inject(message: &'static str, propagator_name: &'static str) -> Self { + PropagationError { + message, + propagator_name, + ops: "inject" + } + } +} + #[cfg(test)] mod tests { use super::*; From e017fabe3b6352b08402c012b046aed16535494b Mon Sep 17 00:00:00 2001 From: Zhongyang Wu Date: Sun, 24 Mar 2024 18:56:21 -0700 Subject: [PATCH 2/3] fix lints --- .../src/propagator.rs | 44 ++++++++++++------- opentelemetry-sdk/src/propagation/baggage.rs | 38 +++++++++------- opentelemetry-sdk/src/propagation/mod.rs | 3 -- opentelemetry/src/global/error_handler.rs | 9 ++-- opentelemetry/src/propagation/mod.rs | 7 +-- 5 files changed, 60 insertions(+), 41 deletions(-) diff --git a/opentelemetry-jaeger-propagator/src/propagator.rs b/opentelemetry-jaeger-propagator/src/propagator.rs index f4b9c0b40f..4ddc5644da 100644 --- a/opentelemetry-jaeger-propagator/src/propagator.rs +++ b/opentelemetry-jaeger-propagator/src/propagator.rs @@ -1,3 +1,4 @@ +use opentelemetry::propagation::PropagationError; use opentelemetry::{ global::{self, Error}, propagation::{text_map_propagator::FieldIter, Extractor, Injector, TextMapPropagator}, @@ -6,7 +7,6 @@ use opentelemetry::{ }; use std::borrow::Cow; use std::str::FromStr; -use opentelemetry::propagation::PropagationError; const JAEGER_HEADER: &str = "uber-trace-id"; const JAEGER_BAGGAGE_PREFIX: &str = "uberctx-"; @@ -86,14 +86,24 @@ impl Propagator { return None; } - // extract trace id - let trace_id = self.extract_trace_id(parts[0])?; - let span_id = self.extract_span_id(parts[1])?; - // Ignore parent span id since it's deprecated. - let flags = self.extract_trace_flags(parts[3])?; - let state = self.extract_trace_state(extractor)?; - - Some(SpanContext::new(trace_id, span_id, flags, true, state)) + match ( + self.extract_trace_id(parts[0]), + self.extract_span_id(parts[1]), + // Ignore parent span id since it's deprecated. + self.extract_trace_flags(parts[3]), + self.extract_trace_state(extractor), + ) { + (Ok(trace_id), Ok(span_id), Ok(flags), Ok(state)) => { + Some(SpanContext::new(trace_id, span_id, flags, true, state)) + } + _ => { + global::handle_error(Error::Propagation(PropagationError::extract( + "invalid jaeger header format", + "JaegerPropagator", + ))); + None + } + } } /// Extract trace id from the header. @@ -193,7 +203,7 @@ impl TextMapPropagator for Propagator { fn extract_with_context(&self, cx: &Context, extractor: &dyn Extractor) -> Context { self.extract_span_context(extractor) .map(|sc| cx.with_remote_span_context(sc)) - .unwrap_or_else(|_| cx.clone()) + .unwrap_or_else(|| cx.clone()) } fn fields(&self) -> FieldIter<'_> { @@ -435,7 +445,7 @@ mod tests { ); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Ok(SpanContext::new( + Some(SpanContext::new( TraceId::from_hex("12345").unwrap(), SpanId::from_hex("54321").unwrap(), TRACE_FLAG_DEBUG | TraceFlags::SAMPLED, @@ -452,7 +462,7 @@ mod tests { ); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Ok(SpanContext::new( + Some(SpanContext::new( TraceId::from_hex("12345").unwrap(), SpanId::from_hex("54321").unwrap(), TRACE_FLAG_DEBUG | TraceFlags::SAMPLED, @@ -468,7 +478,7 @@ mod tests { ); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Err(()) + None, ); map.clear(); @@ -478,7 +488,7 @@ mod tests { ); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Err(()) + None, ); map.clear(); @@ -488,7 +498,7 @@ mod tests { ); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Err(()) + None, ); map.clear(); @@ -498,7 +508,7 @@ mod tests { ); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Err(()) + None, ); map.clear(); @@ -511,7 +521,7 @@ mod tests { map.set(&too_long_baggage_key, "baggage_value".to_owned()); assert_eq!( propagator_with_custom_header.extract_span_context(&map), - Err(()) + None, ); } diff --git a/opentelemetry-sdk/src/propagation/baggage.rs b/opentelemetry-sdk/src/propagation/baggage.rs index e8e901ec89..100c189e47 100644 --- a/opentelemetry-sdk/src/propagation/baggage.rs +++ b/opentelemetry-sdk/src/propagation/baggage.rs @@ -101,23 +101,31 @@ impl TextMapPropagator for BaggagePropagator { { let mut iter = name_and_value.split('='); if let (Some(name), Some(value)) = (iter.next(), iter.next()) { - let name = percent_decode_str(name).decode_utf8().map_err(|_| ())?; - let value = percent_decode_str(value).decode_utf8().map_err(|_| ())?; + let decode_name = percent_decode_str(name).decode_utf8(); + let decode_value = percent_decode_str(value).decode_utf8(); - // Here we don't store the first ; into baggage since it should be treated - // as separator rather part of metadata - let decoded_props = props - .iter() - .flat_map(|prop| percent_decode_str(prop).decode_utf8()) - .map(|prop| prop.trim().to_string()) - .collect::>() - .join(";"); // join with ; because we deleted all ; when calling split above + if let (Ok(name), Ok(value)) = (decode_name, decode_value) { + // Here we don't store the first ; into baggage since it should be treated + // as separator rather part of metadata + let decoded_props = props + .iter() + .flat_map(|prop| percent_decode_str(prop).decode_utf8()) + .map(|prop| prop.trim().to_string()) + .collect::>() + .join(";"); // join with ; because we deleted all ; when calling split above - Some(KeyValueMetadata::new( - name.trim().to_owned(), - value.trim().to_string(), - decoded_props.as_str(), - )) + Some(KeyValueMetadata::new( + name.trim().to_owned(), + value.trim().to_string(), + decoded_props.as_str(), + )) + } else { + global::handle_error(PropagationError::extract( + "invalid UTF8 string in key values", + "BaggagePropagator", + )); + None + } } else { global::handle_error(PropagationError::extract( "invalid baggage key-value format", diff --git a/opentelemetry-sdk/src/propagation/mod.rs b/opentelemetry-sdk/src/propagation/mod.rs index e7ed4058cd..6cfb5d07bd 100644 --- a/opentelemetry-sdk/src/propagation/mod.rs +++ b/opentelemetry-sdk/src/propagation/mod.rs @@ -3,7 +3,4 @@ mod baggage; mod trace_context; pub use baggage::BaggagePropagator; -use std::fmt::Display; pub use trace_context::TraceContextPropagator; - - diff --git a/opentelemetry/src/global/error_handler.rs b/opentelemetry/src/global/error_handler.rs index 8bcecf465f..87149c1b39 100644 --- a/opentelemetry/src/global/error_handler.rs +++ b/opentelemetry/src/global/error_handler.rs @@ -5,10 +5,10 @@ use std::sync::RwLock; use crate::logs::LogError; #[cfg(feature = "metrics")] use crate::metrics::MetricsError; +use crate::propagation::PropagationError; #[cfg(feature = "trace")] use crate::trace::TraceError; use once_cell::sync::Lazy; -use crate::propagation::PropagationError; static GLOBAL_ERROR_HANDLER: Lazy>> = Lazy::new(|| RwLock::new(None)); @@ -32,8 +32,9 @@ pub enum Error { #[error(transparent)] /// Failed to export logs. Log(#[from] LogError), - + #[error(transparent)] + /// Error happens when injecting and extracting information using propagators. Propagation(#[from] PropagationError), #[error("{0}")] @@ -65,7 +66,9 @@ pub fn handle_error>(err: T) { #[cfg(feature = "logs")] #[cfg_attr(docsrs, doc(cfg(feature = "logs")))] Error::Log(err) => eprintln!("OpenTelemetry log error occurred. {}", err), - Error::Propagation(err) => eprintln!("OpenTelemetry propagation error occurred. {}", err), + Error::Propagation(err) => { + eprintln!("OpenTelemetry propagation error occurred. {}", err) + } Error::Other(err_msg) => eprintln!("OpenTelemetry error occurred. {}", err_msg), }, } diff --git a/opentelemetry/src/propagation/mod.rs b/opentelemetry/src/propagation/mod.rs index 8885c1cffc..535d589d00 100644 --- a/opentelemetry/src/propagation/mod.rs +++ b/opentelemetry/src/propagation/mod.rs @@ -20,7 +20,6 @@ //! [`Context`]: crate::Context use std::collections::HashMap; -use std::fmt::Display; use thiserror::Error; pub mod composite; @@ -74,7 +73,8 @@ pub struct PropagationError { ops: &'static str, } -impl PropagationError{ +impl PropagationError { + /// Error happens when extracting information pub fn extract(message: &'static str, propagator_name: &'static str) -> Self { PropagationError { message, @@ -83,11 +83,12 @@ impl PropagationError{ } } + /// Error happens when extracting information pub fn inject(message: &'static str, propagator_name: &'static str) -> Self { PropagationError { message, propagator_name, - ops: "inject" + ops: "inject", } } } From ecd96e59088dcf793f946d4974ea42f4a8e6bf35 Mon Sep 17 00:00:00 2001 From: Zhongyang Wu Date: Mon, 25 Mar 2024 22:59:42 -0700 Subject: [PATCH 3/3] CHANGELOG --- opentelemetry-jaeger-propagator/CHANGELOG.md | 4 ++++ opentelemetry-sdk/CHANGELOG.md | 1 + opentelemetry/CHANGELOG.md | 1 + 3 files changed, 6 insertions(+) diff --git a/opentelemetry-jaeger-propagator/CHANGELOG.md b/opentelemetry-jaeger-propagator/CHANGELOG.md index 1380f3868d..d907763615 100644 --- a/opentelemetry-jaeger-propagator/CHANGELOG.md +++ b/opentelemetry-jaeger-propagator/CHANGELOG.md @@ -2,6 +2,10 @@ ## vNext +### Changed + +- Propagation error will be reported to global error handler [#1640](https://github.com/open-telemetry/opentelemetry-rust/pull/1640) + ## v0.1.0 ### Added diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index 275cf46761..f72ea62851 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -12,6 +12,7 @@ - **Breaking** [#1624](https://github.com/open-telemetry/opentelemetry-rust/pull/1624) Remove `OsResourceDetector` and `ProcessResourceDetector` resource detectors, use the [`opentelemetry-resource-detector`](https://crates.io/crates/opentelemetry-resource-detectors) instead. +- Baggage propagation error will be reported to global error handler [#1640](https://github.com/open-telemetry/opentelemetry-rust/pull/1640) ## v0.22.1 diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index 235201a7eb..efc2499be0 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -5,6 +5,7 @@ ### Added - [#1623](https://github.com/open-telemetry/opentelemetry-rust/pull/1623) Add global::meter_provider_shutdown +- [#1640](https://github.com/open-telemetry/opentelemetry-rust/pull/1640) Add `PropagationError` ### Removed