From 6a479ff2b87cbbf98f06bdbaf6466b72d69c155e Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 3 Jan 2022 16:16:31 +0100 Subject: [PATCH 1/6] feat: Add a Http Service integration with trace propagation (NATIVE-314) This creates a new sentry-tower feature that does trace propagation for Http Services. --- sentry-tower/Cargo.toml | 4 ++ sentry-tower/src/http.rs | 104 +++++++++++++++++++++++++++++++++++++++ sentry-tower/src/lib.rs | 8 ++- 3 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 sentry-tower/src/http.rs diff --git a/sentry-tower/Cargo.toml b/sentry-tower/Cargo.toml index 35c5d831d..b6a821aee 100644 --- a/sentry-tower/Cargo.toml +++ b/sentry-tower/Cargo.toml @@ -11,9 +11,13 @@ Sentry integration for tower-based crates. """ edition = "2018" +[features] +http = [] + [dependencies] tower-layer = "0.3" tower-service = "0.3" +http = { version = "0.2.6", optional = true } sentry-core = { version = "0.23.0", path = "../sentry-core", default-features = false, features = ["client"] } [dev-dependencies] diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs new file mode 100644 index 000000000..e7257601e --- /dev/null +++ b/sentry-tower/src/http.rs @@ -0,0 +1,104 @@ +use std::collections::BTreeMap; +use std::future::Future; +use std::pin::Pin; +use std::task::{Context, Poll}; + +use tower_layer::Layer; +use tower_service::Service; + +#[derive(Clone)] +pub struct SentryHttpLayer; + +#[derive(Clone)] +pub struct SentryHttpService { + service: S, +} + +impl Layer for SentryHttpLayer { + type Service = SentryHttpService; + + fn layer(&self, service: S) -> Self::Service { + Self::Service { service } + } +} + +pub struct SentryHttpFuture { + transaction: Option, + future: F, +} + +impl Future for SentryHttpFuture +where + F: Future, +{ + type Output = F::Output; + + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + // https://doc.rust-lang.org/std/pin/index.html#pinning-is-structural-for-field + let future = unsafe { self.map_unchecked_mut(|s| &mut s.future) }; + match future.poll(cx) { + Poll::Ready(res) => { + if let Some(transaction) = self.transaction.take() { + transaction.finish(); + } + Poll::Ready(res) + } + Poll::Pending => Poll::Pending, + } + } +} + +impl Service> for SentryHttpService +where + S: Service>, +{ + type Response = S::Response; + type Error = S::Error; + type Future = SentryHttpFuture; + + fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + self.service.poll_ready(cx) + } + + fn call(&mut self, request: Request) -> Self::Future { + let transaction = sentry::configure_scope(|scope| { + // https://develop.sentry.dev/sdk/event-payloads/request/ + let mut ctx = BTreeMap::new(); + + // TODO: method, url, query_string + + // headers + let mut headers = BTreeMap::new(); + for (header, value) in request.headers() { + headers.insert( + header.to_string(), + value.to_str().unwrap_or("").into(), + ); + } + ctx.insert("headers".into(), headers); + + scope.set_context("request", sentry_core::protocol::Context::Other(ctx)); + + // TODO: maybe make transaction creation optional? + let transaction = if true { + let tx_ctx = sentry_core::TransactionContext::continue_from_headers( + // TODO: whats the name here? + "", + "http", + request.headers(), + ); + Some(sentry_core::start_transaction(tx_ctx)) + } else { + None + }; + + scope.set_span(transaction.clone()); + transaction + }); + + SentryHttpFuture { + transaction, + future: self.service.call(request), + } + } +} diff --git a/sentry-tower/src/lib.rs b/sentry-tower/src/lib.rs index eb51dfa2d..2a5cee71d 100644 --- a/sentry-tower/src/lib.rs +++ b/sentry-tower/src/lib.rs @@ -102,13 +102,17 @@ #![doc(html_logo_url = "https://sentry-brand.storage.googleapis.com/sentry-glyph-black.png")] #![warn(missing_docs)] -use sentry_core::{Hub, SentryFuture, SentryFutureExt}; use std::marker::PhantomData; use std::sync::Arc; use std::task::{Context, Poll}; + +use sentry_core::{Hub, SentryFuture, SentryFutureExt}; use tower_layer::Layer; use tower_service::Service; +#[cfg(feature = "http")] +mod http; + /// Provides a hub for each request pub trait HubProvider where @@ -140,7 +144,7 @@ pub struct NewFromTopProvider; impl HubProvider, Request> for NewFromTopProvider { fn hub(&self, _request: &Request) -> Arc { - // The Clippy lint here is a falste positive, the suggestion to write + // The Clippy lint here is a false positive, the suggestion to write // `Hub::with(Hub::new_from_top)` does not compiles: // 143 | Hub::with(Hub::new_from_top).into() // | ^^^^^^^^^ implementation of `std::ops::FnOnce` is not general enough From 80ed38283bfc6a502cda1ec91f2ab4435b2537df Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Jan 2022 12:41:32 +0100 Subject: [PATCH 2/6] make the tower-http code actually work --- sentry-tower/Cargo.toml | 5 +- sentry-tower/src/http.rs | 104 ++++++++++++++++++++++---------- sentry-tower/src/lib.rs | 2 + sentry-types/src/protocol/v7.rs | 2 +- 4 files changed, 79 insertions(+), 34 deletions(-) diff --git a/sentry-tower/Cargo.toml b/sentry-tower/Cargo.toml index b6a821aee..733e0a6a0 100644 --- a/sentry-tower/Cargo.toml +++ b/sentry-tower/Cargo.toml @@ -12,12 +12,13 @@ Sentry integration for tower-based crates. edition = "2018" [features] -http = [] +http = ["http_", "pin-project"] [dependencies] tower-layer = "0.3" tower-service = "0.3" -http = { version = "0.2.6", optional = true } +http_ = { package = "http", version = "0.2.6", optional = true } +pin-project = { version = "1.0.10", optional = true } sentry-core = { version = "0.23.0", path = "../sentry-core", default-features = false, features = ["client"] } [dev-dependencies] diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index e7257601e..60018af1f 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -1,45 +1,82 @@ -use std::collections::BTreeMap; use std::future::Future; +use std::iter::FromIterator; use std::pin::Pin; use std::task::{Context, Poll}; +use http_::Request; +use sentry_core::protocol::value::Map as ValueMap; +use sentry_core::protocol::{Map as SentryMap, Value}; use tower_layer::Layer; use tower_service::Service; -#[derive(Clone)] -pub struct SentryHttpLayer; +/// Tower Layer that logs Http Request Headers. +/// +/// The Layer can also optionally start a new performance monitoring transaction +/// based on incoming distributed tracing headers. +#[derive(Clone, Default)] +pub struct SentryHttpLayer { + start_transaction: bool, +} + +impl SentryHttpLayer { + /// Creates a new Layer that only logs Request Headers. + pub fn new() -> Self { + Self::default() + } + /// Creates a new Layer that also starts a new performance monitoring transaction. + pub fn with_transaction() -> Self { + Self { + start_transaction: true, + } + } +} + +/// Tower Service that logs Http Request Headers. +/// +/// The Service can also optionally start a new performance monitoring transaction +/// based on incoming distributed tracing headers. #[derive(Clone)] pub struct SentryHttpService { service: S, + start_transaction: bool, } impl Layer for SentryHttpLayer { type Service = SentryHttpService; fn layer(&self, service: S) -> Self::Service { - Self::Service { service } + Self::Service { + service, + start_transaction: self.start_transaction, + } } } +/// The Future returned from [`SentryHttpService`] +#[pin_project::pin_project] pub struct SentryHttpFuture { - transaction: Option, + transaction: Option<( + sentry_core::TransactionOrSpan, + Option, + )>, + #[pin] future: F, } -impl Future for SentryHttpFuture +impl Future for SentryHttpFuture where F: Future, { type Output = F::Output; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - // https://doc.rust-lang.org/std/pin/index.html#pinning-is-structural-for-field - let future = unsafe { self.map_unchecked_mut(|s| &mut s.future) }; - match future.poll(cx) { + let slf = self.project(); + match slf.future.poll(cx) { Poll::Ready(res) => { - if let Some(transaction) = self.transaction.take() { + if let Some((transaction, parent_span)) = slf.transaction.take() { transaction.finish(); + sentry_core::configure_scope(|scope| scope.set_span(parent_span)); } Poll::Ready(res) } @@ -61,39 +98,44 @@ where } fn call(&mut self, request: Request) -> Self::Future { - let transaction = sentry::configure_scope(|scope| { + let transaction = sentry_core::configure_scope(|scope| { // https://develop.sentry.dev/sdk/event-payloads/request/ - let mut ctx = BTreeMap::new(); + let mut ctx = SentryMap::new(); - // TODO: method, url, query_string + ctx.insert( + "method".into(), + Value::String(format!("{}", request.method())), + ); + ctx.insert("url".into(), Value::String(format!("{}", request.uri()))); // headers - let mut headers = BTreeMap::new(); - for (header, value) in request.headers() { - headers.insert( - header.to_string(), - value.to_str().unwrap_or("").into(), - ); - } - ctx.insert("headers".into(), headers); + let headers = + ValueMap::from_iter(request.headers().into_iter().map(|(header, value)| { + ( + header.to_string(), + value.to_str().unwrap_or("").into(), + ) + })); + ctx.insert("headers".into(), headers.into()); scope.set_context("request", sentry_core::protocol::Context::Other(ctx)); - // TODO: maybe make transaction creation optional? - let transaction = if true { + if self.start_transaction { + let headers = request.headers().into_iter().flat_map(|(header, value)| { + value.to_str().ok().map(|value| (header.as_str(), value)) + }); let tx_ctx = sentry_core::TransactionContext::continue_from_headers( // TODO: whats the name here? - "", - "http", - request.headers(), + "", "http", headers, ); - Some(sentry_core::start_transaction(tx_ctx)) + let transaction: sentry_core::TransactionOrSpan = + sentry_core::start_transaction(tx_ctx).into(); + let parent_span = scope.get_span(); + scope.set_span(Some(transaction.clone())); + Some((transaction, parent_span)) } else { None - }; - - scope.set_span(transaction.clone()); - transaction + } }); SentryHttpFuture { diff --git a/sentry-tower/src/lib.rs b/sentry-tower/src/lib.rs index 2a5cee71d..3dba2023b 100644 --- a/sentry-tower/src/lib.rs +++ b/sentry-tower/src/lib.rs @@ -112,6 +112,8 @@ use tower_service::Service; #[cfg(feature = "http")] mod http; +#[cfg(feature = "http")] +pub use http::*; /// Provides a hub for each request pub trait HubProvider diff --git a/sentry-types/src/protocol/v7.rs b/sentry-types/src/protocol/v7.rs index 32d51f35a..f64602096 100644 --- a/sentry-types/src/protocol/v7.rs +++ b/sentry-types/src/protocol/v7.rs @@ -46,7 +46,7 @@ pub mod debugid { /// An arbitrary (JSON) value. pub use self::value::Value; -/// The internally useed map type. +/// The internally used map type. pub use self::map::Map; /// A wrapper type for collections with attached meta data. From ee4ac4600770552e78481d243b98895ef04cdddf Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Jan 2022 13:04:13 +0100 Subject: [PATCH 3/6] use a proper request obj and an event processor --- sentry-tower/src/http.rs | 41 ++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index 60018af1f..539061247 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -99,26 +99,27 @@ where fn call(&mut self, request: Request) -> Self::Future { let transaction = sentry_core::configure_scope(|scope| { - // https://develop.sentry.dev/sdk/event-payloads/request/ - let mut ctx = SentryMap::new(); - - ctx.insert( - "method".into(), - Value::String(format!("{}", request.method())), - ); - ctx.insert("url".into(), Value::String(format!("{}", request.uri()))); - - // headers - let headers = - ValueMap::from_iter(request.headers().into_iter().map(|(header, value)| { - ( - header.to_string(), - value.to_str().unwrap_or("").into(), - ) - })); - ctx.insert("headers".into(), headers.into()); - - scope.set_context("request", sentry_core::protocol::Context::Other(ctx)); + let sentry_req = sentry_core::protocol::Request { + method: Some(request.method().to_string()), + url: request.uri().to_string().parse().ok(), + headers: request + .headers() + .into_iter() + .map(|(header, value)| { + ( + header.to_string(), + value.to_str().unwrap_or_default().into(), + ) + }) + .collect(), + ..Default::default() + }; + scope.add_event_processor(move |mut event| { + if event.request.is_none() { + event.request = Some(sentry_req.clone()); + } + Some(event) + }); if self.start_transaction { let headers = request.headers().into_iter().flat_map(|(header, value)| { From 648c4f4bc1cd4c2f28f3cbccab7d6fae2e016c3e Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Jan 2022 13:12:26 +0100 Subject: [PATCH 4/6] rm unused imports --- sentry-tower/src/http.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index 539061247..55c6c5972 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -1,11 +1,8 @@ use std::future::Future; -use std::iter::FromIterator; use std::pin::Pin; use std::task::{Context, Poll}; use http_::Request; -use sentry_core::protocol::value::Map as ValueMap; -use sentry_core::protocol::{Map as SentryMap, Value}; use tower_layer::Layer; use tower_service::Service; From e3d9755497976c868e16d35f1c150d6389fc602c Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 10 Jan 2022 16:27:12 +0100 Subject: [PATCH 5/6] add a transaction name --- sentry-tower/src/http.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index 55c6c5972..44f8a24c9 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -122,9 +122,11 @@ where let headers = request.headers().into_iter().flat_map(|(header, value)| { value.to_str().ok().map(|value| (header.as_str(), value)) }); + let tx_name = format!("{} {}", request.method(), request.uri().path()); let tx_ctx = sentry_core::TransactionContext::continue_from_headers( - // TODO: whats the name here? - "", "http", headers, + &tx_name, + "http.server", + headers, ); let transaction: sentry_core::TransactionOrSpan = sentry_core::start_transaction(tx_ctx).into(); From 8c79a3225b9ffa6fa4f25561b9e0b06eaefcdfba Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Tue, 11 Jan 2022 12:54:49 +0100 Subject: [PATCH 6/6] improve wording --- sentry-tower/src/http.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/sentry-tower/src/http.rs b/sentry-tower/src/http.rs index 44f8a24c9..1db10150c 100644 --- a/sentry-tower/src/http.rs +++ b/sentry-tower/src/http.rs @@ -8,8 +8,9 @@ use tower_service::Service; /// Tower Layer that logs Http Request Headers. /// -/// The Layer can also optionally start a new performance monitoring transaction -/// based on incoming distributed tracing headers. +/// The Service created by this Layer can also optionally start a new +/// performance monitoring transaction for each incoming request, +/// continuing the trace based on incoming distributed tracing headers. #[derive(Clone, Default)] pub struct SentryHttpLayer { start_transaction: bool, @@ -21,7 +22,8 @@ impl SentryHttpLayer { Self::default() } - /// Creates a new Layer that also starts a new performance monitoring transaction. + /// Creates a new Layer which starts a new performance monitoring transaction + /// for each incoming request. pub fn with_transaction() -> Self { Self { start_transaction: true, @@ -32,7 +34,8 @@ impl SentryHttpLayer { /// Tower Service that logs Http Request Headers. /// /// The Service can also optionally start a new performance monitoring transaction -/// based on incoming distributed tracing headers. +/// for each incoming request, continuing the trace based on incoming +/// distributed tracing headers. #[derive(Clone)] pub struct SentryHttpService { service: S, @@ -50,7 +53,7 @@ impl Layer for SentryHttpLayer { } } -/// The Future returned from [`SentryHttpService`] +/// The Future returned from [`SentryHttpService`]. #[pin_project::pin_project] pub struct SentryHttpFuture { transaction: Option<(