Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -5,6 +5,7 @@
**Features**:

- Extend GPU context with data for Unreal Engine crash reports. ([#3144](https://github.com/getsentry/relay/pull/3144))
- Parametrize transaction in dynamic sampling context. ([#3141](https://github.com/getsentry/relay/pull/3141))

**Bug Fixes**:

Expand Down
97 changes: 54 additions & 43 deletions relay-event-normalization/src/transactions/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,59 @@ pub struct TransactionNameConfig<'r> {
pub rules: &'r [TransactionNameRule],
}

/// Apply parametrization to transaction.
pub fn normalize_transaction_name(
transaction: &mut Annotated<String>,
rules: &[TransactionNameRule],
) {
// Normalize transaction names for URLs and Sanitized transaction sources.
// This in addition to renaming rules can catch some high cardinality parts.
scrub_identifiers(transaction);

// Apply rules discovered by the transaction clusterer in sentry.
if !rules.is_empty() {
apply_transaction_rename_rules(transaction, rules);
}
}

/// Applies the rule if any found to the transaction name.
///
/// It find the first rule matching the criteria:
/// - source matchining the one provided in the rule sorce
/// - rule hasn't epired yet
/// - glob pattern matches the transaction name
///
/// Note: we add `/` at the end of the transaction name if there isn't one, to make sure that
/// patterns like `/<something>/*/**` where we have `**` at the end are a match.
pub fn apply_transaction_rename_rules(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unchanged, just moved

transaction: &mut Annotated<String>,
rules: &[TransactionNameRule],
) {
let _ = processor::apply(transaction, |transaction, meta| {
let result = rules.iter().find_map(|rule| {
rule.match_and_apply(Cow::Borrowed(transaction))
.map(|applied_result| (rule.pattern.compiled().pattern(), applied_result))
});

if let Some((rule, result)) = result {
if *transaction != result {
// If another rule was applied before, we don't want to
// rename the transaction name to keep the original one.
// We do want to continue adding remarks though, in
// order to keep track of all rules applied.
if meta.original_value().is_none() {
meta.set_original_value(Some(transaction.clone()));
}
// add also the rule which was applied to the transaction name
meta.add_remark(Remark::new(RemarkType::Substituted, rule));
*transaction = result;
}
}

Ok(())
});
}

/// Rejects transactions based on required fields.
#[derive(Debug, Default)]
pub struct TransactionsProcessor<'r> {
Expand All @@ -31,41 +84,6 @@ impl<'r> TransactionsProcessor<'r> {
Self { name_config }
}

/// Applies the rule if any found to the transaction name.
///
/// It find the first rule matching the criteria:
/// - source matchining the one provided in the rule sorce
/// - rule hasn't epired yet
/// - glob pattern matches the transaction name
///
/// Note: we add `/` at the end of the transaction name if there isn't one, to make sure that
/// patterns like `/<something>/*/**` where we have `**` at the end are a match.
pub fn apply_transaction_rename_rule(&self, transaction: &mut Annotated<String>) {
let _ = processor::apply(transaction, |transaction, meta| {
let result = self.name_config.rules.iter().find_map(|rule| {
rule.match_and_apply(Cow::Borrowed(transaction))
.map(|applied_result| (rule.pattern.compiled().pattern(), applied_result))
});

if let Some((rule, result)) = result {
if *transaction != result {
// If another rule was applied before, we don't want to
// rename the transaction name to keep the original one.
// We do want to continue adding remarks though, in
// order to keep track of all rules applied.
if meta.original_value().is_none() {
meta.set_original_value(Some(transaction.clone()));
}
// add also the rule which was applied to the transaction name
meta.add_remark(Remark::new(RemarkType::Substituted, rule));
*transaction = result;
}
}

Ok(())
});
}

/// Returns `true` if the given transaction name should be treated as a URL.
///
/// We treat a transaction as URL if one of the following conditions apply:
Expand All @@ -89,14 +107,7 @@ impl<'r> TransactionsProcessor<'r> {

fn normalize_transaction_name(&self, event: &mut Event) {
if self.treat_transaction_as_url(event) {
// Normalize transaction names for URLs and Sanitized transaction sources.
// This in addition to renaming rules can catch some high cardinality parts.
scrub_identifiers(&mut event.transaction);

// Apply rules discovered by the transaction clusterer in sentry.
if !self.name_config.rules.is_empty() {
self.apply_transaction_rename_rule(&mut event.transaction);
}
normalize_transaction_name(&mut event.transaction, self.name_config.rules);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extracted for re-use with dsc


// Always mark URL transactions as sanitized, even if no modification were made by
// clusterer rules or regex matchers. This has the consequence that the transaction name
Expand Down
77 changes: 75 additions & 2 deletions relay-server/src/envelope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
//!
//! ```

use relay_event_normalization::{normalize_transaction_name, TransactionNameRule};
use std::borrow::Borrow;
use std::collections::BTreeMap;
use std::fmt;
Expand All @@ -42,7 +43,7 @@ use bytes::Bytes;
use chrono::{DateTime, Utc};
use relay_dynamic_config::ErrorBoundary;
use relay_event_schema::protocol::{EventId, EventType};
use relay_protocol::Value;
use relay_protocol::{Annotated, Value};
use relay_quotas::DataCategory;
use relay_sampling::DynamicSamplingContext;
use serde::de::DeserializeOwned;
Expand Down Expand Up @@ -1144,6 +1145,29 @@ impl Envelope {
self.headers.retention = Some(retention);
}

/// Runs transaction parametrization on the DSC trace transaction.
///
/// The purpose is for trace rules to match on the parametrized version of the transaction.
pub fn parametrize_dsc_transaction(&mut self, rules: &[TransactionNameRule]) {
let Some(ErrorBoundary::Ok(dsc)) = &mut self.headers.trace else {
return;
};

let parametrized_transaction = match &dsc.transaction {
Some(transaction) if transaction.contains('/') => {
// Ideally we would only apply transaction rules to transactions with source `url`,
// but the DSC does not contain this information. The chance of a transaction rename rule
// accidentially matching a non-URL transaction should be very low.
let mut annotated = Annotated::new(transaction.clone());
normalize_transaction_name(&mut annotated, rules);
annotated.into_value()
}
_ => return,
};

dsc.transaction = parametrized_transaction;
}

/// Returns the dynamic sampling context from envelope headers, if present.
pub fn dsc(&self) -> Option<&DynamicSamplingContext> {
match &self.headers.trace {
Expand Down Expand Up @@ -1404,7 +1428,7 @@ impl Envelope {

#[cfg(test)]
mod tests {
use relay_base_schema::project::ProjectId;
use relay_base_schema::project::{ProjectId, ProjectKey};

use super::*;

Expand Down Expand Up @@ -1934,4 +1958,53 @@ mod tests {
assert_eq!(item.ty(), &ItemType::Attachment);
}
}

#[test]
fn test_parametrize_root_transaction() {
let dsc = DynamicSamplingContext {
trace_id: Uuid::new_v4(),
public_key: ProjectKey::parse("abd0f232775f45feab79864e580d160b").unwrap(),
release: Some("1.1.1".to_string()),
user: Default::default(),
replay_id: None,
environment: None,
transaction: Some("/auth/login/test/".into()), // the only important bit for this test
sample_rate: Some(0.5),
sampled: Some(true),
other: BTreeMap::new(),
};

let rule: TransactionNameRule = {
// here you see the pattern that'll transform the transaction name.
let json = r#"{
"pattern": "/auth/login/*/**",
"expiry": "3022-11-30T00:00:00.000000Z",
"redaction": {
"method": "replace",
"substitution": "*"
}
}"#;

serde_json::from_str(json).unwrap()
};

// Envelope only created in order to run the parametrize dsc method.
let mut envelope = {
let bytes = bytes::Bytes::from("{\"event_id\":\"9ec79c33ec9942ab8353589fcb2e04dc\",\"dsn\":\"https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42\"}\n");
*Envelope::parse_bytes(bytes).unwrap()
};
envelope.set_dsc(dsc.clone());

assert_eq!(
envelope.dsc().unwrap().transaction.as_ref().unwrap(),
"/auth/login/test/"
);
// parametrize the transaciton name in the dsc.
envelope.parametrize_dsc_transaction(&[rule]);

assert_eq!(
envelope.dsc().unwrap().transaction.as_ref().unwrap(),
"/auth/login/*/"
);
}
}
6 changes: 6 additions & 0 deletions relay-server/src/services/processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,12 @@ impl EnvelopeProcessorService {
&self,
state: &mut ProcessEnvelopeState<G>,
) -> Result<(), ProcessingError> {
if let Some(sampling_state) = state.sampling_project_state.as_ref().map(Arc::clone) {
state
.envelope_mut()
.parametrize_dsc_transaction(&sampling_state.config.tx_name_rules);
}

let request_meta = state.managed_envelope.envelope().meta();
let client_ipaddr = request_meta.client_addr().map(IpAddr::from);

Expand Down
66 changes: 66 additions & 0 deletions tests/integration/test_dynamic_sampling.py
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,72 @@ def test_it_tags_error(mini_sentry, relay, expected_sampled, sample_rate):
assert evt_id == event_id


def test_sample_on_parametrized_root_transaction(mini_sentry, relay):
"""
Tests that dynamic sampling on traces operate on
the parametrized version of the root transaction.
"""
project_id = 42
relay = relay(mini_sentry, _outcomes_enabled_config())

# The pattern used to parametrize the transaction
pattern = "/auth/login/*/**"
# How the original transaction is in the DSC.
original_transaction = "/auth/login/test/"
# What the transaction is transformed into, which the dynamic sampling rules should respect.
parametrized_transaction = "/auth/login/*/"

config = mini_sentry.add_basic_project_config(project_id)
config["config"]["transactionMetrics"] = {"version": 1}

sampling_config = mini_sentry.add_basic_project_config(43)
sampling_public_key = sampling_config["publicKeys"][0]["publicKey"]
sampling_config["config"]["txNameRules"] = [
{
"pattern": pattern,
"expiry": "3022-11-30T00:00:00.000000Z",
Copy link
Contributor Author

@TBS1996 TBS1996 Feb 21, 2024

Choose a reason for hiding this comment

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

note: test will fail 998 years from now

"redaction": {"method": "replace", "substitution": "*"},
}
]

ds = sampling_config["config"].setdefault("sampling", {})
ds.setdefault("version", 2)

sampling_rule = {
"samplingValue": {"type": "sampleRate", "value": 0.0},
"type": "trace",
"condition": {
"op": "and",
"inner": [
{
"op": "eq",
"name": "trace.transaction",
"value": parametrized_transaction,
"options": {
"ignoreCase": True,
},
}
],
},
"id": 1,
}

rules = ds.setdefault("rules", [sampling_rule])
rules.append(sampling_rule)

envelope = Envelope()
transaction_event, trace_id, _ = _create_transaction_item()
envelope.add_transaction(transaction_event)
_add_trace_info(
envelope, trace_id, sampling_public_key, transaction=original_transaction
)

relay.send_envelope(project_id, envelope)

outcome = mini_sentry.captured_outcomes.get(timeout=2)
assert outcome["outcomes"][0]["reason"] == "Sampled:1"


def test_it_keeps_events(mini_sentry, relay):
"""
Tests that when sampling is set to 100% for the trace context project the events are kept
Expand Down