-
Notifications
You must be signed in to change notification settings - Fork 91
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(transactions): Adding support for transaction (re)naming rules #1695
Changes from 7 commits
d52fc21
49ef92b
17c4ded
8b27e09
b90a8ba
967f2ed
5a5b1ca
53d59a6
642833c
ffc24c2
180e930
689a08b
0a6af11
fc1a2f9
596f3db
8199c5c
33fed75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -6,45 +6,110 @@ use regex::Regex; | |||||||
|
||||||||
use crate::macros::impl_str_serde; | ||||||||
|
||||||||
/// A simple glob matcher. | ||||||||
/// | ||||||||
/// Supported are `?` for a single char, `*` for all but a slash and | ||||||||
/// `**` to match with slashes. | ||||||||
#[derive(Clone)] | ||||||||
pub struct Glob { | ||||||||
value: String, | ||||||||
pattern: Regex, | ||||||||
/// Glob options is used to configure the behaviour underlying regex. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this sentence difficult to understand, and without the example below I've not been able to. I suggest the following but feel free to modify it to a different one.
Suggested change
|
||||||||
#[derive(Debug)] | ||||||||
struct GlobPatternOpts<'g> { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would like to give this struct a more descriptive name but I don't have one. Maybe something like |
||||||||
star: &'g str, | ||||||||
double_star: &'g str, | ||||||||
question_mark: &'g str, | ||||||||
} | ||||||||
|
||||||||
impl Glob { | ||||||||
/// Creates a new glob from a string. | ||||||||
pub fn new(glob: &str) -> Glob { | ||||||||
let mut pattern = String::with_capacity(glob.len() + 100); | ||||||||
/// `GlobBuilder` provides the posibility to fine tune the final [`Glob`], mainly what capture | ||||||||
/// groups will be enabled in the underlying regex. | ||||||||
#[derive(Debug)] | ||||||||
pub struct GlobBuilder<'g> { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe all this glob builder logic belongs to a different PR -- the complexity is enough to be on its own; it's easy to make mistakes when regexes, globs and custom logic is involved; and it makes reviewing the core functionality this PR is introducing more complicated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no much logic, just a simple addition to make sure that existing or better to say, required rule application cane be done, and this happens only in |
||||||||
value: &'g str, | ||||||||
opts: GlobPatternOpts<'g>, | ||||||||
} | ||||||||
|
||||||||
impl<'g> GlobBuilder<'g> { | ||||||||
/// Create a new builder with all the captures enabled by default. | ||||||||
pub fn new(value: &'g str) -> Self { | ||||||||
let opts = GlobPatternOpts { | ||||||||
star: "([^/]*?)", | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regex will match everything expect |
||||||||
double_star: "(.*?)", | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the double star, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this also supports proper globs, when you can have the glob like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that these three patterns already exist on master branch, they were just copied to a different location: relay/relay-common/src/utils.rs Lines 33 to 35 in 6e2f7ae
|
||||||||
question_mark: "(.)", | ||||||||
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fantastic idea of surrounding the regexes with parenthesis! |
||||||||
}; | ||||||||
Self { value, opts } | ||||||||
} | ||||||||
|
||||||||
/// Enable capture groups for `*` in the pattern. | ||||||||
pub fn capture_star(mut self, enable: bool) -> Self { | ||||||||
if !enable { | ||||||||
self.opts.star = "(?:[^/]*?)"; | ||||||||
} | ||||||||
self | ||||||||
} | ||||||||
|
||||||||
/// Enable capture groups for `**` in the pattern. | ||||||||
pub fn capture_double_star(mut self, enable: bool) -> Self { | ||||||||
if !enable { | ||||||||
self.opts.double_star = "(?:.*?)"; | ||||||||
} | ||||||||
self | ||||||||
} | ||||||||
|
||||||||
/// Enable capture groups for `?` in the pattern. | ||||||||
pub fn capture_question_mark(mut self, enable: bool) -> Self { | ||||||||
if !enable { | ||||||||
self.opts.question_mark = "(?:.)"; | ||||||||
} | ||||||||
self | ||||||||
} | ||||||||
|
||||||||
/// Create a new [`Glob`] from this builder. | ||||||||
pub fn build(self) -> Glob { | ||||||||
let mut pattern = String::with_capacity(&self.value.len() + 100); | ||||||||
let mut last = 0; | ||||||||
|
||||||||
pattern.push('^'); | ||||||||
|
||||||||
static GLOB_RE: OnceCell<Regex> = OnceCell::new(); | ||||||||
let regex = GLOB_RE.get_or_init(|| Regex::new(r#"\?|\*\*|\*"#).unwrap()); | ||||||||
|
||||||||
for m in regex.find_iter(glob) { | ||||||||
pattern.push_str(®ex::escape(&glob[last..m.start()])); | ||||||||
for m in regex.find_iter(self.value) { | ||||||||
pattern.push_str(®ex::escape(&self.value[last..m.start()])); | ||||||||
match m.as_str() { | ||||||||
"?" => pattern.push_str("(.)"), | ||||||||
"**" => pattern.push_str("(.*?)"), | ||||||||
"*" => pattern.push_str("([^/]*?)"), | ||||||||
"?" => pattern.push_str(self.opts.question_mark), | ||||||||
"**" => pattern.push_str(self.opts.double_star), | ||||||||
"*" => pattern.push_str(self.opts.star), | ||||||||
_ => {} | ||||||||
} | ||||||||
last = m.end(); | ||||||||
} | ||||||||
pattern.push_str(®ex::escape(&glob[last..])); | ||||||||
pattern.push_str(®ex::escape(&self.value[last..])); | ||||||||
pattern.push('$'); | ||||||||
|
||||||||
Glob { | ||||||||
value: glob.to_string(), | ||||||||
value: self.value.to_owned(), | ||||||||
pattern: Regex::new(&pattern).unwrap(), | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
/// A simple glob matcher. | ||||||||
/// | ||||||||
/// Supported are `?` for a single char, `*` for all but a slash and | ||||||||
/// `**` to match with slashes. | ||||||||
#[derive(Clone)] | ||||||||
pub struct Glob { | ||||||||
value: String, | ||||||||
pattern: Regex, | ||||||||
} | ||||||||
|
||||||||
impl Glob { | ||||||||
/// Creates the [`GlobBuilder`], which can be fine-tunned using helper methods. | ||||||||
pub fn builder(glob: &'_ str) -> GlobBuilder { | ||||||||
GlobBuilder::new(glob) | ||||||||
} | ||||||||
|
||||||||
/// Creates a new glob from a string. | ||||||||
/// | ||||||||
/// All the glob patterns (wildcards) are enabled in the captures, and can be returned by | ||||||||
/// `matches` function. | ||||||||
pub fn new(glob: &str) -> Glob { | ||||||||
GlobBuilder::new(glob).build() | ||||||||
} | ||||||||
|
||||||||
/// Returns the pattern as str. | ||||||||
pub fn pattern(&self) -> &str { | ||||||||
|
@@ -56,6 +121,26 @@ impl Glob { | |||||||
self.pattern.is_match(value) | ||||||||
} | ||||||||
|
||||||||
/// Currently support replacing only all `*` in the input string with provided replacement. | ||||||||
/// If no match is found, then a copy of the string is returned unchanged. | ||||||||
pub fn apply(&self, input: &str, replacement: &str) -> String { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
|
||||||||
let mut output = String::new(); | ||||||||
let mut current = 0; | ||||||||
|
||||||||
for caps in self.pattern.captures_iter(input) { | ||||||||
// Create the iter on subcaptures and ignore the first capture, since this is always | ||||||||
// the entire string. | ||||||||
for cap in caps.iter().flatten().skip(1) { | ||||||||
output.push_str(&input[current..cap.start()]); | ||||||||
output.push_str(replacement); | ||||||||
current = cap.end(); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
output.push_str(&input[current..]); | ||||||||
output | ||||||||
} | ||||||||
|
||||||||
/// Checks if the value matches and returns the wildcard matches. | ||||||||
pub fn matches<'t>(&self, value: &'t str) -> Option<Vec<&'t str>> { | ||||||||
self.pattern.captures(value).map(|caps| { | ||||||||
|
@@ -67,6 +152,14 @@ impl Glob { | |||||||
} | ||||||||
} | ||||||||
|
||||||||
impl PartialEq for Glob { | ||||||||
fn eq(&self, other: &Self) -> bool { | ||||||||
self.value == other.value | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
impl Eq for Glob {} | ||||||||
|
||||||||
impl str::FromStr for Glob { | ||||||||
type Err = (); | ||||||||
|
||||||||
|
@@ -167,6 +260,26 @@ mod tests { | |||||||
let g = Glob::new("api/**/store/"); | ||||||||
assert!(g.is_match("api/some/stuff/here/store/")); | ||||||||
assert!(g.is_match("api/some/store/")); | ||||||||
|
||||||||
let g = Glob::new("/api/*/stuff/**"); | ||||||||
assert!(g.is_match("/api/some/stuff/here/store/")); | ||||||||
assert!(!g.is_match("/api/some/store/")); | ||||||||
} | ||||||||
|
||||||||
#[test] | ||||||||
fn test_glob_replace() { | ||||||||
let g = Glob::builder("/foo/*/bar/**") | ||||||||
.capture_star(true) | ||||||||
.capture_double_star(false) | ||||||||
.capture_question_mark(false) | ||||||||
.build(); | ||||||||
|
||||||||
assert_eq!( | ||||||||
g.apply("/foo/some/bar/here/store", "*"), | ||||||||
"/foo/*/bar/here/store" | ||||||||
); | ||||||||
assert_eq!(g.apply("/foo/testing/bar/", "*"), "/foo/*/bar/"); | ||||||||
assert_eq!(g.apply("/foo/testing/1/", "*"), "/foo/testing/1/"); | ||||||||
} | ||||||||
|
||||||||
#[test] | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,12 +1,15 @@ | ||||||
use std::fmt; | ||||||
use std::str::FromStr; | ||||||
|
||||||
use serde::{Deserialize, Serialize}; | ||||||
|
||||||
use crate::processor::ProcessValue; | ||||||
use crate::protocol::Timestamp; | ||||||
use crate::types::{Annotated, Empty, ErrorKind, FromValue, IntoValue, SkipSerialization, Value}; | ||||||
|
||||||
/// Describes how the name of the transaction was determined. | ||||||
#[derive(Clone, Debug, Eq, PartialEq)] | ||||||
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] | ||||||
#[serde(rename_all = "kebab-case")] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol TIL kebab-case, I didn't know there was a name for this. It's made my day 😂 |
||||||
#[cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema))] | ||||||
#[cfg_attr(feature = "jsonschema", schemars(rename_all = "kebab-case"))] | ||||||
pub enum TransactionSource { | ||||||
|
@@ -20,6 +23,8 @@ pub enum TransactionSource { | |||||
View, | ||||||
/// Named after a software component, such as a function or class name. | ||||||
Component, | ||||||
/// The transaction name was updated to remove high cardinality parts. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably a nit -- a "sanitized" transaction name doesn't necessarily require getting high cardinality parts removed. Currently, we are only going to support the removal feature, but let's not limit ourselves to that.
Suggested change
|
||||||
Sanitized, | ||||||
/// Name of a background task (e.g. a Celery task). | ||||||
Task, | ||||||
/// This is the default value set by Relay for legacy SDKs. | ||||||
|
@@ -37,6 +42,7 @@ impl TransactionSource { | |||||
Self::Route => "route", | ||||||
Self::View => "view", | ||||||
Self::Component => "component", | ||||||
Self::Sanitized => "sanitized", | ||||||
Self::Task => "task", | ||||||
Self::Unknown => "unknown", | ||||||
Self::Other(ref s) => s, | ||||||
|
@@ -54,6 +60,7 @@ impl FromStr for TransactionSource { | |||||
"route" => Ok(Self::Route), | ||||||
"view" => Ok(Self::View), | ||||||
"component" => Ok(Self::Component), | ||||||
"sanitized" => Ok(Self::Sanitized), | ||||||
"task" => Ok(Self::Task), | ||||||
"unknown" => Ok(Self::Unknown), | ||||||
s => Ok(Self::Other(s.to_owned())), | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
mod processor; | ||
mod rules; | ||
|
||
pub use processor::*; | ||
pub use rules::*; |
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've mentioned this somewhere in a comment below: let's not focus this PR on having the source set to
url
. This is the default use case and the only one we currently support, but that's defined insentry
and not in relay. In fact, if the code is a bit generalized we could already be supporting more sources out of the box.