-
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
feat(spans): Filter spans based on module #2511
Changes from 10 commits
b998ecd
91600f6
d9780bf
8c766cc
ecd5460
f03772e
ff1e92a
042aaea
8988921
ecf8a88
3b134b7
ca70bd3
bcb62b5
420014d
6f4604c
1e7292a
3f4233d
3aaf055
d8bcb9a
6fd5766
e044074
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 |
---|---|---|
@@ -1,6 +1,9 @@ | ||
use relay_base_schema::data_category::DataCategory; | ||
use relay_common::glob2::LazyGlob; | ||
use relay_sampling::condition::{EqCondition, RuleCondition}; | ||
use relay_common::glob3::GlobPatterns; | ||
use relay_sampling::condition::{ | ||
AndCondition, EqCondition, GlobCondition, NotCondition, OrCondition, RuleCondition, | ||
}; | ||
use serde_json::Value; | ||
|
||
use crate::feature::Feature; | ||
|
@@ -25,12 +28,71 @@ pub fn add_span_metrics(project_config: &mut ProjectConfig) { | |
return; | ||
} | ||
|
||
// Add conditions to filter spans if a specific module is enabled. | ||
// By default, this will extract all spans. | ||
let span_op_field_name = "span.op"; | ||
let span_op_conditions = if project_config | ||
.features | ||
.has(Feature::SpanMetricsExtractionAllModules) | ||
{ | ||
None | ||
} else { | ||
let mut rules: Vec<RuleCondition> = Vec::new(); | ||
|
||
if project_config | ||
.features | ||
.has(Feature::SpanMetricsExtractionDBModule) | ||
{ | ||
rules.push(RuleCondition::And(AndCondition { | ||
inner: vec![ | ||
RuleCondition::Glob(GlobCondition { | ||
name: span_op_field_name.into(), | ||
value: GlobPatterns::new(vec!["db*".into()]), | ||
}), | ||
RuleCondition::Not(NotCondition { | ||
inner: Box::new(RuleCondition::Eq(EqCondition { | ||
name: span_op_field_name.into(), | ||
value: Value::Array(vec![ | ||
"db.clickhouse".into(), | ||
"db.redis".into(), | ||
"db.sql.query".into(), | ||
]), | ||
options: Default::default(), | ||
})), | ||
}), | ||
], | ||
})); | ||
} | ||
|
||
if project_config | ||
.features | ||
.has(Feature::SpanMetricsExtractionBrowserModule) | ||
{ | ||
rules.push(RuleCondition::Glob(GlobCondition { | ||
name: span_op_field_name.into(), | ||
value: GlobPatterns::new(vec![ | ||
"browser*".into(), | ||
"http*".into(), | ||
"resource".into(), | ||
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.
|
||
"ui*".into(), | ||
]), | ||
})); | ||
} | ||
|
||
// If every module is disabled, no extraction. | ||
if rules.is_empty() { | ||
return; | ||
} | ||
|
||
Some(RuleCondition::Or(OrCondition { inner: rules })) | ||
}; | ||
|
||
config.metrics.extend([ | ||
MetricSpec { | ||
category: DataCategory::Span, | ||
mri: "d:spans/exclusive_time@millisecond".into(), | ||
field: Some("span.exclusive_time".into()), | ||
condition: None, | ||
condition: span_op_conditions.clone(), | ||
tags: vec![TagSpec { | ||
key: "transaction".into(), | ||
field: Some("span.data.transaction".into()), | ||
|
@@ -42,7 +104,7 @@ pub fn add_span_metrics(project_config: &mut ProjectConfig) { | |
category: DataCategory::Span, | ||
mri: "d:spans/exclusive_time_light@millisecond".into(), | ||
field: Some("span.exclusive_time".into()), | ||
condition: None, | ||
condition: span_op_conditions.clone(), | ||
phacops marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tags: Default::default(), | ||
}, | ||
]); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,7 @@ use { | |
relay_event_normalization::{StoreConfig, StoreProcessor}, | ||
relay_event_schema::protocol::ProfileContext, | ||
relay_quotas::{RateLimitingError, RedisRateLimiter}, | ||
std::collections::HashSet, | ||
symbolic_unreal::{Unreal4Error, Unreal4ErrorKind}, | ||
}; | ||
|
||
|
@@ -2278,6 +2279,27 @@ impl EnvelopeProcessorService { | |
state.managed_envelope.envelope_mut().add_item(item); | ||
}; | ||
|
||
let span_op_db_module_denylist: HashSet<String> = HashSet::from([ | ||
"db.clickhouse".into(), | ||
"db.redis".into(), | ||
"db.sql.query".into(), | ||
]); | ||
let span_op_browser_module_prefix_allowlist: Vec<String> = vec![ | ||
"browser".into(), | ||
"http".into(), | ||
"resource".into(), | ||
"ui".into(), | ||
]; | ||
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. Since these don't change during execution, what do you think about making them |
||
let all_modules_enabled = state | ||
.project_state | ||
.has_feature(Feature::SpanMetricsExtractionAllModules); | ||
let db_module_enabled = state | ||
.project_state | ||
.has_feature(Feature::SpanMetricsExtractionDBModule); | ||
let browser_module_enabled = state | ||
.project_state | ||
.has_feature(Feature::SpanMetricsExtractionBrowserModule); | ||
|
||
// Add child spans as envelope items. | ||
if let Some(child_spans) = event.spans.value() { | ||
for span in child_spans { | ||
|
@@ -2287,14 +2309,30 @@ impl EnvelopeProcessorService { | |
let Some(inner_span) = span.value_mut() else { | ||
continue; | ||
}; | ||
inner_span.segment_id = transaction_span.segment_id.clone(); | ||
inner_span.is_segment = Annotated::new(false); | ||
add_span(span); | ||
// HACK: filter spans based on module until we figure out grouping. | ||
let Some(span_op) = inner_span.op.value() else { | ||
continue; | ||
phacops marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
if all_modules_enabled | ||
|| (db_module_enabled | ||
&& span_op.starts_with("db") | ||
&& !span_op_db_module_denylist.contains(span_op)) | ||
|| (browser_module_enabled | ||
&& span_op_browser_module_prefix_allowlist | ||
.iter() | ||
.any(|prefix| span_op.starts_with(prefix))) | ||
{ | ||
inner_span.segment_id = transaction_span.segment_id.clone(); | ||
inner_span.is_segment = Annotated::new(false); | ||
add_span(span); | ||
} | ||
} | ||
} | ||
|
||
// Add transaction span as an envelope item. | ||
add_span(transaction_span.into()); | ||
// HACK: temporarily omit this span. | ||
//add_span(transaction_span.into()); | ||
} | ||
|
||
/// Computes the sampling decision on the incoming event | ||
|
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.
@AbhiPrasad what do you think about
"db.sql.query"
? I feel like we should allow this by default, and opt out the one problematic project that came up so farThere 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.
ah yeah I think we should allow it - good catch!