Skip to content

Commit

Permalink
fix(spans): Support lowercase redis commands (#4235)
Browse files Browse the repository at this point in the history
Not all redis integrations send uppercase redis commands, which results
in redis spans being scrubbed as *.

This PR removes the redis regex and introduces a list of redis command
that can be prefix-searched.
  • Loading branch information
jjbayer authored Nov 12, 2024
1 parent 4a956e6 commit 1c1e4de
Show file tree
Hide file tree
Showing 5 changed files with 673 additions and 27 deletions.
7 changes: 4 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,19 @@
- Removes support for metric meta envelope items. ([#4152](https://github.com/getsentry/relay/pull/4152))
- Removes support for the project cache endpoint version 2 and before. ([#4147](https://github.com/getsentry/relay/pull/4147))

**Bug Fixes**
**Bug Fixes**:

- Allow profile chunks without release. ([#4155](https://github.com/getsentry/relay/pull/4155))

**Features:**
**Features**:

- Add check to ensure unreal user info is not empty. ([#4225](https://github.com/getsentry/relay/pull/4225))
- Retain empty string values in `span.data` and `event.contexts.trace.data`. ([#4174](https://github.com/getsentry/relay/pull/4174))
- Allow `sample_rate` to be float type when deserializing `DynamicSamplingContext`. ([#4181](https://github.com/getsentry/relay/pull/4181))
- Support inbound filters for profiles. ([#4176](https://github.com/getsentry/relay/pull/4176))
- Scrub lower-case redis commands. ([#4235](https://github.com/getsentry/relay/pull/4235))

**Internal:**
**Internal**:

- Add a metric that counts span volume in the root project for dynamic sampling (`c:spans/count_per_root_project@none`). ([#4134](https://github.com/getsentry/relay/pull/4134))
- Add a tag `target_project_id` to both root project metrics for dynamic sampling (`c:transactions/count_per_root_project@none` and `c:spans/count_per_root_project@none`) which shows the flow trace traffic from root to target projects. ([#4170](https://github.com/getsentry/relay/pull/4170))
Expand Down
1 change: 1 addition & 0 deletions relay-config/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2592,6 +2592,7 @@ impl Default for Config {

#[cfg(test)]
mod tests {

use super::*;

/// Regression test for renaming the envelope buffer flags.
Expand Down
78 changes: 59 additions & 19 deletions relay-event-normalization/src/normalize/span/description/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//! Span description scrubbing logic.
mod redis;
mod resource;
mod sql;
use once_cell::sync::Lazy;
Expand All @@ -16,8 +17,9 @@ use url::{Host, Url};

use crate::regexes::{
DB_SQL_TRANSACTION_CORE_DATA_REGEX, DB_SUPABASE_REGEX, FUNCTION_NORMALIZER_REGEX,
REDIS_COMMAND_REGEX, RESOURCE_NORMALIZER_REGEX,
RESOURCE_NORMALIZER_REGEX,
};
use crate::span::description::redis::matching_redis_command;
use crate::span::description::resource::COMMON_PATH_SEGMENTS;
use crate::span::tag_extraction::HTTP_METHOD_EXTRACTOR_REGEX;
use crate::span::TABLE_NAME_REGEX;
Expand Down Expand Up @@ -384,15 +386,20 @@ pub fn concatenate_host_and_port(host: Option<&str>, port: Option<u16>) -> Cow<s
}

fn scrub_redis_keys(string: &str) -> Option<String> {
let parts = REDIS_COMMAND_REGEX
.captures(string)
.map(|caps| (caps.name("command"), caps.name("args")));
let scrubbed = match parts {
Some((Some(command), Some(_args))) => command.as_str().to_owned() + " *",
Some((Some(command), None)) => command.as_str().into(),
None | Some((None, _)) => "*".into(),
};
Some(scrubbed)
let string = string.trim();
Some(match matching_redis_command(string) {
Some(command) => {
let mut command = command.to_uppercase();
match string.get(command.len()..) {
None | Some("") => command,
Some(_other) => {
command.push_str(" *");
command
}
}
}
None => "*".to_owned(),
})
}

enum UrlType {
Expand Down Expand Up @@ -671,6 +678,13 @@ mod tests {
};
}

macro_rules! span_description_test_with_lowercase {
($name:ident, $name2:ident, $description_in:expr, $op_in:literal, $expected:literal) => {
span_description_test!($name, $description_in, $op_in, $expected);
span_description_test!($name2, ($description_in).to_lowercase(), $op_in, $expected);
};
}

span_description_test!(empty, "", "http.client", "");

span_description_test!(
Expand Down Expand Up @@ -792,30 +806,57 @@ mod tests {
""
);

span_description_test!(
span_description_test_with_lowercase!(
cache,
cache_lower,
"GET abc:12:{def}:{34}:{fg56}:EAB38:zookeeper",
"cache.get_item",
"GET *"
);

span_description_test!(redis_set, "SET mykey myvalue", "db.redis", "SET *");
span_description_test_with_lowercase!(
redis_set,
redis_set_lower,
"SET mykey myvalue",
"db.redis",
"SET *"
);

span_description_test!(
span_description_test_with_lowercase!(
redis_set_quoted,
redis_set_quoted_lower,
r#"SET mykey 'multi: part, value'"#,
"db.redis",
"SET *"
);

span_description_test!(redis_whitespace, " GET asdf:123", "db.redis", "GET *");
span_description_test_with_lowercase!(
redis_whitespace,
redis_whitespace_lower,
" GET asdf:123",
"db.redis",
"GET *"
);

span_description_test!(redis_no_args, "EXEC", "db.redis", "EXEC");
span_description_test_with_lowercase!(
redis_no_args,
redis_no_args_lower,
"EXEC",
"db.redis",
"EXEC"
);

span_description_test!(redis_invalid, "What a beautiful day!", "db.redis", "*");
span_description_test_with_lowercase!(
redis_invalid,
redis_invalid_lower,
"What a beautiful day!",
"db.redis",
"*"
);

span_description_test!(
span_description_test_with_lowercase!(
redis_long_command,
redis_long_command_lower,
"ACL SETUSER jane",
"db.redis",
"ACL SETUSER *"
Expand Down Expand Up @@ -1282,8 +1323,7 @@ mod tests {
ScrubMongoDescription::Disabled,
);

// NOTE: this should return `DEL *`, but we cannot detect lowercase command names yet.
assert_eq!(scrubbed.0.as_deref(), Some("*"));
assert_eq!(scrubbed.0.as_deref(), Some("DEL *"));
}

#[test]
Expand Down
Loading

0 comments on commit 1c1e4de

Please sign in to comment.