feat(dynamic-sampling): Update error tagging by considering new field in DSC #2290

iambriccardo · 2023-07-07T07:03:14Z

This PR adds the new sampled check for the dsc.

The new logic will work as follows:

If DSC has no sampled field, tagging will not be performed.
If DSC has sampled = true, tagging will be performed and the value will depend on the sampling decision.
If DSC has sampled = false, tagging will be performed and the value will be false.

TBS1996 · 2023-07-07T07:39:22Z

relay-server/src/actors/processor.rs

+        // We test with incoming dsc that contains `sampled = false`.
+        let mut envelope = Envelope::from_request(Some(event_id), request_meta.clone());
+        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("transaction1".into()),
+            sample_rate: None,
+            sampled: Some(false),
+            other: BTreeMap::new(),
+        };
+        envelope.set_dsc(dsc);
+        envelope.add_item(mocked_error_item());
+        let sampling_project_state = project_state_with_single_rule(0.0);
+        let new_envelope = process_envelope_with_root_project_state(
+            envelope,
+            Some(Arc::new(sampling_project_state)),
+        );
+        let event = extract_first_event_from_envelope(new_envelope);
+        let trace_context = event
+            .contexts
+            .value()
+            .unwrap()
+            .get_context(TraceContext::default_key())
+            .unwrap();
+
+        assert!(matches!(trace_context, Trace(..)));
+        if let Trace(context) = trace_context {
+            assert!(!context.sampled.value().unwrap())
+        }
+
+        // We test with incoming dsc that doesn't have a transaction set.
+        let mut envelope = Envelope::from_request(Some(event_id), request_meta.clone());
+        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: None,
+            sample_rate: None,
+            sampled: Some(true),
+            other: BTreeMap::new(),
+        };
+        envelope.set_dsc(dsc);
+        envelope.add_item(mocked_error_item());
+        let sampling_project_state = project_state_with_single_rule(0.0);
+        let new_envelope = process_envelope_with_root_project_state(
+            envelope,
+            Some(Arc::new(sampling_project_state)),
+        );
+        let event = extract_first_event_from_envelope(new_envelope);
+
+        assert!(event.contexts.value().is_none());
+


this test is getting very long, what do you think about splitting it ?

I had two ways of proceeding, clustering it by areas or individual tests. Which proposal would you have?

jan-auer · 2023-07-10T13:45:29Z

relay-sampling/src/lib.rs

+        }
+    }
+
+    pub fn serialize<S>(value: &Option<bool>, serializer: S) -> Result<S::Ok, S::Error>


Let's support deserializing from both bool and string, then always serialize with the default serializer.

If we want to be extra lenient, we could ignore all errors and make them None.

There's a way to deserialize this without going through Value by implementing a visitor. I can help with that tomorrow.

As for serialization, I'd still ask to serialize directly as boolean. You can remove fn serialize entirely and just define deserialize_with.

jan-auer · 2023-07-10T13:55:29Z

relay-server/src/actors/processor.rs

+            // In case the dsc doesn't have a transaction, we don't want to tag it. This can happen
+            // since in tracing without performance we can have a trace that is not made up of
+            // transactions.


I'm not clear on the reasoning behind this, could you elaborate? It looks like we should primarily look at the sampled flag from the client.

… in DSC

jan-auer · 2023-07-11T19:11:50Z

relay-sampling/src/lib.rs

+        }
+    }
+
+    pub fn serialize<S>(value: &Option<bool>, serializer: S) -> Result<S::Ok, S::Error>


There's a way to deserialize this without going through Value by implementing a visitor. I can help with that tomorrow.

As for serialization, I'd still ask to serialize directly as boolean. You can remove fn serialize entirely and just define deserialize_with.

jan-auer · 2023-07-11T19:14:12Z

relay-sampling/src/lib.rs

@@ -1287,7 +1330,9 @@ impl DynamicSamplingContext {

        let contexts = event.contexts.value()?;
        let context = contexts.get(TraceContext::default_key())?.value()?;
-        let Context::Trace(ref trace) = context.0 else { return None };
+        let Context::Trace(ref trace) = context.0 else {


Sorry for causing conflicts through #2296. Once you merge or rebase master, this would be removed.

jan-auer · 2023-07-11T19:15:28Z

relay-sampling/src/lib.rs

+        }
+        "#;
+        let dsc = serde_json::from_str::<DynamicSamplingContext>(json).unwrap();
+        insta::assert_ron_snapshot!(dsc, @r###"


Instead of RON, could you assert JSON here? This should roundtrip cleanly, since we require the upstream to parse this again.

jan-auer · 2023-07-11T19:18:39Z

relay-server/src/actors/processor.rs

+        let context = event
+            .contexts
+            .get_or_insert_with(Contexts::new)
+            .get_or_insert_with(TraceContext::default_key(), || Trace(Box::default()));


Also here please check for the upstream diff, since there's a new Contexts::insert_or_default() and the import on enum variant Trace has been removed as per style guide.

jan-auer · 2023-07-11T19:19:57Z

relay-server/src/actors/processor.rs

+    }
+
+    #[tokio::test]
+    async fn test_error_is_not_tagged_if_inputs_are_invalid() {


Should such tests rather be unit tests on one of the util functions? It's great to have a few tests covering the full call hierarchy, though the differences between these tests actually occur within is_trace_fully_sampled, if I read this correctly.

Yep, I didn't refactor them since i wanted to wait for your ok first

relay-server/src/utils/dynamic_sampling.rs

jan-auer · 2023-07-11T19:24:56Z

relay-server/src/utils/dynamic_sampling.rs

+    // In reality the dynamic sampling logic supports optional root state and dsc but it will
+    // return keep. In our case having a keep in case of none root state and dsc will be
+    // a problem, since in reality we can't infer anything without trace metadata.
+    let sampling_result = if let (Some(root_project_state), Some(dsc)) = (root_project_state, dsc) {


nit: This function could become more readable if written with early returns. Even though it is just a minor style change, the cases and control flow discussed in the comment demonstrate that we need to take great care here.

let dsc = dsc?; let root_project_state = root_project_state?; // If the sampled field is not set, we prefer to not tag the error since we have no clue on // whether the head of the trace was kept or dropped on the client side. // If the head of the trace was dropped on the client we will immediately mark // the trace as not fully sampled. if !dsc.sampled? { return Some(false); } let sampling_result = get_sampling_result(...); // rest here...

Yeah much better, I personally don't like the usage of ! and ? in the same expression but it's a matter of personal taste.

jan-auer · 2023-07-11T19:26:28Z

relay-server/src/utils/dynamic_sampling.rs

+                ),
+                // If the head of the trace was dropped on the client we will immediately mark
+                // the trace as not fully sampled.
+                false => SamplingResult::Drop(MatchedRuleIds(vec![])),


This constructs a SamplingResult only to convert it back to a boolean false later. You could map this directly here, and map in the other branch alone. This would also qualify for another early return.

jan-auer · 2023-07-11T19:34:23Z

relay-server/src/utils/dynamic_sampling.rs

@@ -101,6 +101,51 @@ pub fn get_sampling_result(
    SamplingResult::determine_from_sampling_match(sampling_result)
 }

+pub fn is_trace_fully_sampled(
+    processing_enabled: bool,


For a follow-up PR: The processing_enabled flag doesn't really belong here, and if we trace it in, then it's just to emit an error warning. Could we look at an alternative way or log this unconditionally?

We would have to really try to lift that out and see how we can implement sampling either without or with a semantically more meaningful value.

Due to the changes in getsentry/relay#2290, `trace.sampled` is being applied more conservatively to events. With the current sorting logic, we may be selecting events FROM before the relay change because they were the only ones with `trace.sampled: true`. Since we only check the last 7 days for the `/helpful` endpoint, we can add this line back in once the relay change is 7 days old.

iambriccardo force-pushed the riccardo/feat/dsc-with-sampled branch 2 times, most recently from 58aa85c to 89a72e7 Compare July 7, 2023 07:23

TBS1996 reviewed Jul 7, 2023

View reviewed changes

iambriccardo force-pushed the riccardo/feat/dsc-with-sampled branch 2 times, most recently from 1f40d8d to 8b8ce6e Compare July 10, 2023 06:19

iambriccardo requested review from TBS1996 and jan-auer July 10, 2023 13:36

jan-auer reviewed Jul 10, 2023

View reviewed changes

iambriccardo force-pushed the riccardo/feat/dsc-with-sampled branch from 2af9640 to f676fcf Compare July 11, 2023 09:24

iambriccardo added 9 commits July 11, 2023 15:30

feat(dynamic-sampling): Update error tagging by considering new field…

4e5ec58

… in DSC

Fix

dedad6b

Improve tests

a7d092c

Fix tests

8a83d37

fiX

41f32f5

Fix

2d24958

Update

fe16ef7

Add tests

99a3254

Add tests

58dff9d

iambriccardo force-pushed the riccardo/feat/dsc-with-sampled branch from eedb303 to 42e6d94 Compare July 11, 2023 13:30

iambriccardo marked this pull request as ready for review July 11, 2023 14:12

iambriccardo requested review from a team and jan-auer July 11, 2023 14:12

iambriccardo force-pushed the riccardo/feat/dsc-with-sampled branch from 42e6d94 to 7606f9b Compare July 11, 2023 14:13

Fix

376cb7e

iambriccardo force-pushed the riccardo/feat/dsc-with-sampled branch from 7606f9b to 376cb7e Compare July 11, 2023 14:13

jan-auer reviewed Jul 11, 2023

View reviewed changes

iambriccardo added 4 commits July 12, 2023 09:16

Fix

b4f0c26

Merge

9f70704

Fix

9d6f7cd

Fix

4adba50

Fix

d97c8f8

jan-auer approved these changes Jul 12, 2023

View reviewed changes

Fix

0d49368

iambriccardo merged commit e075e84 into master Jul 13, 2023

iambriccardo deleted the riccardo/feat/dsc-with-sampled branch July 13, 2023 11:36

malwilley mentioned this pull request Jul 18, 2023

feat(helpful-event): Temporarily remove trace.sampled from sort getsentry/sentry#53065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dynamic-sampling): Update error tagging by considering new field in DSC #2290

feat(dynamic-sampling): Update error tagging by considering new field in DSC #2290

iambriccardo commented Jul 7, 2023 •

edited

Loading

TBS1996 Jul 7, 2023

iambriccardo Jul 7, 2023

jan-auer Jul 10, 2023

jan-auer Jul 11, 2023

jan-auer Jul 10, 2023 •

edited

Loading

jan-auer Jul 11, 2023

jan-auer Jul 11, 2023

jan-auer Jul 11, 2023

jan-auer Jul 11, 2023

jan-auer Jul 11, 2023

iambriccardo Jul 12, 2023

jan-auer Jul 11, 2023

iambriccardo Jul 12, 2023

jan-auer Jul 11, 2023

jan-auer Jul 11, 2023

iambriccardo Jul 12, 2023

feat(dynamic-sampling): Update error tagging by considering new field in DSC #2290

feat(dynamic-sampling): Update error tagging by considering new field in DSC #2290

Conversation

iambriccardo commented Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo commented Jul 7, 2023 •

edited

Loading

jan-auer Jul 10, 2023 •

edited

Loading