Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(pattern): Fix mongodb patterns, assert all patterns in debug profile #4125

Merged
merged 3 commits into from
Oct 9, 2024
+125 −5
Merged

fix(pattern): Fix mongodb patterns, assert all patterns in debug profile #4125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a97bc3c
test(pattern): Assert all patterns when compiling in debug mode
Dav1dde Oct 9, 2024
21db425
fix mongodb globs
Dav1dde Oct 9, 2024
69dc769
fix broken span db system query
Dav1dde Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions relay-dynamic-config/src/defaults.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const MOBILE_OPS: &[&str] = &[
const APP_START_ROOT_SPAN_DESCRIPTIONS: &[&str] = &["Cold Start", "Warm Start"];

/// A list of patterns found in MongoDB queries.
const MONGODB_QUERIES: &[&str] = &["*\"$*", "{*", "*({*", "*[{*"];
const MONGODB_QUERIES: &[&str] = &["*\"$*", r"\{*", r"*(\{*", r"*\[\{*"];
Dav1dde marked this conversation as resolved.
Show resolved Hide resolved

/// A list of patterns for resource span ops we'd like to ingest.
const RESOURCE_SPAN_OPS: &[&str] = &["resource.script", "resource.css", "resource.img"];
Expand Down Expand Up @@ -117,7 +117,7 @@ pub fn hardcoded_span_metrics() -> Vec<(GroupKey, Vec<MetricSpec>, Vec<TagMappin
let is_db = RuleCondition::eq("span.sentry_tags.category", "db")
& !RuleCondition::glob("span.op", DISABLED_DATABASES)
// MongoDB queries are only allowed when `span.system` is set to `mongodb`.
& (RuleCondition::eq("span.system", "mongodb")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjq turns out this condition had the wrong field, which we did not notice because MONGODB_QUERIES contained an invalid pattern.

& (RuleCondition::eq("span.data.db\\.system", "mongodb")
| !RuleCondition::glob("span.description", MONGODB_QUERIES));
let is_resource = RuleCondition::glob("span.op", RESOURCE_SPAN_OPS);

Expand Down
10 changes: 7 additions & 3 deletions relay-pattern/src/typed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,9 @@ impl<C: PatternConfig> FromIterator<String> for TypedPatterns<C> {
fn from_iter<T: IntoIterator<Item = String>>(iter: T) -> Self {
let mut builder = Self::builder();
for pattern in iter.into_iter() {
let _ = builder.add(pattern);
let _err = builder.add(pattern);
#[cfg(debug_assertions)]
_err.expect("all patterns should be valid patterns");
}
builder.build()
}
Expand Down Expand Up @@ -248,7 +250,9 @@ impl<'de, C: PatternConfig> serde::Deserialize<'de> for TypedPatterns<C> {

while let Some(item) = seq.next_element()? {
// Ignore invalid patterns as documented.
let _ = builder.add(item);
let _err = builder.add(item);
#[cfg(debug_assertions)]
_err.expect("all patterns should be valid patterns");
}

Ok(builder.build())
Expand Down Expand Up @@ -398,7 +402,7 @@ mod tests {
}

#[test]
#[cfg(feature = "serde")]
#[cfg(all(feature = "serde", not(debug_assertions)))]
fn test_patterns_deserialize_err() {
let r: TypedPatterns<CaseInsensitive> =
serde_json::from_str(r#"["[invalid","foobar"]"#).unwrap();
Expand Down
60 changes: 60 additions & 0 deletions ...snapshots/relay_server__metrics_extraction__event__tests__both_feature_flags_enabled.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3682,8 +3682,12 @@ expression: metrics
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
"span.system": "mongodb",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
metadata: BucketMetadata {
Expand All @@ -3694,6 +3698,32 @@ expression: metrics
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
name: MetricName(
"d:spans/exclusive_time_light@millisecond",
),
value: Distribution(
[
2000.0,
],
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
"span.system": "mongodb",
},
metadata: BucketMetadata {
merges: 1,
received_at: Some(
UnixTimestamp(0),
),
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
Expand All @@ -3707,8 +3737,12 @@ expression: metrics
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
"span.system": "mongodb",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
metadata: BucketMetadata {
Expand All @@ -3719,6 +3753,32 @@ expression: metrics
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
name: MetricName(
"d:spans/duration_light@millisecond",
),
value: Distribution(
[
2000.0,
],
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
"span.system": "mongodb",
},
metadata: BucketMetadata {
merges: 1,
received_at: Some(
UnixTimestamp(0),
),
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
Expand Down
56 changes: 56 additions & 0 deletions ...ics_extraction/snapshots/relay_server__metrics_extraction__event__tests__only_common.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3388,8 +3388,11 @@ expression: metrics
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
metadata: BucketMetadata {
Expand All @@ -3400,6 +3403,31 @@ expression: metrics
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
name: MetricName(
"d:spans/exclusive_time_light@millisecond",
),
value: Distribution(
[
2000.0,
],
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
},
metadata: BucketMetadata {
merges: 1,
received_at: Some(
UnixTimestamp(0),
),
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
Expand All @@ -3413,8 +3441,11 @@ expression: metrics
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
"transaction": "gEt /api/:version/users/",
"transaction.method": "POST",
"transaction.op": "myop",
},
metadata: BucketMetadata {
Expand All @@ -3425,6 +3456,31 @@ expression: metrics
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
name: MetricName(
"d:spans/duration_light@millisecond",
),
value: Distribution(
[
2000.0,
],
),
tags: {
"environment": "fake_environment",
"span.action": "COUNT",
"span.category": "db",
"span.op": "db.sql.query",
},
metadata: BucketMetadata {
merges: 1,
received_at: Some(
UnixTimestamp(0),
),
extracted_from_indexed: false,
},
},
Bucket {
timestamp: UnixTimestamp(1597976302),
width: 0,
Expand Down
Loading