Skip to content

Commit

Permalink
fix(spans): Run SQL scrubber even if already parameterized (#2286)
Browse files Browse the repository at this point in the history 
In #2211, we fixed the problem
that noop-scrubbing led to an empty span description:

> In database spans, it's common for SDKs to already send scrubbed
queries, and usually Relay won't do further scrubbing, resulting in the
span description not being present.

But that change had the side effect that queries partially scrubbed on
the client side were not scrubbed any further in relay. With this PR, we
restore best-effort scrubbing on those, for two reasons:

* Catch additional parameters that the SDK did not mark as such.
* Better aggregation by replacing `IN (%s, %s, %s)` (variable list
length) with `IN (%s)`.
  • Loading branch information
@jjbayer
jjbayer authored Jul 6, 2023
1 parent 6070131 commit d3863b4
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 194 deletions.
18 changes: 12 additions & 6 deletions relay-general/src/store/transactions/processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,11 +435,10 @@ fn scrub_identifiers(string: &mut Annotated<String>) -> Result<bool, ProcessingA

/// Normalize the given SQL-query-like string.
fn scrub_sql_queries(string: &mut Annotated<String>) -> Result<bool, ProcessingAction> {
if is_sql_query_scrubbed(string) {
return Ok(true);
}
let mut mark_as_scrubbed = is_sql_query_scrubbed(string);
mark_as_scrubbed |= scrub_identifiers_with_regex(string, &SQL_NORMALIZER_REGEX, "%s")?;

scrub_identifiers_with_regex(string, &SQL_NORMALIZER_REGEX, "%s")
Ok(mark_as_scrubbed)
}

fn scrub_redis_keys(string: &mut Annotated<String>) -> Result<bool, ProcessingAction> {
Expand Down Expand Up @@ -2814,14 +2813,14 @@ mod tests {
span_description_scrub_various_parameterized_ins_percentage,
"SELECT count() FROM table WHERE id IN (%s, %s) AND id IN (%s, %s, %s)",
"db.sql.query",
"SELECT count() FROM table WHERE id IN (%s, %s) AND id IN (%s, %s, %s)"
"SELECT count() FROM table WHERE id IN (%s) AND id IN (%s)"
);

span_description_test!(
span_description_scrub_various_parameterized_ins_dollar,
"SELECT count() FROM table WHERE id IN ($1, $2, $3)",
"db.sql.query",
"SELECT count() FROM table WHERE id IN ($1, $2, $3)"
"SELECT count() FROM table WHERE id IN (%s)"
);

span_description_test!(
Expand All @@ -2845,6 +2844,13 @@ mod tests {
"select count() from table where id in (%s)"
);

span_description_test!(
span_description_scrub_mixed,
"UPDATE foo SET a = %s, b = log(e + 5) * 600 + 12345 WHERE true",
"db.sql.query",
"UPDATE foo SET a = %s, b = log(e + %s) * %s + %s WHERE %s"
);

span_description_test!(
span_description_scrub_savepoint_uppercase,
"SAVEPOINT unquoted_identifier",
Expand Down
14 changes: 0 additions & 14 deletions relay-server/src/metrics_extraction/spans/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -964,20 +964,6 @@ mod tests {
"db.operation": "SELECT"
}
},
{
"description": "SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
"op": "db",
"parent_span_id": "8f5a2b8768cafb4e",
"span_id": "bb7af8b99e95af5f",
"start_timestamp": 1597976300.0000000,
"timestamp": 1597976302.0000000,
"trace_id": "ff62a8b040f340bda5d830223def1d81",
"status": "ok",
"data": {
"db.system": "MyDatabase",
"db.operation": "SELECT"
}
},
{
"description": "SAVEPOINT save_this_one",
"op": "db",
Expand Down
91 changes: 0 additions & 91 deletions ...ans/snapshots/relay_server__metrics_extraction__spans__tests__extract_span_metrics-2.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1371,97 +1371,6 @@ expression: metrics
"transaction.op": "myop",
},
},
Metric {
name: "s:spans/user@none",
value: Set(
933084975,
),
timestamp: UnixTimestamp(1619420400),
tags: {
"environment": "fake_environment",
"http.status_code": "500",
"span.action": "SELECT",
"span.category": "db",
"span.description": "SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
"span.domain": "table",
"span.group": "8ea340ad7f24b50a",
"span.module": "db",
"span.op": "db",
"span.status": "ok",
"span.system": "mydatabase",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
},
Metric {
name: "d:spans/exclusive_time@millisecond",
value: Distribution(
2000.0,
),
timestamp: UnixTimestamp(1619420400),
tags: {
"environment": "fake_environment",
"http.status_code": "500",
"span.action": "SELECT",
"span.category": "db",
"span.description": "SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
"span.domain": "table",
"span.group": "8ea340ad7f24b50a",
"span.module": "db",
"span.op": "db",
"span.status": "ok",
"span.system": "mydatabase",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
},
Metric {
name: "d:spans/exclusive_time_light@millisecond",
value: Distribution(
2000.0,
),
timestamp: UnixTimestamp(1619420400),
tags: {
"environment": "fake_environment",
"http.status_code": "500",
"span.action": "SELECT",
"span.category": "db",
"span.description": "SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
"span.domain": "table",
"span.group": "8ea340ad7f24b50a",
"span.module": "db",
"span.op": "db",
"span.status": "ok",
"span.system": "mydatabase",
"transaction.method": "POST",
"transaction.op": "myop",
},
},
Metric {
name: "d:spans/duration@millisecond",
value: Distribution(
2000.0,
),
timestamp: UnixTimestamp(1619420400),
tags: {
"environment": "fake_environment",
"http.status_code": "500",
"span.action": "SELECT",
"span.category": "db",
"span.description": "SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
"span.domain": "table",
"span.group": "8ea340ad7f24b50a",
"span.module": "db",
"span.op": "db",
"span.status": "ok",
"span.system": "mydatabase",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
},
Metric {
name: "s:spans/user@none",
value: Set(
Expand Down
83 changes: 0 additions & 83 deletions ...spans/snapshots/relay_server__metrics_extraction__spans__tests__extract_span_metrics.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1213,89 +1213,6 @@ expression: event.value().unwrap().spans
},
other: {},
},
Span {
timestamp: Timestamp(
2020-08-21T02:18:22Z,
),
start_timestamp: Timestamp(
2020-08-21T02:18:20Z,
),
exclusive_time: 2000.0,
description: "SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
op: "db",
span_id: SpanId(
"bb7af8b99e95af5f",
),
parent_span_id: SpanId(
"8f5a2b8768cafb4e",
),
trace_id: TraceId(
"ff62a8b040f340bda5d830223def1d81",
),
status: Ok,
tags: ~,
origin: ~,
data: {
"db.operation": String(
"SELECT",
),
"db.system": String(
"MyDatabase",
),
"description.scrubbed": String(
"SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
),
"environment": String(
"fake_environment",
),
"http.status_code": String(
"500",
),
"release": String(
"1.2.3",
),
"span.action": String(
"SELECT",
),
"span.category": String(
"db",
),
"span.description": String(
"SELECT 'TABLE'.'col' FROM 'TABLE' WHERE 'TABLE'.'col' = %s",
),
"span.domain": String(
"table",
),
"span.group": String(
"8ea340ad7f24b50a",
),
"span.module": String(
"db",
),
"span.op": String(
"db",
),
"span.status": String(
"ok",
),
"span.system": String(
"mydatabase",
),
"transaction": String(
"gEt /api/:version/users/",
),
"transaction.method": String(
"POST",
),
"transaction.op": String(
"myop",
),
"user": String(
"id:user123",
),
},
other: {},
},
Span {
timestamp: Timestamp(
2020-08-21T02:18:22Z,
Expand Down

0 comments on commit d3863b4

Please sign in to comment.