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.

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): Infer span.op if not defined #4056

Merged
merged 5 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

- Add a config option to add default tags to all Relay Sentry events. ([#3944](https://github.com/getsentry/relay/pull/3944))
- Automatically derive `client.address` and `user.geo` for standalone spans. ([#4047](https://github.com/getsentry/relay/pull/4047))
- Configurable span.op inference. ([#4056](https://github.com/getsentry/relay/pull/4056))

## 24.9.0

Expand Down
1 change: 1 addition & 0 deletions relay-cabi/src/processing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ pub unsafe extern "C" fn relay_store_normalizer_normalize_event(
replay_id: config.replay_id,
span_allowed_hosts: &[], // only supported in relay
scrub_mongo_description: ScrubMongoDescription::Disabled, // only supported in relay
span_op_defaults: Default::default(), // only supported in relay
};
normalize_event(&mut event, &normalization_config);

Expand Down
11 changes: 9 additions & 2 deletions relay-dynamic-config/src/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::io::BufReader;
use std::path::Path;

use relay_base_schema::metrics::MetricNamespace;
use relay_event_normalization::{MeasurementsConfig, ModelCosts};
use relay_event_normalization::{MeasurementsConfig, ModelCosts, SpanOpDefaults};
use relay_filter::GenericFiltersConfig;
use relay_quotas::Quota;
use serde::{de, Deserialize, Serialize};
Expand Down Expand Up @@ -49,6 +49,13 @@ pub struct GlobalConfig {
/// Configuration for AI span measurements.
#[serde(skip_serializing_if = "is_missing")]
pub ai_model_costs: ErrorBoundary<ModelCosts>,

/// Configuration to derive the `span.op` from other span fields.
#[serde(
deserialize_with = "default_on_error",
skip_serializing_if = "is_default"
)]
pub span_op_defaults: SpanOpDefaults,
}

impl GlobalConfig {
Expand Down Expand Up @@ -337,7 +344,7 @@ pub enum BucketEncoding {

/// Returns `true` if this value is equal to `Default::default()`.
fn is_default<T: Default + PartialEq>(t: &T) -> bool {
*t == T::default()
t == &T::default()
}

fn default_on_error<'de, D, T>(deserializer: D) -> Result<T, D::Error>
Expand Down
16 changes: 11 additions & 5 deletions relay-event-normalization/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ use crate::span::tag_extraction::extract_span_tags_from_event;
use crate::utils::{self, get_event_user_tag, MAX_DURATION_MOBILE_MS};
use crate::{
breakdowns, event_error, legacy, mechanism, remove_other, schema, span, stacktrace,
transactions, trimming, user_agent, BreakdownsConfig, CombinedMeasurementsConfig, GeoIpLookup,
MaxChars, ModelCosts, PerformanceScoreConfig, RawUserAgentInfo, SpanDescriptionRule,
TransactionNameConfig,
transactions, trimming, user_agent, BorrowedSpanOpDefaults, BreakdownsConfig,
CombinedMeasurementsConfig, GeoIpLookup, MaxChars, ModelCosts, PerformanceScoreConfig,
RawUserAgentInfo, SpanDescriptionRule, TransactionNameConfig,
};

/// Configuration for [`normalize_event`].
Expand Down Expand Up @@ -161,6 +161,9 @@ pub struct NormalizationConfig<'a> {

/// Controls whether or not MongoDB span descriptions will be scrubbed.
pub scrub_mongo_description: ScrubMongoDescription,

/// Rules to infer `span.op` from other span fields.
pub span_op_defaults: BorrowedSpanOpDefaults<'a>,
}

impl<'a> Default for NormalizationConfig<'a> {
Expand Down Expand Up @@ -194,6 +197,7 @@ impl<'a> Default for NormalizationConfig<'a> {
replay_id: Default::default(),
span_allowed_hosts: Default::default(),
scrub_mongo_description: ScrubMongoDescription::Disabled,
span_op_defaults: Default::default(),
}
}
}
Expand Down Expand Up @@ -244,8 +248,10 @@ fn normalize(event: &mut Event, meta: &mut Meta, config: &NormalizationConfig) {
// (internally noops for non-transaction events).
// TODO: Parts of this processor should probably be a filter so we
// can revert some changes to ProcessingAction)
let mut transactions_processor =
transactions::TransactionsProcessor::new(config.transaction_name_config.clone());
let mut transactions_processor = transactions::TransactionsProcessor::new(
config.transaction_name_config,
config.span_op_defaults,
);
let _ = transactions_processor.process_event(event, meta, ProcessingState::root());

// Process security reports first to ensure all props.
Expand Down
144 changes: 129 additions & 15 deletions relay-event-normalization/src/transactions/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ use relay_event_schema::processor::{
self, ProcessValue, ProcessingResult, ProcessingState, Processor,
};
use relay_event_schema::protocol::{Event, Span, SpanStatus, TraceContext, TransactionSource};
use relay_protocol::{Annotated, Meta, Remark, RemarkType};
use relay_protocol::{Annotated, Meta, Remark, RemarkType, RuleCondition};
use serde::{Deserialize, Serialize};

use crate::regexes::TRANSACTION_NAME_NORMALIZER_REGEX;
use crate::TransactionNameRule;

/// Configuration for sanitizing unparameterized transaction names.
#[derive(Clone, Debug, Default)]
#[derive(Clone, Copy, Debug, Default)]
pub struct TransactionNameConfig<'r> {
/// Rules for identifier replacement that were discovered by Sentry's transaction clusterer.
pub rules: &'r [TransactionNameRule],
Expand Down Expand Up @@ -76,12 +77,27 @@ pub fn apply_transaction_rename_rules(
#[derive(Debug, Default)]
pub struct TransactionsProcessor<'r> {
name_config: TransactionNameConfig<'r>,
span_op_defaults: BorrowedSpanOpDefaults<'r>,
}

impl<'r> TransactionsProcessor<'r> {
/// Creates a new `TransactionsProcessor` instance.
pub fn new(name_config: TransactionNameConfig<'r>) -> Self {
Self { name_config }
pub fn new(
name_config: TransactionNameConfig<'r>,
span_op_defaults: BorrowedSpanOpDefaults<'r>,
) -> Self {
Self {
name_config,
span_op_defaults,
}
}

#[cfg(test)]
fn new_name_config(name_config: TransactionNameConfig<'r>) -> Self {
Self {
name_config,
..Default::default()
}
}

/// Returns `true` if the given transaction name should be treated as a URL.
Expand Down Expand Up @@ -163,14 +179,61 @@ impl Processor for TransactionsProcessor<'_> {
_meta: &mut Meta,
state: &ProcessingState<'_>,
) -> ProcessingResult {
span.op.get_or_insert_with(|| "default".to_owned());

if span.op.value().is_none() {
*span.op.value_mut() = Some(self.span_op_defaults.infer(span));
}
span.process_child_values(self, state)?;

Ok(())
}
}

/// Rules used to infer `span.op` from other span fields.
#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)]
pub struct SpanOpDefaults {
/// List of rules to apply. First match wins.
pub rules: Vec<SpanOpDefaultRule>,
}

impl SpanOpDefaults {
/// Gets a borrowed version of this config.
pub fn borrow(&self) -> BorrowedSpanOpDefaults {
BorrowedSpanOpDefaults {
rules: self.rules.as_slice(),
}
}
}
Dav1dde marked this conversation as resolved.
Show resolved Hide resolved

/// Borrowed version of [`SpanOpDefaults`].
#[derive(Clone, Copy, Debug, Default)]
pub struct BorrowedSpanOpDefaults<'a> {
rules: &'a [SpanOpDefaultRule],
}

impl BorrowedSpanOpDefaults<'_> {
/// Infer the span op from a set of rules.
///
/// The first matching rule determines the span op.
/// If no rule matches, `"default"` is returned.
fn infer(&self, span: &Span) -> String {
for rule in self.rules {
if rule.condition.matches(span) {
return rule.value.clone();
}
}
"default".to_owned()
Comment on lines +219 to +224
Copy link
Member

Choose a reason for hiding this comment

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

Untested:

Suggested change
for rule in self.rules {
if rule.condition.matches(span) {
return rule.value.clone();
}
}
"default".to_owned()
self.rules.iter()
.find(|rule| rule.condition.matches(span))
.cloned()
.unwrap_or("default".to_owned())

Copy link
Member Author

Choose a reason for hiding this comment

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

I kind of like the for loop, even though it takes one more line.

}
}

/// A rule to infer [`Span::op`] from other span fields.
#[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
pub struct SpanOpDefaultRule {
/// When to set the given value.
pub condition: RuleCondition,
/// Value for the [`Span::op`]. Only set if omitted by the SDK.
pub value: String,
}

/// Span status codes for the Ruby Rack integration that indicate raw URLs being sent as
/// transaction names. These cases are considered as high-cardinality.
///
Expand Down Expand Up @@ -336,14 +399,14 @@ fn scrub_identifiers_with_regex(

#[cfg(test)]
mod tests {

use chrono::{Duration, TimeZone, Utc};
use insta::assert_debug_snapshot;
use itertools::Itertools;
use relay_common::glob2::LazyGlob;
use relay_event_schema::processor::process_value;
use relay_event_schema::protocol::{ClientSdkInfo, Contexts, SpanId, TraceId};
use relay_protocol::{assert_annotated_snapshot, get_value};
use serde_json::json;

use crate::validation::validate_event;
use crate::{EventValidationConfig, RedactionRule};
Expand Down Expand Up @@ -911,7 +974,7 @@ mod tests {

process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig {
&mut TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: &[rule1, rule2, rule3],
}),
ProcessingState::root(),
Expand Down Expand Up @@ -992,7 +1055,7 @@ mod tests {

process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig {
&mut TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: &[rule1, rule2, rule3],
}),
ProcessingState::root(),
Expand Down Expand Up @@ -1059,7 +1122,7 @@ mod tests {

let mut event = Annotated::<Event>::from_json(json).unwrap();

let mut processor = TransactionsProcessor::new(TransactionNameConfig {
let mut processor = TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: rules.as_ref(),
});
process_value(&mut event, &mut processor, ProcessingState::root()).unwrap();
Expand Down Expand Up @@ -1174,7 +1237,7 @@ mod tests {
// This must not normalize transaction name, since it's disabled.
process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig {
&mut TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: rules.as_ref(),
}),
ProcessingState::root(),
Expand Down Expand Up @@ -1231,7 +1294,7 @@ mod tests {

process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig {
&mut TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: rules.as_ref(),
}),
ProcessingState::root(),
Expand Down Expand Up @@ -1316,7 +1379,7 @@ mod tests {

process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig { rules: &[rule] }),
&mut TransactionsProcessor::new_name_config(TransactionNameConfig { rules: &[rule] }),
ProcessingState::root(),
)
.unwrap();
Expand Down Expand Up @@ -1521,7 +1584,7 @@ mod tests {

process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig {
&mut TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: &[TransactionNameRule {
pattern: LazyGlob::new("/remains/*/1234567890/".to_owned()),
expiry: Utc.with_ymd_and_hms(3000, 1, 1, 1, 1, 1).unwrap(),
Expand Down Expand Up @@ -1587,7 +1650,7 @@ mod tests {

process_value(
&mut event,
&mut TransactionsProcessor::new(TransactionNameConfig {
&mut TransactionsProcessor::new_name_config(TransactionNameConfig {
rules: &[TransactionNameRule {
pattern: LazyGlob::new("/remains/*/**".to_owned()),
expiry: Utc.with_ymd_and_hms(3000, 1, 1, 1, 1, 1).unwrap(),
Expand Down Expand Up @@ -1627,4 +1690,55 @@ mod tests {
},
]"#);
}

#[test]
fn test_infer_span_op_default() {
let span = Annotated::from_json(r#"{}"#).unwrap();
let defaults: SpanOpDefaults = serde_json::from_value(json!({
"rules": [{
"condition": {
"op": "not",
"inner": {
"op": "eq",
"name": "span.data.messaging\\.system",
"value": null,
},
},
"value": "message"
}]
}
))
.unwrap();
let op = defaults.borrow().infer(span.value().unwrap());
assert_eq!(&op, "default");
}

#[test]
fn test_infer_span_op_messaging() {
let span = Annotated::from_json(
r#"{
"data": {
"messaging.system": "activemq"
}
}"#,
)
.unwrap();
let defaults: SpanOpDefaults = serde_json::from_value(json!({
"rules": [{
"condition": {
"op": "not",
"inner": {
"op": "eq",
"name": "span.data.messaging\\.system",
"value": null,
},
},
"value": "message"
}]
}
))
.unwrap();
let op = defaults.borrow().infer(span.value().unwrap());
assert_eq!(&op, "message");
}
}
1 change: 1 addition & 0 deletions relay-server/src/services/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1610,6 +1610,7 @@ impl EnvelopeProcessorService {
} else {
ScrubMongoDescription::Disabled
},
span_op_defaults: global_config.span_op_defaults.borrow(),
};

metric!(timer(RelayTimers::EventProcessingNormalization), {
Expand Down
Loading
Loading