From 92c5425ceee7f942fae3a0b2235e6889e1238656 Mon Sep 17 00:00:00 2001 From: Devin Date: Fri, 20 May 2022 11:54:23 -0700 Subject: [PATCH] opentelemetry: enforce event_location for span tags (#2124) The `with_event_location(false)` method will now properly omit `code.*` tags from spans. ## Motivation Recently, I attempted to remove the `code.*` tags from opentelemetry tracing spans utilizing the [`with_event_location`] of OpenTelemetrySubscriber. This did not work as expected because the [`on_new_span`] doesn't account for the `event_location` boolean similar to how [`on_event`] does. ## Solution The change presented will expand the use of the `location` field to check before adding the `code.*` KeyValue attributes in `on_new_span`. In addition, `with_event_location` was renamed to `with_location`, as it now controls both span *and* event locations, and the `with_event_location` method has been deprecated. ## Testing Additional unit tests are included in [tracing-opentelemetry/src/subscriber.rs] to cover both boolean cases of `with_location`. [`with_event_location`]: https://github.com/tokio-rs/tracing/blob/master/tracing-opentelemetry/src/subscriber.rs#L343 [`on_new_span`]: https://github.com/tokio-rs/tracing/blob/master/tracing-opentelemetry/src/subscriber.rs#L448-L460 [`on_event`]: https://github.com/tokio-rs/tracing/blob/master/tracing-opentelemetry/src/subscriber.rs#L582 [tracing-opentelemetry/src/subscriber.rs]: https://github.com/tokio-rs/tracing/pull/2124/files#diff-69011e8b23dffcbe19b9b72e5ac54330a7871f424a90700ed7f5c5686daf431bR911-R975) --- tracing-opentelemetry/src/layer.rs | 102 +++++++++++++++++++++++++---- 1 file changed, 88 insertions(+), 14 deletions(-) diff --git a/tracing-opentelemetry/src/layer.rs b/tracing-opentelemetry/src/layer.rs index 722d7ce804..9f2da0b601 100644 --- a/tracing-opentelemetry/src/layer.rs +++ b/tracing-opentelemetry/src/layer.rs @@ -28,7 +28,7 @@ const SPAN_STATUS_MESSAGE_FIELD: &str = "otel.status_message"; /// [tracing]: https://github.com/tokio-rs/tracing pub struct OpenTelemetryLayer { tracer: T, - event_location: bool, + location: bool, tracked_inactivity: bool, get_context: WithContext, _registry: marker::PhantomData, @@ -312,7 +312,7 @@ where pub fn new(tracer: T) -> Self { OpenTelemetryLayer { tracer, - event_location: true, + location: true, tracked_inactivity: true, get_context: WithContext(Self::get_context), _registry: marker::PhantomData, @@ -351,20 +351,32 @@ where { OpenTelemetryLayer { tracer, - event_location: self.event_location, + location: self.location, tracked_inactivity: self.tracked_inactivity, get_context: WithContext(OpenTelemetryLayer::::get_context), _registry: self._registry, } } + /// Sets whether or not span and event metadata should include detailed + /// location information, such as the file, module and line number. + /// + /// By default, locations are enabled. + pub fn with_location(self, location: bool) -> Self { + Self { location, ..self } + } + /// Sets whether or not event span's metadata should include detailed location /// information, such as the file, module and line number. /// /// By default, event locations are enabled. + #[deprecated( + since = "0.17.3", + note = "renamed to `OpenTelemetrySubscriber::with_location`" + )] pub fn with_event_location(self, event_location: bool) -> Self { Self { - event_location, + location: event_location, ..self } } @@ -467,18 +479,20 @@ where .attributes .get_or_insert(Vec::with_capacity(attrs.fields().len() + 3)); - let meta = attrs.metadata(); + if self.location { + let meta = attrs.metadata(); - if let Some(filename) = meta.file() { - builder_attrs.push(KeyValue::new("code.filepath", filename)); - } + if let Some(filename) = meta.file() { + builder_attrs.push(KeyValue::new("code.filepath", filename)); + } - if let Some(module) = meta.module_path() { - builder_attrs.push(KeyValue::new("code.namespace", module)); - } + if let Some(module) = meta.module_path() { + builder_attrs.push(KeyValue::new("code.namespace", module)); + } - if let Some(line) = meta.line() { - builder_attrs.push(KeyValue::new("code.lineno", line as i64)); + if let Some(line) = meta.line() { + builder_attrs.push(KeyValue::new("code.lineno", line as i64)); + } } attrs.record(&mut SpanAttributeVisitor(&mut builder)); @@ -601,7 +615,7 @@ where builder.status_code = Some(otel::StatusCode::Error); } - if self.event_location { + if self.location { #[cfg(not(feature = "tracing-log"))] let normalized_meta: Option> = None; let (file, module) = match &normalized_meta { @@ -999,4 +1013,64 @@ mod tests { ) ); } + + #[test] + fn includes_span_location() { + let tracer = TestTracer(Arc::new(Mutex::new(None))); + let subscriber = tracing_subscriber::registry() + .with(layer().with_tracer(tracer.clone()).with_location(true)); + + tracing::subscriber::with_default(subscriber, || { + tracing::debug_span!("request"); + }); + + let attributes = tracer + .0 + .lock() + .unwrap() + .as_ref() + .unwrap() + .builder + .attributes + .as_ref() + .unwrap() + .clone(); + let keys = attributes + .iter() + .map(|attr| attr.key.as_str()) + .collect::>(); + assert!(keys.contains(&"code.filepath")); + assert!(keys.contains(&"code.namespace")); + assert!(keys.contains(&"code.lineno")); + } + + #[test] + fn excludes_span_location() { + let tracer = TestTracer(Arc::new(Mutex::new(None))); + let subscriber = tracing_subscriber::registry() + .with(layer().with_tracer(tracer.clone()).with_location(false)); + + tracing::subscriber::with_default(subscriber, || { + tracing::debug_span!("request"); + }); + + let attributes = tracer + .0 + .lock() + .unwrap() + .as_ref() + .unwrap() + .builder + .attributes + .as_ref() + .unwrap() + .clone(); + let keys = attributes + .iter() + .map(|attr| attr.key.as_str()) + .collect::>(); + assert!(!keys.contains(&"code.filepath")); + assert!(!keys.contains(&"code.namespace")); + assert!(!keys.contains(&"code.lineno")); + } }