Skip to content

Commit

Permalink
fix(profiling): Only transfer valid profile ids (#3809)
Browse files Browse the repository at this point in the history 
This was transfering the profile id onto the transaction all the time
which is not desirable since we need to fully validate the profile
first.
  • Loading branch information
@Zylphrex
Zylphrex committed Jul 12, 2024
1 parent afc85d5 commit 8806f9c
Show file tree
Hide file tree
Showing 3 changed files with 192 additions and 21 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
- Fixes metrics dropped due to missing project state. ([#3553](https://github.com/getsentry/relay/issues/3553))
- Incorrect span outcomes when generated from a indexed transaction quota. ([#3793](https://github.com/getsentry/relay/pull/3793))
- Report outcomes for spans when transactions are rate limited. ([#3749](https://github.com/getsentry/relay/pull/3749))
- Only transfer valid profile ids. ([#3809](https://github.com/getsentry/relay/pull/3809))

**Internal**:

Expand Down
1 change: 1 addition & 0 deletions relay-server/src/services/processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1699,6 +1699,7 @@ impl EnvelopeProcessorService {
if_processing!(self.inner.config, {
// Process profiles before extracting metrics, to make sure they are removed if they are invalid.
let profile_id = profile::process(state, &self.inner.config);
profile::transfer_id(state, profile_id);

// Always extract metrics in processing Relays for sampled items.
self.extract_transaction_metrics(state, SamplingDecision::Keep, profile_id)?;
Expand Down
211 changes: 190 additions & 21 deletions relay-server/src/services/processor/profile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ fn expand_profile(item: &mut Item, event: &Event, config: &Config) -> Result<Pro
mod tests {
use std::sync::Arc;

#[cfg(feature = "processing")]
use insta::assert_debug_snapshot;
#[cfg(not(feature = "processing"))]
use relay_dynamic_config::Feature;
Expand All @@ -162,10 +163,18 @@ mod tests {

use super::*;

#[cfg(feature = "processing")]
#[tokio::test]
async fn test_profile_id_transfered() {
// Setup
let processor = create_test_processor(Default::default());
let config = Config::from_json_value(serde_json::json!({
"processing": {
"enabled": true,
"kafka_config": []
}
}))
.unwrap();
let processor = create_test_processor(config);
let event_id = EventId::new();
let dsn = "https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42"
.parse()
Expand All @@ -179,25 +188,24 @@ mod tests {

item.set_payload(
ContentType::Json,
r#"
{
"type": "transaction",
"transaction": "/foo/",
"timestamp": 946684810.0,
"start_timestamp": 946684800.0,
"contexts": {
"trace": {
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
"span_id": "fa90fdead5f74053",
"op": "http.server",
"type": "trace"
r#"{
"event_id": "9b73438f70e044ecbd006b7fd15b7373",
"type": "transaction",
"transaction": "/foo/",
"timestamp": 946684810.0,
"start_timestamp": 946684800.0,
"contexts": {
"trace": {
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
"span_id": "fa90fdead5f74053",
"op": "http.server",
"type": "trace"
}
},
"transaction_info": {
"source": "url"
}
},
"transaction_info": {
"source": "url"
}
}
"#,
}"#,
);
item
});
Expand All @@ -213,7 +221,32 @@ mod tests {
"platform": "android",
"os": {"name": "foo", "version": "bar"},
"device": {"architecture": "zap"},
"timestamp": "2023-10-10 00:00:00Z"
"timestamp": "2023-10-10 00:00:00Z",
"profile": {
"samples":[
{
"stack_id":0,
"elapsed_since_start_ns":1,
"thread_id":1
},
{
"stack_id":0,
"elapsed_since_start_ns":2,
"thread_id":1
}
],
"stacks":[[0]],
"frames":[{
"function":"main"
}]
},
"transactions": [
{
"id": "9b73438f70e044ecbd006b7fd15b7373",
"name": "/foo/",
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2"
}
]
}"#,
);
item
Expand Down Expand Up @@ -260,6 +293,134 @@ mod tests {
"###);
}

#[cfg(feature = "processing")]
#[tokio::test]
async fn test_invalid_profile_id_not_transfered() {
// Setup
let config = Config::from_json_value(serde_json::json!({
"processing": {
"enabled": true,
"kafka_config": []
}
}))
.unwrap();
let processor = create_test_processor(config);
let event_id = EventId::new();
let dsn = "https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42"
.parse()
.unwrap();
let request_meta = RequestMeta::new(dsn);
let mut envelope = Envelope::from_request(Some(event_id), request_meta);

// Add a valid transaction item.
envelope.add_item({
let mut item = Item::new(ItemType::Transaction);

item.set_payload(
ContentType::Json,
r#"{
"event_id": "9b73438f70e044ecbd006b7fd15b7373",
"type": "transaction",
"transaction": "/foo/",
"timestamp": 946684810.0,
"start_timestamp": 946684800.0,
"contexts": {
"trace": {
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
"span_id": "fa90fdead5f74053",
"op": "http.server",
"type": "trace"
}
},
"transaction_info": {
"source": "url"
}
}"#,
);
item
});

// Add a profile to the same envelope.
envelope.add_item({
let mut item = Item::new(ItemType::Profile);
item.set_payload(
ContentType::Json,
r#"{
"profile_id": "012d836b15bb49d7bbf99e64295d995b",
"version": "1",
"platform": "android",
"os": {"name": "foo", "version": "bar"},
"device": {"architecture": "zap"},
"timestamp": "2023-10-10 00:00:00Z",
"profile": {
"samples":[
{
"stack_id":0,
"elapsed_since_start_ns":1,
"thread_id":1
},
{
"stack_id":1,
"elapsed_since_start_ns":2,
"thread_id":1
}
],
"stacks":[[0],[]],
"frames":[{
"function":"main"
}]
},
"transactions": [
{
"id": "9b73438f70e044ecbd006b7fd15b7373",
"name": "/foo/",
"trace_id": "4c79f60c11214eb38604f4ae0781bfb2"
}
]
}"#,
);
item
});

let mut project_state = ProjectState::allowed();
project_state.config.features.0.insert(Feature::Profiling);

let mut envelopes = ProcessingGroup::split_envelope(*envelope);
assert_eq!(envelopes.len(), 1);

let (group, envelope) = envelopes.pop().unwrap();
let envelope = ManagedEnvelope::standalone(envelope, Addr::dummy(), Addr::dummy(), group);

let message = ProcessEnvelope {
envelope,
project_state: Arc::new(project_state),
sampling_project_state: None,
reservoir_counters: ReservoirCounters::default(),
};

let envelope_response = processor.process(message).unwrap();
let ctx = envelope_response.envelope.unwrap();
let new_envelope = ctx.envelope();

// Get the re-serialized context.
let item = new_envelope
.get_item_by(|item| item.ty() == &ItemType::Transaction)
.unwrap();
let transaction = Annotated::<Event>::from_json_bytes(&item.payload()).unwrap();
let context = transaction
.value()
.unwrap()
.context::<ProfileContext>()
.unwrap();

assert_debug_snapshot!(context, @r###"
ProfileContext {
profile_id: ~,
profiler_id: ~,
}
"###);
}

#[tokio::test]
async fn filter_standalone_profile() {
relay_log::init_test!();
Expand Down Expand Up @@ -311,10 +472,18 @@ mod tests {
assert!(envelope_response.envelope.is_none());
}

#[cfg(feature = "processing")]
#[tokio::test]
async fn test_profile_id_removed_profiler_id_kept() {
// Setup
let processor = create_test_processor(Default::default());
let config = Config::from_json_value(serde_json::json!({
"processing": {
"enabled": true,
"kafka_config": []
}
}))
.unwrap();
let processor = create_test_processor(config);
let event_id = EventId::new();
let dsn = "https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42"
.parse()
Expand Down

0 comments on commit 8806f9c

Please sign in to comment.