From 75f64c6b0150e1ff0ba050d93d8641e35444ad26 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 12:27:08 +0100 Subject: [PATCH 1/7] update sample rate in dsc --- sentry_sdk/scope.py | 7 +++++++ sentry_sdk/tracing_utils.py | 2 +- tests/tracing/test_sampling.py | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index c22cdfb030..2412ffbd69 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -1043,6 +1043,13 @@ def start_transaction( sampling_context.update(custom_sampling_context) transaction._set_initial_sampling_decision(sampling_context=sampling_context) + # update the sample rate in the dsc + propagation_context = self.get_active_propagation_context() + if propagation_context: + dsc = propagation_context.dynamic_sampling_context + if dsc is not None: + dsc["sample_rate"] = str(transaction.sample_rate) + if transaction.sampled: profile = Profile( transaction.sampled, transaction._start_timestamp_monotonic_ns diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index 0459563776..00b081a084 100644 --- a/sentry_sdk/tracing_utils.py +++ b/sentry_sdk/tracing_utils.py @@ -378,7 +378,7 @@ def __init__( self.parent_sampled = parent_sampled """Boolean indicator if the parent span was sampled. Important when the parent span originated in an upstream service, - because we watn to sample the whole trace, or nothing from the trace.""" + because we want to sample the whole trace, or nothing from the trace.""" self.dynamic_sampling_context = dynamic_sampling_context """Data that is used for dynamic sampling decisions.""" diff --git a/tests/tracing/test_sampling.py b/tests/tracing/test_sampling.py index 2e6ed0dab3..f10c246b6a 100644 --- a/tests/tracing/test_sampling.py +++ b/tests/tracing/test_sampling.py @@ -211,7 +211,7 @@ def test_passes_parent_sampling_decision_in_sampling_context( assert sampling_context["parent_sampled"]._mock_wraps is parent_sampling_decision -def test_passes_custom_samling_context_from_start_transaction_to_traces_sampler( +def test_passes_custom_sampling_context_from_start_transaction_to_traces_sampler( sentry_init, DictionaryContaining # noqa: N803 ): traces_sampler = mock.Mock() From feb2c1375d8f72658887ad3a74be67adda218ded Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 12:50:32 +0100 Subject: [PATCH 2/7] test --- sentry_sdk/scope.py | 4 ++- sentry_sdk/tracing.py | 1 - tests/test_dsc.py | 79 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 2412ffbd69..3283f376ea 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -1043,7 +1043,9 @@ def start_transaction( sampling_context.update(custom_sampling_context) transaction._set_initial_sampling_decision(sampling_context=sampling_context) - # update the sample rate in the dsc + # update the sample rate in the dsc. the baggage should be immutable + # at this point, but in order to not have broken traces we make + # an exception here propagation_context = self.get_active_propagation_context() if propagation_context: dsc = propagation_context.dynamic_sampling_context diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 3868b2e6c8..bc973f4b8f 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -1061,7 +1061,6 @@ def get_baggage(self): The first time a new baggage with Sentry items is made, it will be frozen.""" - if not self._baggage or self._baggage.mutable: self._baggage = Baggage.populate_from_transaction(self) diff --git a/tests/test_dsc.py b/tests/test_dsc.py index 3b8cff5baf..eb6e7099b8 100644 --- a/tests/test_dsc.py +++ b/tests/test_dsc.py @@ -8,6 +8,9 @@ This is not tested in this file. """ +import random +from unittest import mock + import pytest import sentry_sdk @@ -134,6 +137,82 @@ def test_dsc_continuation_of_trace(sentry_init, capture_envelopes): assert envelope_trace_header["transaction"] == "bar" +def test_dsc_continuation_of_trace_sample_rate_changed(sentry_init, capture_envelopes): + """ + Another service calls our service and passes tracing information to us. + Our service is continuing the trace, but modifies the sample rate. + The DSC propagated further should contain the updated sample rate. + """ + + def my_traces_sampler(sampling_context): + return 0.25 + + sentry_init( + dsn="https://mysecret@bla.ingest.sentry.io/12312012", + release="myapp@0.0.1", + environment="canary", + traces_sampler=my_traces_sampler, + ) + envelopes = capture_envelopes() + + # This is what the upstream service sends us + sentry_trace = "771a43a4192642f0b136d5159a501700-1234567890abcdef-1" + baggage = ( + "other-vendor-value-1=foo;bar;baz, " + "sentry-trace_id=771a43a4192642f0b136d5159a501700, " + "sentry-public_key=frontendpublickey, " + "sentry-sample_rate=1.0, " + "sentry-sampled=true, " + "sentry-release=myfrontend@1.2.3, " + "sentry-environment=bird, " + "sentry-transaction=bar, " + "other-vendor-value-2=foo;bar;" + ) + incoming_http_headers = { + "HTTP_SENTRY_TRACE": sentry_trace, + "HTTP_BAGGAGE": baggage, + } + + # We continue the incoming trace and start a new transaction + with mock.patch.object(random, "random", return_value=0.2): + transaction = sentry_sdk.continue_trace(incoming_http_headers) + with sentry_sdk.start_transaction(transaction, name="foo"): + pass + + assert len(envelopes) == 1 + + transaction_envelope = envelopes[0] + envelope_trace_header = transaction_envelope.headers["trace"] + + assert "trace_id" in envelope_trace_header + assert type(envelope_trace_header["trace_id"]) == str + assert envelope_trace_header["trace_id"] == "771a43a4192642f0b136d5159a501700" + + assert "public_key" in envelope_trace_header + assert type(envelope_trace_header["public_key"]) == str + assert envelope_trace_header["public_key"] == "frontendpublickey" + + assert "sample_rate" in envelope_trace_header + assert type(envelope_trace_header["sample_rate"]) == str + assert envelope_trace_header["sample_rate"] == "0.25" + + assert "sampled" in envelope_trace_header + assert type(envelope_trace_header["sampled"]) == str + assert envelope_trace_header["sampled"] == "true" + + assert "release" in envelope_trace_header + assert type(envelope_trace_header["release"]) == str + assert envelope_trace_header["release"] == "myfrontend@1.2.3" + + assert "environment" in envelope_trace_header + assert type(envelope_trace_header["environment"]) == str + assert envelope_trace_header["environment"] == "bird" + + assert "transaction" in envelope_trace_header + assert type(envelope_trace_header["transaction"]) == str + assert envelope_trace_header["transaction"] == "bar" + + def test_dsc_issue(sentry_init, capture_envelopes): """ Our service is a standalone service that does not have tracing enabled. Just uses Sentry for error reporting. From d8241135676be2f3236ce255d2d7f1ec9fb1ca39 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 12:59:29 +0100 Subject: [PATCH 3/7] guard against none --- sentry_sdk/scope.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 3283f376ea..fc2e760c3d 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -1047,7 +1047,7 @@ def start_transaction( # at this point, but in order to not have broken traces we make # an exception here propagation_context = self.get_active_propagation_context() - if propagation_context: + if propagation_context and transaction.sample_rate is not None: dsc = propagation_context.dynamic_sampling_context if dsc is not None: dsc["sample_rate"] = str(transaction.sample_rate) From 94c5aacdf6eab00a479eb7da45dde1f766fc8e25 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 14:22:13 +0100 Subject: [PATCH 4/7] meh --- sentry_sdk/scope.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index fc2e760c3d..1c4cf57e8c 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -1043,15 +1043,18 @@ def start_transaction( sampling_context.update(custom_sampling_context) transaction._set_initial_sampling_decision(sampling_context=sampling_context) - # update the sample rate in the dsc. the baggage should be immutable - # at this point, but in order to not have broken traces we make - # an exception here + # update the sample rate in the dsc propagation_context = self.get_active_propagation_context() if propagation_context and transaction.sample_rate is not None: dsc = propagation_context.dynamic_sampling_context if dsc is not None: dsc["sample_rate"] = str(transaction.sample_rate) + if transaction._baggage: + dsc = transaction._baggage.sentry_items["sample_rate"] = str( + transaction.sample_rate + ) + if transaction.sampled: profile = Profile( transaction.sampled, transaction._start_timestamp_monotonic_ns From 11ed72ed9998e32fa78533350f3c8018fdd36c94 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 14:52:14 +0100 Subject: [PATCH 5/7] tests --- tests/integrations/stdlib/test_httplib.py | 15 ++++++++------- tests/test_dsc.py | 6 ++++-- tests/tracing/test_integration_tests.py | 2 +- tests/tracing/test_sampling.py | 21 ++++++++++----------- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/tests/integrations/stdlib/test_httplib.py b/tests/integrations/stdlib/test_httplib.py index 200b282f53..2522300a0a 100644 --- a/tests/integrations/stdlib/test_httplib.py +++ b/tests/integrations/stdlib/test_httplib.py @@ -140,12 +140,13 @@ def test_outgoing_trace_headers(sentry_init, monkeypatch): sentry_init(traces_sample_rate=1.0) - headers = {} - headers["baggage"] = ( - "other-vendor-value-1=foo;bar;baz, sentry-trace_id=771a43a4192642f0b136d5159a501700, " - "sentry-public_key=49d0f7386ad645858ae85020e393bef3, sentry-sample_rate=0.01337, " - "sentry-user_id=Am%C3%A9lie, other-vendor-value-2=foo;bar;" - ) + headers = { + "baggage": ( + "other-vendor-value-1=foo;bar;baz, sentry-trace_id=771a43a4192642f0b136d5159a501700, " + "sentry-public_key=49d0f7386ad645858ae85020e393bef3, sentry-sample_rate=0.01337, " + "sentry-user_id=Am%C3%A9lie, other-vendor-value-2=foo;bar;" + ), + } transaction = Transaction.continue_from_headers(headers) @@ -175,7 +176,7 @@ def test_outgoing_trace_headers(sentry_init, monkeypatch): expected_outgoing_baggage = ( "sentry-trace_id=771a43a4192642f0b136d5159a501700," "sentry-public_key=49d0f7386ad645858ae85020e393bef3," - "sentry-sample_rate=0.01337," + "sentry-sample_rate=1.0," "sentry-user_id=Am%C3%A9lie" ) diff --git a/tests/test_dsc.py b/tests/test_dsc.py index eb6e7099b8..4837384a8e 100644 --- a/tests/test_dsc.py +++ b/tests/test_dsc.py @@ -118,7 +118,7 @@ def test_dsc_continuation_of_trace(sentry_init, capture_envelopes): assert "sample_rate" in envelope_trace_header assert type(envelope_trace_header["sample_rate"]) == str - assert envelope_trace_header["sample_rate"] == "0.01337" + assert envelope_trace_header["sample_rate"] == "1.0" assert "sampled" in envelope_trace_header assert type(envelope_trace_header["sampled"]) == str @@ -137,7 +137,9 @@ def test_dsc_continuation_of_trace(sentry_init, capture_envelopes): assert envelope_trace_header["transaction"] == "bar" -def test_dsc_continuation_of_trace_sample_rate_changed(sentry_init, capture_envelopes): +def test_dsc_continuation_of_trace_sample_rate_changed_in_traces_sampler( + sentry_init, capture_envelopes +): """ Another service calls our service and passes tracing information to us. Our service is continuing the trace, but modifies the sample rate. diff --git a/tests/tracing/test_integration_tests.py b/tests/tracing/test_integration_tests.py index da3efef9eb..bd2686cddc 100644 --- a/tests/tracing/test_integration_tests.py +++ b/tests/tracing/test_integration_tests.py @@ -132,7 +132,7 @@ def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_r "public_key": "49d0f7386ad645858ae85020e393bef3", "trace_id": "771a43a4192642f0b136d5159a501700", "user_id": "Amelie", - "sample_rate": "0.01337", + "sample_rate": str(sample_rate), } assert message_payload["message"] == "hello" diff --git a/tests/tracing/test_sampling.py b/tests/tracing/test_sampling.py index f10c246b6a..1ad08ecec2 100644 --- a/tests/tracing/test_sampling.py +++ b/tests/tracing/test_sampling.py @@ -198,17 +198,16 @@ def test_passes_parent_sampling_decision_in_sampling_context( transaction = Transaction.continue_from_headers( headers={"sentry-trace": sentry_trace_header}, name="dogpark" ) - spy = mock.Mock(wraps=transaction) - start_transaction(transaction=spy) - - # there's only one call (so index at 0) and kwargs are always last in a call - # tuple (so index at -1) - sampling_context = spy._set_initial_sampling_decision.mock_calls[0][-1][ - "sampling_context" - ] - assert "parent_sampled" in sampling_context - # because we passed in a spy, attribute access requires unwrapping - assert sampling_context["parent_sampled"]._mock_wraps is parent_sampling_decision + + def mock_set_initial_sampling_decision(_, sampling_context): + assert "parent_sampled" in sampling_context + assert sampling_context["parent_sampled"] is parent_sampling_decision + + with mock.patch( + "sentry_sdk.tracing.Transaction._set_initial_sampling_decision", + mock_set_initial_sampling_decision, + ): + start_transaction(transaction=transaction) def test_passes_custom_sampling_context_from_start_transaction_to_traces_sampler( From 19082e8eb81e2071669fc459f338a55de44ae053 Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 14:53:50 +0100 Subject: [PATCH 6/7] cleanup --- sentry_sdk/scope.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 1c4cf57e8c..53191c45da 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -1044,16 +1044,16 @@ def start_transaction( transaction._set_initial_sampling_decision(sampling_context=sampling_context) # update the sample rate in the dsc - propagation_context = self.get_active_propagation_context() - if propagation_context and transaction.sample_rate is not None: - dsc = propagation_context.dynamic_sampling_context - if dsc is not None: - dsc["sample_rate"] = str(transaction.sample_rate) - - if transaction._baggage: - dsc = transaction._baggage.sentry_items["sample_rate"] = str( - transaction.sample_rate - ) + if transaction.sample_rate is not None: + propagation_context = self.get_active_propagation_context() + if propagation_context: + dsc = propagation_context.dynamic_sampling_context + if dsc is not None: + dsc["sample_rate"] = str(transaction.sample_rate) + if transaction._baggage: + transaction._baggage.sentry_items["sample_rate"] = str( + transaction.sample_rate + ) if transaction.sampled: profile = Profile( From f259ede00052143fa7e6f0445a49aeb31ff363ce Mon Sep 17 00:00:00 2001 From: Ivana Kellyer Date: Thu, 6 Feb 2025 15:27:21 +0100 Subject: [PATCH 7/7] fix test --- tests/tracing/test_integration_tests.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/tests/tracing/test_integration_tests.py b/tests/tracing/test_integration_tests.py index bd2686cddc..bb41364c35 100644 --- a/tests/tracing/test_integration_tests.py +++ b/tests/tracing/test_integration_tests.py @@ -51,9 +51,11 @@ def test_basic(sentry_init, capture_events, sample_rate): assert not events -@pytest.mark.parametrize("sampled", [True, False, None]) +@pytest.mark.parametrize("parent_sampled", [True, False, None]) @pytest.mark.parametrize("sample_rate", [0.0, 1.0]) -def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_rate): +def test_continue_from_headers( + sentry_init, capture_envelopes, parent_sampled, sample_rate +): """ Ensure data is actually passed along via headers, and that they are read correctly. @@ -64,7 +66,7 @@ def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_r # make a parent transaction (normally this would be in a different service) with start_transaction(name="hi", sampled=True if sample_rate == 0 else None): with start_span() as old_span: - old_span.sampled = sampled + old_span.sampled = parent_sampled headers = dict( sentry_sdk.get_current_scope().iter_trace_propagation_headers(old_span) ) @@ -79,7 +81,7 @@ def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_r # child transaction, to prove that we can read 'sentry-trace' header data correctly child_transaction = Transaction.continue_from_headers(headers, name="WRONG") assert child_transaction is not None - assert child_transaction.parent_sampled == sampled + assert child_transaction.parent_sampled == parent_sampled assert child_transaction.trace_id == old_span.trace_id assert child_transaction.same_process_as_parent is False assert child_transaction.parent_span_id == old_span.span_id @@ -104,8 +106,8 @@ def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_r sentry_sdk.get_current_scope().transaction = "ho" capture_message("hello") - # in this case the child transaction won't be captured - if sampled is False or (sample_rate == 0 and sampled is None): + if parent_sampled is False or (sample_rate == 0 and parent_sampled is None): + # in this case the child transaction won't be captured trace1, message = envelopes message_payload = message.get_event() trace1_payload = trace1.get_transaction_event() @@ -127,12 +129,17 @@ def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_r == message_payload["contexts"]["trace"]["trace_id"] ) + if parent_sampled is not None: + expected_sample_rate = str(float(parent_sampled)) + else: + expected_sample_rate = str(sample_rate) + assert trace2.headers["trace"] == baggage.dynamic_sampling_context() assert trace2.headers["trace"] == { "public_key": "49d0f7386ad645858ae85020e393bef3", "trace_id": "771a43a4192642f0b136d5159a501700", "user_id": "Amelie", - "sample_rate": str(sample_rate), + "sample_rate": expected_sample_rate, } assert message_payload["message"] == "hello"