diff --git a/sentry_sdk/integrations/opentelemetry/sampler.py b/sentry_sdk/integrations/opentelemetry/sampler.py index 5fa41d28fc..0631c0b19e 100644 --- a/sentry_sdk/integrations/opentelemetry/sampler.py +++ b/sentry_sdk/integrations/opentelemetry/sampler.py @@ -34,10 +34,14 @@ def get_parent_sampled(parent_context, trace_id): # Only inherit sample rate if `traceId` is the same if is_span_context_valid and parent_context.trace_id == trace_id: # this is getSamplingDecision in JS + # if there was no sampling flag, defer the decision + dsc_sampled = parent_context.trace_state.get(TRACESTATE_SAMPLED_KEY) + if dsc_sampled == "deferred": + return None + if parent_context.trace_flags.sampled is not None: return parent_context.trace_flags.sampled - dsc_sampled = parent_context.trace_state.get(TRACESTATE_SAMPLED_KEY) if dsc_sampled == "true": return True elif dsc_sampled == "false": @@ -53,6 +57,8 @@ def dropped_result(parent_span_context, attributes, sample_rate=None): if TRACESTATE_SAMPLED_KEY not in trace_state: trace_state = trace_state.add(TRACESTATE_SAMPLED_KEY, "false") + elif trace_state.get(TRACESTATE_SAMPLED_KEY) == "deferred": + trace_state = trace_state.update(TRACESTATE_SAMPLED_KEY, "false") if sample_rate and TRACESTATE_SAMPLE_RATE_KEY not in trace_state: trace_state = trace_state.add(TRACESTATE_SAMPLE_RATE_KEY, str(sample_rate)) @@ -88,6 +94,9 @@ def sampled_result(span_context, attributes, sample_rate): if TRACESTATE_SAMPLED_KEY not in trace_state: trace_state = trace_state.add(TRACESTATE_SAMPLED_KEY, "true") + elif trace_state.get(TRACESTATE_SAMPLED_KEY) == "deferred": + trace_state = trace_state.update(TRACESTATE_SAMPLED_KEY, "true") + if TRACESTATE_SAMPLE_RATE_KEY not in trace_state: trace_state = trace_state.add(TRACESTATE_SAMPLE_RATE_KEY, str(sample_rate)) diff --git a/sentry_sdk/integrations/opentelemetry/scope.py b/sentry_sdk/integrations/opentelemetry/scope.py index 56df9a774a..f6df844109 100644 --- a/sentry_sdk/integrations/opentelemetry/scope.py +++ b/sentry_sdk/integrations/opentelemetry/scope.py @@ -6,6 +6,7 @@ SpanContext, NonRecordingSpan, TraceFlags, + TraceState, use_span, ) @@ -14,6 +15,7 @@ SENTRY_FORK_ISOLATION_SCOPE_KEY, SENTRY_USE_CURRENT_SCOPE_KEY, SENTRY_USE_ISOLATION_SCOPE_KEY, + TRACESTATE_SAMPLED_KEY, ) from sentry_sdk.integrations.opentelemetry.utils import trace_state_from_baggage from sentry_sdk.scope import Scope, ScopeType @@ -96,10 +98,15 @@ def _incoming_otel_span_context(self): else TraceFlags.DEFAULT ) - # TODO-neel-potel do we need parent and sampled like JS? - trace_state = None if self._propagation_context.baggage: trace_state = trace_state_from_baggage(self._propagation_context.baggage) + else: + trace_state = TraceState() + + # for twp to work, we also need to consider deferred sampling when the sampling + # flag is not present, so the above TraceFlags are not sufficient + if self._propagation_context.parent_sampled is None: + trace_state = trace_state.add(TRACESTATE_SAMPLED_KEY, "deferred") span_context = SpanContext( trace_id=int(self._propagation_context.trace_id, 16), # type: ignore diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 18b18ba8ef..56816c1328 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -1311,6 +1311,8 @@ def __exit__(self, ty, value, tb): # type: (Optional[Any], Optional[Any], Optional[Any]) -> None if value is not None: self.set_status(SPANSTATUS.INTERNAL_ERROR) + else: + self.set_status(SPANSTATUS.OK) self.finish() context.detach(self._ctx_token) diff --git a/tests/integrations/opentelemetry/test_compat.py b/tests/integrations/opentelemetry/test_compat.py index ece08ec900..f2292d9ff2 100644 --- a/tests/integrations/opentelemetry/test_compat.py +++ b/tests/integrations/opentelemetry/test_compat.py @@ -9,7 +9,7 @@ def test_transaction_name_span_description_compat( events = capture_events() - with sentry_sdk.start_transaction( + with sentry_sdk.start_span( name="trx-name", op="trx-op", ) as trx: @@ -33,13 +33,12 @@ def test_transaction_name_span_description_compat( assert spn.__class__.__name__ == "POTelSpan" assert spn.op == "span-op" assert spn.description == "span-desc" - assert spn.name is None + assert spn.name == "span-desc" assert spn._otel_span is not None assert spn._otel_span.name == "span-desc" assert spn._otel_span.attributes["sentry.op"] == "span-op" assert spn._otel_span.attributes["sentry.description"] == "span-desc" - assert "sentry.name" not in spn._otel_span.attributes transaction = events[0] assert transaction["transaction"] == "trx-name" @@ -53,4 +52,3 @@ def test_transaction_name_span_description_compat( assert span["op"] == "span-op" assert span["data"]["sentry.op"] == "span-op" assert span["data"]["sentry.description"] == "span-desc" - assert "sentry.name" not in span["data"] diff --git a/tests/integrations/opentelemetry/test_experimental.py b/tests/integrations/opentelemetry/test_experimental.py deleted file mode 100644 index 8e4b703361..0000000000 --- a/tests/integrations/opentelemetry/test_experimental.py +++ /dev/null @@ -1,47 +0,0 @@ -from unittest.mock import MagicMock, patch - -import pytest - - -@pytest.mark.forked -def test_integration_enabled_if_option_is_on(sentry_init, reset_integrations): - mocked_setup_once = MagicMock() - - with patch( - "sentry_sdk.integrations.opentelemetry.integration.OpenTelemetryIntegration.setup_once", - mocked_setup_once, - ): - sentry_init( - _experiments={ - "otel_powered_performance": True, - }, - ) - mocked_setup_once.assert_called_once() - - -@pytest.mark.forked -def test_integration_not_enabled_if_option_is_off(sentry_init, reset_integrations): - mocked_setup_once = MagicMock() - - with patch( - "sentry_sdk.integrations.opentelemetry.integration.OpenTelemetryIntegration.setup_once", - mocked_setup_once, - ): - sentry_init( - _experiments={ - "otel_powered_performance": False, - }, - ) - mocked_setup_once.assert_not_called() - - -@pytest.mark.forked -def test_integration_not_enabled_if_option_is_missing(sentry_init, reset_integrations): - mocked_setup_once = MagicMock() - - with patch( - "sentry_sdk.integrations.opentelemetry.integration.OpenTelemetryIntegration.setup_once", - mocked_setup_once, - ): - sentry_init() - mocked_setup_once.assert_not_called() diff --git a/tests/integrations/opentelemetry/test_potel.py b/tests/integrations/opentelemetry/test_potel.py index 2b972addd1..39c48f8cc8 100644 --- a/tests/integrations/opentelemetry/test_potel.py +++ b/tests/integrations/opentelemetry/test_potel.py @@ -1,8 +1,8 @@ import pytest - from opentelemetry import trace import sentry_sdk +from tests.conftest import ApproxDict tracer = trace.get_tracer(__name__) @@ -43,7 +43,6 @@ def test_root_span_transaction_payload_started_with_otel_only(capture_envelopes) assert "span_id" in trace_context assert trace_context["origin"] == "manual" assert trace_context["op"] == "request" - assert trace_context["status"] == "ok" assert payload["spans"] == [] @@ -63,7 +62,6 @@ def test_child_span_payload_started_with_otel_only(capture_envelopes): assert span["op"] == "db" assert span["description"] == "db" assert span["origin"] == "manual" - assert span["status"] == "ok" assert span["span_id"] is not None assert span["trace_id"] == payload["contexts"]["trace"]["trace_id"] assert span["parent_span_id"] == payload["contexts"]["trace"]["span_id"] @@ -222,8 +220,8 @@ def test_span_attributes_in_data_started_with_otel(capture_envelopes): (item,) = envelope.items payload = item.payload.json - assert payload["contexts"]["trace"]["data"] == {"foo": "bar", "baz": 42} - assert payload["spans"][0]["data"] == {"abc": 99, "def": "moo"} + assert payload["contexts"]["trace"]["data"] == ApproxDict({"foo": "bar", "baz": 42}) + assert payload["spans"][0]["data"] == ApproxDict({"abc": 99, "def": "moo"}) def test_span_data_started_with_sentry(capture_envelopes): @@ -238,18 +236,22 @@ def test_span_data_started_with_sentry(capture_envelopes): (item,) = envelope.items payload = item.payload.json - assert payload["contexts"]["trace"]["data"] == { - "foo": "bar", - "sentry.origin": "manual", - "sentry.description": "request", - "sentry.op": "http", - } - assert payload["spans"][0]["data"] == { - "baz": 42, - "sentry.origin": "manual", - "sentry.description": "statement", - "sentry.op": "db", - } + assert payload["contexts"]["trace"]["data"] == ApproxDict( + { + "foo": "bar", + "sentry.origin": "manual", + "sentry.description": "request", + "sentry.op": "http", + } + ) + assert payload["spans"][0]["data"] == ApproxDict( + { + "baz": 42, + "sentry.origin": "manual", + "sentry.description": "statement", + "sentry.op": "db", + } + ) def test_transaction_tags_started_with_otel(capture_envelopes): diff --git a/tests/integrations/opentelemetry/test_sampler.py b/tests/integrations/opentelemetry/test_sampler.py index dfd4981ecf..9e67eb7921 100644 --- a/tests/integrations/opentelemetry/test_sampler.py +++ b/tests/integrations/opentelemetry/test_sampler.py @@ -71,17 +71,13 @@ def test_sampling_traces_sample_rate_50(sentry_init, capture_envelopes): envelopes = capture_envelopes() - with mock.patch( - "sentry_sdk.integrations.opentelemetry.sampler.random", return_value=0.2 - ): # drop + with mock.patch("random.random", return_value=0.2): # drop with sentry_sdk.start_span(description="request a"): with sentry_sdk.start_span(description="cache a"): with sentry_sdk.start_span(description="db a"): ... - with mock.patch( - "sentry_sdk.integrations.opentelemetry.sampler.random", return_value=0.7 - ): # keep + with mock.patch("random.random", return_value=0.7): # keep with sentry_sdk.start_span(description="request b"): with sentry_sdk.start_span(description="cache b"): with sentry_sdk.start_span(description="db b"): @@ -101,46 +97,34 @@ def test_sampling_traces_sample_rate_50(sentry_init, capture_envelopes): def test_sampling_traces_sampler(sentry_init, capture_envelopes): def keep_only_a(sampling_context): if " a" in sampling_context["transaction_context"]["name"]: - return 0.05 + return 1 else: return 0 - sentry_init( - traces_sample_rate=1.0, - traces_sampler=keep_only_a, - ) + sentry_init(traces_sampler=keep_only_a) envelopes = capture_envelopes() - # Make sure random() always returns the same values - with mock.patch( - "sentry_sdk.integrations.opentelemetry.sampler.random", - side_effect=[0.04 for _ in range(12)], - ): - - with sentry_sdk.start_span(description="request a"): # keep - with sentry_sdk.start_span(description="cache a"): # keep - with sentry_sdk.start_span(description="db a"): # keep - ... + # children inherit from root spans + with sentry_sdk.start_span(description="request a"): # keep + with sentry_sdk.start_span(description="cache a"): + with sentry_sdk.start_span(description="db a"): + ... - with sentry_sdk.start_span(description="request b"): # drop - with sentry_sdk.start_span(description="cache b"): # drop - with sentry_sdk.start_span(description="db b"): # drop - ... + with sentry_sdk.start_span(description="request b"): # drop + with sentry_sdk.start_span(description="cache b"): + with sentry_sdk.start_span(description="db b"): + ... - with sentry_sdk.start_span(description="request c"): # drop - with sentry_sdk.start_span( - description="cache a c" - ): # keep (but trx dropped, so not collected) - with sentry_sdk.start_span( - description="db a c" - ): # keep (but trx dropped, so not collected) - ... + with sentry_sdk.start_span(description="request c"): # drop + with sentry_sdk.start_span(description="cache a c"): + with sentry_sdk.start_span(description="db a c"): + ... - with sentry_sdk.start_span(description="new a c"): # keep - with sentry_sdk.start_span(description="cache c"): # drop - with sentry_sdk.start_span(description="db c"): # drop - ... + with sentry_sdk.start_span(description="new a c"): # keep + with sentry_sdk.start_span(description="cache c"): + with sentry_sdk.start_span(description="db c"): + ... assert len(envelopes) == 2 (envelope1, envelope2) = envelopes @@ -150,7 +134,7 @@ def keep_only_a(sampling_context): assert transaction1["transaction"] == "request a" assert len(transaction1["spans"]) == 2 assert transaction2["transaction"] == "new a c" - assert len(transaction2["spans"]) == 0 + assert len(transaction2["spans"]) == 2 def test_sampling_traces_sampler_boolean(sentry_init, capture_envelopes): @@ -168,13 +152,13 @@ def keep_only_a(sampling_context): envelopes = capture_envelopes() with sentry_sdk.start_span(description="request a"): # keep - with sentry_sdk.start_span(description="cache a"): # keep - with sentry_sdk.start_span(description="db X"): # drop + with sentry_sdk.start_span(description="cache a"): + with sentry_sdk.start_span(description="db X"): ... with sentry_sdk.start_span(description="request b"): # drop - with sentry_sdk.start_span(description="cache b"): # drop - with sentry_sdk.start_span(description="db b"): # drop + with sentry_sdk.start_span(description="cache b"): + with sentry_sdk.start_span(description="db b"): ... assert len(envelopes) == 1 @@ -182,7 +166,7 @@ def keep_only_a(sampling_context): transaction = envelope.items[0].payload.json assert transaction["transaction"] == "request a" - assert len(transaction["spans"]) == 1 + assert len(transaction["spans"]) == 2 @pytest.mark.parametrize( @@ -237,21 +221,24 @@ def test_sampling_parent_sampled( @pytest.mark.parametrize( - "traces_sample_rate, expected_num_of_envelopes", + "traces_sample_rate, upstream_sampled, expected_num_of_envelopes", [ # special case for testing, do not pass any traces_sample_rate to init() (the default traces_sample_rate=None will be used) - (-1, 0), + (-1, 0, 0), # traces_sample_rate=None means do not create new traces, and also do not continue incoming traces. So, no envelopes at all. - (None, 0), + (None, 1, 0), # traces_sample_rate=0 means do not create new traces (0% of the requests), but continue incoming traces. So envelopes will be created only if there is an incoming trace. - (0, 0), + (0, 0, 0), + (0, 1, 1), # traces_sample_rate=1 means create new traces for 100% of requests (and also continue incoming traces, of course). - (1, 1), + (1, 0, 0), + (1, 1, 1), ], ) def test_sampling_parent_dropped( sentry_init, traces_sample_rate, + upstream_sampled, expected_num_of_envelopes, capture_envelopes, ): @@ -265,7 +252,7 @@ def test_sampling_parent_dropped( # The upstream service has dropped the request headers = { - "sentry-trace": "771a43a4192642f0b136d5159a501700-1234567890abcdef-0", + "sentry-trace": f"771a43a4192642f0b136d5159a501700-1234567890abcdef-{upstream_sampled}", } with sentry_sdk.continue_trace(headers): with sentry_sdk.start_span(description="request a"):