Skip to content

Commit

Permalink
fix(spans): Extract http status code from span data when provided as …
Browse files Browse the repository at this point in the history
…int (#3491)

Some SDKs send this field as an integer, which gets converted to `None`
by the `v.as_str()` call.

I added code to catch if the value is an integer and convert it to a
string. In terms of testing, I tested the function which extracts the
status, but also added sample spans to the mobile event to generate a
snapshot validating both string or integers are extracted.

The issue was identified as a difference between Android and iOS events.
  • Loading branch information
narsaynorath authored Apr 29, 2024
1 parent 8c75086 commit cdc7a1f
Show file tree
Hide file tree
Showing 4 changed files with 378 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
**Bug fixes:**

- Respect country code TLDs when scrubbing span tags. ([#3458](https://github.com/getsentry/relay/pull/3458))
- Extract HTTP status code from span data when sent as integers. ([#3491](https://github.com/getsentry/relay/pull/3491))

**Features**:

Expand Down
33 changes: 28 additions & 5 deletions relay-event-normalization/src/normalize/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use std::f64::consts::SQRT_2;

use relay_event_schema::protocol::{Event, ResponseContext, Span, TraceContext, User};
use relay_protocol::Value;

/// Used to decide when to extract mobile-specific tags.
pub const MOBILE_SDKS: [&str; 4] = [
Expand All @@ -31,10 +32,14 @@ pub fn http_status_code_from_span(span: &Span) -> Option<String> {
.data
.value()
.and_then(|data| data.http_response_status_code.value())
.and_then(|v| v.as_str())
.map(|v| v.to_string())
.map(|v| match v {
Value::String(s) => Some(s.as_str().to_owned()),
Value::I64(i) => Some(i.to_string()),
Value::U64(u) => Some(u.to_string()),
_ => None,
})
{
return Some(status_code);
return status_code;
}

// For SDKs which put the HTTP status code into the span tags.
Expand Down Expand Up @@ -194,8 +199,8 @@ pub fn calculate_cdf_score(value: f64, p10: f64, p50: f64) -> f64 {

#[cfg(test)]
mod tests {
use crate::utils::get_event_user_tag;
use relay_event_schema::protocol::User;
use crate::utils::{get_event_user_tag, http_status_code_from_span};
use relay_event_schema::protocol::{Span, User};
use relay_protocol::Annotated;

#[test]
Expand Down Expand Up @@ -241,4 +246,22 @@ mod tests {

assert!(get_event_user_tag(&user).is_none());
}

#[test]
fn test_extracts_http_status_code_when_int() {
let span = Annotated::<Span>::from_json(
r#"{
"data": {
"http.response.status_code": 400
}
}"#,
)
.unwrap()
.into_value()
.unwrap();

let result = http_status_code_from_span(&span);

assert_eq!(result, Some("400".to_string()));
}
}
24 changes: 23 additions & 1 deletion relay-server/src/metrics_extraction/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,28 @@ mod tests {
"start_timestamp": 1597976300.0000000,
"timestamp": 1597976303.0000000,
"trace_id": "ff62a8b040f340bda5d830223def1d81"
},
{
"op": "http.client",
"description": "www.example.com",
"span_id": "bd429c44b67a3eb2",
"start_timestamp": 1597976300.0000000,
"timestamp": 1597976303.0000000,
"trace_id": "ff62a8b040f340bda5d830223def1d81",
"data": {
"http.response.status_code": "200"
}
},
{
"op": "http.client",
"description": "www.example.com",
"span_id": "bd429c44b67a3eb2",
"start_timestamp": 1597976300.0000000,
"timestamp": 1597976303.0000000,
"trace_id": "ff62a8b040f340bda5d830223def1d81",
"data": {
"http.response.status_code": 200
}
}
]
}
Expand Down Expand Up @@ -1373,7 +1395,7 @@ mod tests {
.filter(|b| &*b.name == "c:spans/usage@none")
.collect::<Vec<_>>();

let expected_usage = 10; // We count all spans received by Relay, plus one for the transaction
let expected_usage = 12; // We count all spans received by Relay, plus one for the transaction
assert_eq!(usage_metrics.len(), expected_usage);
for m in usage_metrics {
assert!(m.tags.is_empty());
Expand Down
Loading

0 comments on commit cdc7a1f

Please sign in to comment.