From 197f19781d9841a7b2865a50fe28af341abd2048 Mon Sep 17 00:00:00 2001 From: tl-rodrigo-gryzinski <73602926+tl-rodrigo-gryzinski@users.noreply.github.com> Date: Thu, 10 Nov 2022 13:21:07 +0000 Subject: [PATCH] Make http.url field opt-in (#70) --- reqwest-tracing/CHANGELOG.md | 3 +- reqwest-tracing/src/lib.rs | 6 +- .../src/reqwest_otel_span_builder.rs | 68 ++++++++++++++++++- .../src/reqwest_otel_span_macro.rs | 7 +- 4 files changed, 76 insertions(+), 8 deletions(-) diff --git a/reqwest-tracing/CHANGELOG.md b/reqwest-tracing/CHANGELOG.md index 834aa11..4afb6f1 100644 --- a/reqwest-tracing/CHANGELOG.md +++ b/reqwest-tracing/CHANGELOG.md @@ -5,7 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] -- HTTP URL is captured in traces as the `http.url` attribute. +### Added +- `SpanBackendWithUrl` for capturing `http.url` in traces - Require an explicit otel name in the macros. Reduces cardinality and complies with otel specification for HTTP bindings. - Permit customisation of the otel name from the non-macro layer. diff --git a/reqwest-tracing/src/lib.rs b/reqwest-tracing/src/lib.rs index a24ad55..2a6b766 100644 --- a/reqwest-tracing/src/lib.rs +++ b/reqwest-tracing/src/lib.rs @@ -96,9 +96,9 @@ mod reqwest_otel_span_builder; pub use middleware::TracingMiddleware; pub use reqwest_otel_span_builder::{ default_on_request_end, default_on_request_failure, default_on_request_success, - DefaultSpanBackend, OtelName, ReqwestOtelSpanBackend, ERROR_CAUSE_CHAIN, ERROR_MESSAGE, - HTTP_HOST, HTTP_METHOD, HTTP_SCHEME, HTTP_STATUS_CODE, HTTP_URL, HTTP_USER_AGENT, - NET_HOST_PORT, OTEL_KIND, OTEL_NAME, OTEL_STATUS_CODE, + DefaultSpanBackend, OtelName, ReqwestOtelSpanBackend, SpanBackendWithUrl, ERROR_CAUSE_CHAIN, + ERROR_MESSAGE, HTTP_HOST, HTTP_METHOD, HTTP_SCHEME, HTTP_STATUS_CODE, HTTP_URL, + HTTP_USER_AGENT, NET_HOST_PORT, OTEL_KIND, OTEL_NAME, OTEL_STATUS_CODE, }; #[doc(hidden)] diff --git a/reqwest-tracing/src/reqwest_otel_span_builder.rs b/reqwest-tracing/src/reqwest_otel_span_builder.rs index 9c3dbdc..095b9b5 100644 --- a/reqwest-tracing/src/reqwest_otel_span_builder.rs +++ b/reqwest-tracing/src/reqwest_otel_span_builder.rs @@ -1,7 +1,7 @@ use std::borrow::Cow; use reqwest::header::{HeaderMap, HeaderValue}; -use reqwest::{Request, Response, StatusCode as RequestStatusCode}; +use reqwest::{Request, Response, StatusCode as RequestStatusCode, Url}; use reqwest_middleware::{Error, Result}; use task_local_extensions::Extensions; use tracing::Span; @@ -88,7 +88,8 @@ pub fn default_on_request_failure(span: &Span, e: &Error) { } } -/// The default [`ReqwestOtelSpanBackend`] for [`TracingMiddleware`]. +/// The default [`ReqwestOtelSpanBackend`] for [`TracingMiddleware`]. Note that it doesn't include +/// the `http.url` field in spans, you can use [`SpanBackendWithUrl`] to add it. /// /// [`TracingMiddleware`]: crate::middleware::TracingMiddleware pub struct DefaultSpanBackend; @@ -112,6 +113,26 @@ fn get_header_value(key: &str, headers: &HeaderMap) -> String { format!("{:?}", headers.get(key).unwrap_or(header_default)).replace('"', "") } +/// Similar to [`DefaultSpanBackend`] but also adds the `http.url` attribute to request spans. +/// +/// [`TracingMiddleware`]: crate::middleware::TracingMiddleware +pub struct SpanBackendWithUrl; + +impl ReqwestOtelSpanBackend for SpanBackendWithUrl { + fn on_request_start(req: &Request, ext: &mut Extensions) -> Span { + let name = ext + .get::() + .map(|on| on.0.as_ref()) + .unwrap_or("reqwest-http-client"); + + reqwest_otel_span!(name = name, req, http.url = %remove_credentials(req.url())) + } + + fn on_request_end(span: &Span, outcome: &Result, _: &mut Extensions) { + default_on_request_end(span, outcome) + } +} + /// HTTP Mapping /// /// Maps the the http status to an Opentelemetry span status following the the specified convention above. @@ -163,6 +184,21 @@ fn get_span_status(request_status: RequestStatusCode) -> Option<&'static str> { #[derive(Clone)] pub struct OtelName(pub Cow<'static, str>); +/// Removes the username and/or password parts of the url, if present. +fn remove_credentials(url: &Url) -> Cow<'_, str> { + if !url.username().is_empty() || url.password().is_some() { + let mut url = url.clone(); + // Errors settings username/password are set when the URL can't have credentials, so + // they're just ignored. + url.set_username("") + .and_then(|_| url.set_password(None)) + .ok(); + url.to_string().into() + } else { + url.as_ref().into() + } +} + #[cfg(test)] mod tests { use super::*; @@ -176,4 +212,32 @@ mod tests { let value = get_header_value("test", &header_map); assert_eq!(value, expect); } + + #[test] + fn remove_credentials_from_url_without_credentials_is_noop() { + let url = "http://nocreds.com/".parse().unwrap(); + let clean = remove_credentials(&url); + assert_eq!(clean, "http://nocreds.com/"); + } + + #[test] + fn remove_credentials_removes_username_only() { + let url = "http://user@withuser.com/".parse().unwrap(); + let clean = remove_credentials(&url); + assert_eq!(clean, "http://withuser.com/"); + } + + #[test] + fn remove_credentials_removes_password_only() { + let url = "http://:123@withpwd.com/".parse().unwrap(); + let clean = remove_credentials(&url); + assert_eq!(clean, "http://withpwd.com/"); + } + + #[test] + fn remove_credentials_removes_username_and_password() { + let url = "http://user:123@both.com/".parse().unwrap(); + let clean = remove_credentials(&url); + assert_eq!(clean, "http://both.com/"); + } } diff --git a/reqwest-tracing/src/reqwest_otel_span_macro.rs b/reqwest-tracing/src/reqwest_otel_span_macro.rs index 8925238..5c1237f 100644 --- a/reqwest-tracing/src/reqwest_otel_span_macro.rs +++ b/reqwest-tracing/src/reqwest_otel_span_macro.rs @@ -52,7 +52,10 @@ /// ``` /// /// If nothing else is specified, the span generated by `reqwest_otel_span!` is identical to the one you'd -/// get by using [`DefaultSpanBackend`]. +/// get by using [`DefaultSpanBackend`]. Note that to avoid leaking sensitive information, the +/// macro doesn't include `http.url`, even though it's required by opentelemetry. You can add the +/// URL attribute explicitly by usng [`SpanBackendWithUrl`] instead of `DefaultSpanBackend` or +/// adding the field on your own implementation. /// /// You can define new fields following the same syntax of [`tracing::info_span!`] for fields: /// @@ -95,6 +98,7 @@ /// /// /// [`DefaultSpanBackend`]: crate::reqwest_otel_span_builder::DefaultSpanBackend +/// [`SpanBackendWithUrl`]: crate::reqwest_otel_span_builder::DefaultSpanBackend /// [`default_on_request_success`]: crate::reqwest_otel_span_builder::default_on_request_success /// [`default_on_request_failure`]: crate::reqwest_otel_span_builder::default_on_request_failure /// [`default_on_request_end`]: crate::reqwest_otel_span_builder::default_on_request_end @@ -129,7 +133,6 @@ macro_rules! reqwest_otel_span { http.method = %method, http.scheme = %scheme, http.host = %host, - http.url = %url, net.host.port = %host_port, otel.kind = "client", otel.name = %otel_name,