fix: Prefer match_pattern over match_name in actix (#539)

Author: wuerges

sentry-actix/src/lib.rs

@@ -237,12 +237,10 @@ where
             .as_ref()
             .map_or(false, |client| client.options().send_default_pii);
 
-        let (mut tx, sentry_req) = sentry_request_from_http(&req, with_pii);
+        let sentry_req = sentry_request_from_http(&req, with_pii);
+        let name = transaction_name_from_http(&req);
 
         let transaction = if inner.start_transaction {
-            let name = std::mem::take(&mut tx)
-                .unwrap_or_else(|| format!("{} {}", req.method(), req.uri()));
-
             let headers = req.headers().iter().flat_map(|(header, value)| {
                 value.to_str().ok().map(|value| (header.as_str(), value))
             });
@@ -262,7 +260,7 @@ where
             if let Some(transaction) = transaction.as_ref() {
                 scope.set_span(Some(transaction.clone().into()));
             } else {
-                scope.set_transaction(tx.as_deref());
+                scope.set_transaction((!inner.start_transaction).then_some(&name));
             }
             scope.add_event_processor(move |event| Some(process_event(event, &sentry_req)));
             parent_span
@@ -336,14 +334,14 @@ fn map_status(status: StatusCode) -> protocol::SpanStatus {
     }
 }
 
-/// Build a Sentry request struct from the HTTP request
-fn sentry_request_from_http(request: &ServiceRequest, with_pii: bool) -> (Option<String>, Request) {
-    let transaction = if let Some(name) = request.match_name() {
-        Some(String::from(name))
-    } else {
-        request.match_pattern()
-    };
+/// Extract a transaction name from the HTTP request
+fn transaction_name_from_http(req: &ServiceRequest) -> String {
+    let path_part = req.match_pattern().unwrap_or_else(|| "<none>".to_string());
+    format!("{} {}", req.method(), path_part)
+}
 
+/// Build a Sentry request struct from the HTTP request
+fn sentry_request_from_http(request: &ServiceRequest, with_pii: bool) -> Request {
     let mut sentry_req = Request {
         url: format!(
             "{}://{}{}",
@@ -369,7 +367,7 @@ fn sentry_request_from_http(request: &ServiceRequest, with_pii: bool) -> (Option
         }
     };
 
-    (transaction, sentry_req)
+    sentry_req
 }
 
 /// Add request data to a Sentry event
@@ -451,13 +449,56 @@ mod tests {
         assert_eq!(events.len(), 2);
         for event in events {
             let request = event.request.expect("Request should be set.");
-            assert_eq!(event.transaction, Some("/test".into()));
+            assert_eq!(event.transaction, Some("GET /test".into()));
             assert_eq!(event.message, Some("Message".into()));
             assert_eq!(event.level, Level::Warning);
             assert_eq!(request.method, Some("GET".into()));
         }
     }
 
+    /// Test transaction name HTTP verb.
+    #[actix_web::test]
+    async fn test_match_pattern() {
+        let events = sentry::test::with_captured_events(|| {
+            block_on(async {
+                let service = |_name: String| {
+                    // Current Hub should have no events
+                    _assert_hub_no_events();
+
+                    sentry::capture_message("Message", Level::Warning);
+
+                    // Current Hub should have the event
+                    _assert_hub_has_events();
+
+                    HttpResponse::Ok()
+                };
+
+                let app = init_service(
+                    App::new()
+                        .wrap(Sentry::builder().with_hub(Hub::current()).finish())
+                        .service(web::resource("/test/{name}").route(web::post().to(service))),
+                )
+                .await;
+
+                // Call the service twice (sequentially) to ensure the middleware isn't sticky
+                for _ in 0..2 {
+                    let req = TestRequest::post().uri("/test/fake_name").to_request();
+                    let res = call_service(&app, req).await;
+                    assert!(res.status().is_success());
+                }
+            })
+        });
+
+        assert_eq!(events.len(), 2);
+        for event in events {
+            let request = event.request.expect("Request should be set.");
+            assert_eq!(event.transaction, Some("POST /test/{name}".into()));
+            assert_eq!(event.message, Some("Message".into()));
+            assert_eq!(event.level, Level::Warning);
+            assert_eq!(request.method, Some("POST".into()));
+        }
+    }
+
     /// Ensures errors returned in the Actix service trigger an event.
     #[actix_web::test]
     async fn test_response_errors() {
@@ -490,7 +531,7 @@ mod tests {
         assert_eq!(events.len(), 2);
         for event in events {
             let request = event.request.expect("Request should be set.");
-            assert_eq!(event.transaction, Some("failing".into())); // Transaction name is the name of the function
+            assert_eq!(event.transaction, Some("GET /test".into())); // Transaction name is the matcher of the route
             assert_eq!(event.message, None);
             assert_eq!(event.exception.values[0].ty, String::from("Custom"));
             assert_eq!(event.exception.values[0].value, Some("Test Error".into()));