-
Notifications
You must be signed in to change notification settings - Fork 94
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
fix(txnames): Avoid replacing url encoded transaction names #1839
Conversation
…ntry/relay into fix/do-not-match-urlencoded-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.
The change looks good, but there are still tests failing.
.to_string(); | ||
if *trans != changed && changed != "*" { | ||
// Sort by the longest match end. | ||
caps.sort_by_key(|(m, _)| m.end()); |
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: Rename m
to match
everywhere?
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.
match
is actually the reserved word 😄. But I also can change it to capture
, since it's basically what it is. What do you think?
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.
done in 85a517d
Modified regex to avoid replacing url encoded strings, this way if there are something like
%[A-F0-9]
in the string, we will skip replacing it, since we do track only the named captures./cc @jan-auer
fix: https://github.com/getsentry/team-ingest/issues/56
#skip-changelog