-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat(spans): parameterize redis spans #3306
Conversation
relay-event-normalization/src/normalize/span/description/mod.rs
Outdated
Show resolved
Hide resolved
relay-event-normalization/src/normalize/span/description/mod.rs
Outdated
Show resolved
Hide resolved
// Splits a string on whitespace but ignores spaces between single and double quotes. | ||
// There are edge cases with this implementation for example if the quote was escaped, or if there is a double quote within single quotes. | ||
// This should be fine for auditing purposes | ||
fn split_string_with_quotes(string: &str) -> Vec<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because correctly parsing string literals is hard, I have a suggestion for simplification: What if we always only parametrize the first key, and make everything else "*"
? With something like
let scrubbed_args = if let Some((first, rest)) = args.split_once(" ") {
if first.starts_with('"') || first.starts_with('\'') { // It's a string literal
"*".to_owned()
} else {
let mut result = parameterize_redis_key(first);
result.push_str(" ");
result.push_str(rest);
}
}
you would not need split_string_with_quotes
at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the intention, but i'm hesitant in only parameterizing the first key. The plan is to show metrics for each key namespace, in the scenario where you often perform a command that touches many keys, the metrics would be incorrect.
I am open to trying this out first, as its much simpler and we can audit to see what the data really looks like!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with @jjbayer on this.
You should also not really distinguish between the single and multi argument commands. I'd parse the command then replace everything with a *
and then we can re-evaluate and specialize based on commands perhaps.
Redis has a somewhat easy protocol to understand. If we need to extract more from this, we can look at the definitions of each command and parse it properly. I wouldn't be surprised if there weren't a library doing that already in Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For v0 we are really looking to get some level of key grouping here, so ideally we don't replace everything except the command with a *. I'm okay to just try @jjbayer suggestion and only parametrize the first key.
Co-authored-by: Joris Bayer <[email protected]>
@@ -30,6 +30,12 @@ const MAX_SEGMENT_LENGTH: usize = 25; | |||
/// Some bundlers attach characters to the end of a filename, try to catch those. | |||
const MAX_EXTENSION_LENGTH: usize = 10; | |||
|
|||
const REDIS_COMMAND_SINGLE_KEY: [&str; 13] = [ | |||
"GET", "SET", "DEL", "EXISTS", "HGET", "HSET", "HDEL", "HEXISTS", "HLEN", "APPEND", "EXISTS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot more commands than this in Redis. We should try to parse them in a more generic fashion.
// Splits a string on whitespace but ignores spaces between single and double quotes. | ||
// There are edge cases with this implementation for example if the quote was escaped, or if there is a double quote within single quotes. | ||
// This should be fine for auditing purposes | ||
fn split_string_with_quotes(string: &str) -> Vec<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with @jjbayer on this.
You should also not really distinguish between the single and multi argument commands. I'd parse the command then replace everything with a *
and then we can re-evaluate and specialize based on commands perhaps.
Redis has a somewhat easy protocol to understand. If we need to extract more from this, we can look at the definitions of each command and parse it properly. I wouldn't be surprised if there weren't a library doing that already in Rust.
@DominikB2014 is this PR still relevant? Or can we close it? |
@jjbayer not needed, I'll close it! |
Work towards #cach
First pass to parameterize redis spans, there is still a lot of other commands to consider, and improvements to be made, but if we merge this we can audit the cardinality and real data.
Also in a future PR we will have to tag the scrubbed keys as well.