Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix actix match name bug #539

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 56 additions & 15 deletions sentry-actix/src/lib.rs
Expand Up @@ -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))
});
Expand All @@ -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
Expand Down Expand Up @@ -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!(
"{}://{}{}",
Expand All @@ -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
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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()));
Expand Down