-
Notifications
You must be signed in to change notification settings - Fork 87
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(transaction): Normalize transaction name #1621
Conversation
Adding feature which, when enabled, supports transaction names normalization by replacing UUIDs, SHAs and numerical IDs in transaction names by placeholders. This is the first step which should help to prevent high cardinality for transaction names.
relay-general/src/store/normalize.rs
Outdated
@@ -663,6 +663,7 @@ pub struct LightNormalizationConfig<'a> { | |||
pub measurements_config: Option<&'a MeasurementsConfig>, | |||
pub breakdowns_config: Option<&'a BreakdownsConfig>, | |||
pub normalize_user_agent: Option<bool>, | |||
pub normalize_transaction_name: Option<bool>, |
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.
nit: Does this need to be an Option<bool>
? I don't know why normalize_user_agent
is one to be honest, I feel like a simple bool
would do just fine.
// ranges will change. | ||
for matches in TRANSACTION_NAME_NORMALIZER_REGEX.captures_iter(trans) { | ||
// Skip first, since it's always the entire match and the following are groups. | ||
// And if there is no matches, just cotinue to next iteration. |
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.
// And if there is no matches, just cotinue to next iteration. | |
// And if there is no matches, just continue to next iteration. |
for name in capture_names { | ||
if let Some(caps) = TRANSACTION_NAME_NORMALIZER_REGEX.captures(trans) { |
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 name in capture_names { | |
if let Some(caps) = TRANSACTION_NAME_NORMALIZER_REGEX.captures(trans) { | |
if let Some(caps) = TRANSACTION_NAME_NORMALIZER_REGEX.captures(trans) { | |
for name in capture_names { |
if meta.original_value().is_none() { | ||
meta.set_original_value(Some(trans.to_string())) | ||
} | ||
let mut collector = String::new(); |
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.
It should be possible to allocate a new String only once for each transaction name. Assuming that capture_names
are ordered , we can start writing [0..m1.start()]
, then [m1.end()...m2.start()]
, etc.
*trans = collector; | ||
} | ||
} | ||
} |
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.
Is there no way we can unify the two for loops in this function?
[ | ||
"normalize_transaction_name", | ||
"s", | ||
51, | ||
54 | ||
] | ||
], |
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.
Nice!
relay-server/src/actors/project.rs
Outdated
@@ -71,6 +71,9 @@ pub enum Feature { | |||
/// Enables ingestion of Session Replays (Replay Recordings and Replay Events) | |||
#[serde(rename = "organizations:session-replay")] | |||
Replays, | |||
/// docs |
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.
Don't forget the docs :)
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 knew you will see it! 😄
… names (#41642) See relay side in getsentry/relay#1621 where the new feature will be consumed. Once enabled, we will try to normalize the transaction names by replacing UUID, SHAs, numerical IDs with placeholders. ref: getsentry/relay#1597
* master: chore(toolchain): Fix lint and test for beta toolchain (#1623) ref(system): Introduce a generic recipient type (#1622) feat(transaction): Normalize transaction name (#1621) feat(server): Track metrics for OpenTelemetry events (#1618) bug(replays): Increase reserved space in payload chunks (#1601) feat(protocol): Add OpenTelemetry Context (#1617)
Literal asterisks in transaction names (such as created by getsentry/relay#1621) should not be confused with the wildcard `*`, which for transaction name rules means "match and replace".
Adding feature which, when enabled, supports transaction names normalization by replacing UUIDs, SHAs and numerical IDs in transaction names by placeholders.
This is the first step which should help to prevent high cardinality for transaction names.
ref: #1597