From abd4baa946b0a9c79b1e21b408c9419d4568628d Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 4 Dec 2024 20:15:29 +0100 Subject: [PATCH 1/2] Fix status related tests again --- tests/integrations/opentelemetry/test_propagator.py | 13 +++++-------- tests/test_api.py | 4 ++-- tests/tracing/test_integration_tests.py | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/tests/integrations/opentelemetry/test_propagator.py b/tests/integrations/opentelemetry/test_propagator.py index b318dccdf7..ef952ea50a 100644 --- a/tests/integrations/opentelemetry/test_propagator.py +++ b/tests/integrations/opentelemetry/test_propagator.py @@ -1,6 +1,5 @@ import pytest -from unittest import mock from unittest.mock import MagicMock from opentelemetry.trace.propagation import get_current_span @@ -12,8 +11,6 @@ SENTRY_TRACE_KEY, ) from sentry_sdk.integrations.opentelemetry.propagator import SentryPropagator -from sentry_sdk.integrations.opentelemetry.span_processor import SentrySpanProcessor -from sentry_sdk.tracing_utils import Baggage @pytest.mark.forked @@ -23,7 +20,7 @@ def test_extract_no_context_no_sentry_trace_header(): Extract should return empty context. """ carrier = None - context = None + context = {} getter = MagicMock() getter.get.return_value = None @@ -144,8 +141,8 @@ def test_inject_continue_trace(sentry_init, SortedBaggage): with sentry_sdk.continue_trace(incoming_headers): with sentry_sdk.start_span(name="foo") as span: SentryPropagator().inject(carrier, setter=setter) - assert(carrier["sentry-trace"]) == f"{trace_id}-{span.span_id}-1" - assert(carrier["baggage"]) == SortedBaggage(baggage) + assert (carrier["sentry-trace"]) == f"{trace_id}-{span.span_id}-1" + assert (carrier["baggage"]) == SortedBaggage(baggage) def test_inject_head_sdk(sentry_init, SortedBaggage): @@ -156,7 +153,7 @@ def test_inject_head_sdk(sentry_init, SortedBaggage): with sentry_sdk.start_span(name="foo") as span: SentryPropagator().inject(carrier, setter=setter) - assert(carrier["sentry-trace"]) == f"{span.trace_id}-{span.span_id}-1" - assert(carrier["baggage"]) == SortedBaggage( + assert (carrier["sentry-trace"]) == f"{span.trace_id}-{span.span_id}-1" + assert (carrier["baggage"]) == SortedBaggage( f"sentry-transaction=foo,sentry-release=release,sentry-environment=production,sentry-trace_id={span.trace_id},sentry-sample_rate=1.0,sentry-sampled=true" ) diff --git a/tests/test_api.py b/tests/test_api.py index 1be69d4a84..6c81c93d21 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -91,8 +91,8 @@ def test_baggage_with_tracing_disabled(sentry_init, SortedBaggage): @pytest.mark.forked def test_baggage_with_tracing_enabled(sentry_init, SortedBaggage): sentry_init(traces_sample_rate=1.0, release="1.0.0", environment="dev") - with start_span() as span: - expected_baggage = "sentry-trace_id={},sentry-environment=dev,sentry-release=1.0.0,sentry-sample_rate=1.0,sentry-sampled={}".format( + with start_span(name="foo") as span: + expected_baggage = "sentry-transaction=foo,sentry-trace_id={},sentry-environment=dev,sentry-release=1.0.0,sentry-sample_rate=1.0,sentry-sampled={}".format( span.trace_id, "true" if span.sampled else "false" ) assert get_baggage() == SortedBaggage(expected_baggage) diff --git a/tests/tracing/test_integration_tests.py b/tests/tracing/test_integration_tests.py index 3a4bef77fb..d6f306b0fd 100644 --- a/tests/tracing/test_integration_tests.py +++ b/tests/tracing/test_integration_tests.py @@ -39,7 +39,7 @@ def test_basic(sentry_init, capture_events, sample_rate): assert span1["status"] == "internal_error" assert span1["op"] == "foo" assert span1["description"] == "foodesc" - assert "status" not in span2.get("tags", {}) + assert span2["tags"]["status"] == "ok" assert span2["op"] == "bar" assert span2["description"] == "bardesc" assert parent_span["transaction"] == "hi" From 65fbd508f45371d0b5d6a6d888819c69b3777075 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Thu, 5 Dec 2024 14:22:04 +0100 Subject: [PATCH 2/2] Move scope context init outside integration (#3850) * Move scope context init outside integration * Fix ThreadingIntegration by carrying forward span reference in (#3851) `use_scope` Since the otel context span reference is the source of truth for the current active span, we need to explicitly pass the span reference on the scope through when we use `use_scope` since we are changing context variables in the other thread. Also, * fixes some typing in the original scope * adds more types to the `contextvars_context` manager --- MIGRATION_GUIDE.md | 1 + sentry_sdk/_init_implementation.py | 2 ++ .../opentelemetry/contextvars_context.py | 32 +++++++++++++++++-- .../integrations/opentelemetry/integration.py | 13 -------- .../integrations/opentelemetry/scope.py | 12 ++++++- sentry_sdk/integrations/threading.py | 20 +++--------- sentry_sdk/scope.py | 7 ++-- sentry_sdk/tracing.py | 4 +-- tests/conftest.py | 8 +++-- .../integrations/threading/test_threading.py | 20 ++++++------ tests/test_scope.py | 6 ++-- 11 files changed, 72 insertions(+), 53 deletions(-) diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index 1c0fa76fb0..6f0aeb4510 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -138,6 +138,7 @@ Looking to upgrade from Sentry SDK 2.x to 3.x? Here's a comprehensive list of wh - `span.containing_transaction` has been removed. Use `span.root_span` instead. - `continue_from_headers`, `continue_from_environ` and `from_traceparent` have been removed, please use top-level API `sentry_sdk.continue_trace` instead. - `PropagationContext` constructor no longer takes a `dynamic_sampling_context` but takes a `baggage` object instead. +- `ThreadingIntegration` no longer takes the `propagate_hub` argument. ### Deprecated diff --git a/sentry_sdk/_init_implementation.py b/sentry_sdk/_init_implementation.py index dc235af243..74bbd9a20f 100644 --- a/sentry_sdk/_init_implementation.py +++ b/sentry_sdk/_init_implementation.py @@ -1,6 +1,7 @@ from typing import TYPE_CHECKING import sentry_sdk +from sentry_sdk.integrations.opentelemetry.scope import setup_scope_context_management if TYPE_CHECKING: from typing import Any, Optional @@ -24,6 +25,7 @@ def _init(*args, **kwargs): """ client = sentry_sdk.Client(*args, **kwargs) sentry_sdk.get_global_scope().set_client(client) + setup_scope_context_management() _check_python_deprecations() diff --git a/sentry_sdk/integrations/opentelemetry/contextvars_context.py b/sentry_sdk/integrations/opentelemetry/contextvars_context.py index b66b10d18a..8025f26ba8 100644 --- a/sentry_sdk/integrations/opentelemetry/contextvars_context.py +++ b/sentry_sdk/integrations/opentelemetry/contextvars_context.py @@ -1,3 +1,6 @@ +from typing import cast, TYPE_CHECKING + +from opentelemetry.trace import set_span_in_context from opentelemetry.context import Context, get_value, set_value from opentelemetry.context.contextvars_context import ContextVarsRuntimeContext @@ -9,25 +12,50 @@ SENTRY_USE_ISOLATION_SCOPE_KEY, ) +if TYPE_CHECKING: + from typing import Optional + from sentry_sdk.integrations.opentelemetry.scope import PotelScope + class SentryContextVarsRuntimeContext(ContextVarsRuntimeContext): def attach(self, context): # type: (Context) -> object scopes = get_value(SENTRY_SCOPES_KEY, context) + should_fork_isolation_scope = context.pop( SENTRY_FORK_ISOLATION_SCOPE_KEY, False ) + should_fork_isolation_scope = cast("bool", should_fork_isolation_scope) + should_use_isolation_scope = context.pop(SENTRY_USE_ISOLATION_SCOPE_KEY, None) + should_use_isolation_scope = cast( + "Optional[PotelScope]", should_use_isolation_scope + ) + should_use_current_scope = context.pop(SENTRY_USE_CURRENT_SCOPE_KEY, None) + should_use_current_scope = cast( + "Optional[PotelScope]", should_use_current_scope + ) - if scopes and isinstance(scopes, tuple): + if scopes: + scopes = cast("tuple[PotelScope, PotelScope]", scopes) (current_scope, isolation_scope) = scopes else: current_scope = sentry_sdk.get_current_scope() isolation_scope = sentry_sdk.get_isolation_scope() + new_context = context + if should_use_current_scope: new_scope = should_use_current_scope + + # the main case where we use use_scope is for + # scope propagation in the ThreadingIntegration + # so we need to carry forward the span reference explicitly too + span = should_use_current_scope.span + if span: + new_context = set_span_in_context(span._otel_span, new_context) + else: new_scope = current_scope.fork() @@ -40,5 +68,5 @@ def attach(self, context): new_scopes = (new_scope, new_isolation_scope) - new_context = set_value(SENTRY_SCOPES_KEY, new_scopes, context) + new_context = set_value(SENTRY_SCOPES_KEY, new_scopes, new_context) return super().attach(new_context) diff --git a/sentry_sdk/integrations/opentelemetry/integration.py b/sentry_sdk/integrations/opentelemetry/integration.py index 013575dfa7..551ef48891 100644 --- a/sentry_sdk/integrations/opentelemetry/integration.py +++ b/sentry_sdk/integrations/opentelemetry/integration.py @@ -5,14 +5,10 @@ """ from sentry_sdk.integrations import DidNotEnable, Integration -from sentry_sdk.integrations.opentelemetry.scope import setup_initial_scopes from sentry_sdk.integrations.opentelemetry.propagator import SentryPropagator from sentry_sdk.integrations.opentelemetry.span_processor import ( SentrySpanProcessor, ) -from sentry_sdk.integrations.opentelemetry.contextvars_context import ( - SentryContextVarsRuntimeContext, -) from sentry_sdk.integrations.opentelemetry.sampler import SentrySampler from sentry_sdk.utils import logger @@ -45,7 +41,6 @@ def setup_once(): "Use at your own risk." ) - _setup_scope_context_management() _setup_sentry_tracing() _patch_readable_span() # _setup_instrumentors() @@ -70,14 +65,6 @@ def sentry_patched_readable_span(self): Span._readable_span = sentry_patched_readable_span -def _setup_scope_context_management(): - # type: () -> None - import opentelemetry.context - - opentelemetry.context._RUNTIME_CONTEXT = SentryContextVarsRuntimeContext() - setup_initial_scopes() - - def _setup_sentry_tracing(): # type: () -> None provider = TracerProvider(sampler=SentrySampler()) diff --git a/sentry_sdk/integrations/opentelemetry/scope.py b/sentry_sdk/integrations/opentelemetry/scope.py index 2e12cb53d4..89da1af68c 100644 --- a/sentry_sdk/integrations/opentelemetry/scope.py +++ b/sentry_sdk/integrations/opentelemetry/scope.py @@ -2,7 +2,6 @@ from contextlib import contextmanager from opentelemetry.context import ( - Context, get_value, set_value, attach, @@ -24,6 +23,9 @@ SENTRY_USE_ISOLATION_SCOPE_KEY, TRACESTATE_SAMPLED_KEY, ) +from sentry_sdk.integrations.opentelemetry.contextvars_context import ( + SentryContextVarsRuntimeContext, +) from sentry_sdk.integrations.opentelemetry.utils import trace_state_from_baggage from sentry_sdk.scope import Scope, ScopeType from sentry_sdk.tracing import POTelSpan @@ -152,6 +154,14 @@ def setup_initial_scopes(): attach(set_value(SENTRY_SCOPES_KEY, scopes)) +def setup_scope_context_management(): + # type: () -> None + import opentelemetry.context + + opentelemetry.context._RUNTIME_CONTEXT = SentryContextVarsRuntimeContext() + setup_initial_scopes() + + @contextmanager def isolation_scope(): # type: () -> Generator[Scope, None, None] diff --git a/sentry_sdk/integrations/threading.py b/sentry_sdk/integrations/threading.py index 99bfc66611..33cdd0d0be 100644 --- a/sentry_sdk/integrations/threading.py +++ b/sentry_sdk/integrations/threading.py @@ -7,7 +7,6 @@ from sentry_sdk.utils import ( event_from_exception, capture_internal_exceptions, - logger, reraise, ) @@ -27,22 +26,10 @@ class ThreadingIntegration(Integration): identifier = "threading" - def __init__(self, propagate_hub=None, propagate_scope=True): - # type: (Optional[bool], bool) -> None - if propagate_hub is not None: - logger.warning( - "Deprecated: propagate_hub is deprecated. This will be removed in the future." - ) - - # Note: propagate_hub did not have any effect on propagation of scope data - # scope data was always propagated no matter what the value of propagate_hub was - # This is why the default for propagate_scope is True - + def __init__(self, propagate_scope=True): + # type: (bool) -> None self.propagate_scope = propagate_scope - if propagate_hub is not None: - self.propagate_scope = propagate_hub - @staticmethod def setup_once(): # type: () -> None @@ -99,7 +86,8 @@ def _run_old_run_func(): with sentry_sdk.use_scope(current_scope_to_use): return _run_old_run_func() else: - return _run_old_run_func() + with sentry_sdk.isolation_scope(): + return _run_old_run_func() return run # type: ignore diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 54e6fc8928..7083c3709c 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -26,6 +26,7 @@ SENTRY_TRACE_HEADER_NAME, NoOpSpan, Span, + POTelSpan, Transaction, ) from sentry_sdk.utils import ( @@ -669,7 +670,7 @@ def clear(self): self.clear_breadcrumbs() self._should_capture = True # type: bool - self._span = None # type: Optional[Span] + self._span = None # type: Optional[POTelSpan] self._session = None # type: Optional[Session] self._force_auto_session_tracking = None # type: Optional[bool] @@ -777,13 +778,13 @@ def set_user(self, value): @property def span(self): - # type: () -> Optional[Span] + # type: () -> Optional[POTelSpan] """Get current tracing span.""" return self._span @span.setter def span(self, span): - # type: (Optional[Span]) -> None + # type: (Optional[POTelSpan]) -> None """Set current tracing span.""" self._span = span diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 6728b9b4c9..7686dcf052 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -1280,11 +1280,11 @@ def __eq__(self, other): def __repr__(self): # type: () -> str return ( - "<%s(op=%r, description:%r, trace_id=%r, span_id=%r, parent_span_id=%r, sampled=%r, origin=%r)>" + "<%s(op=%r, name:%r, trace_id=%r, span_id=%r, parent_span_id=%r, sampled=%r, origin=%r)>" % ( self.__class__.__name__, self.op, - self.description, + self.name, self.trace_id, self.span_id, self.parent_span_id, diff --git a/tests/conftest.py b/tests/conftest.py index 18b3ec3576..a32ebd5eb1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -62,7 +62,10 @@ def benchmark(): from sentry_sdk import scope -import sentry_sdk.integrations.opentelemetry.scope as potel_scope +from sentry_sdk.integrations.opentelemetry.scope import ( + setup_scope_context_management, + setup_initial_scopes, +) @pytest.fixture(autouse=True) @@ -74,7 +77,7 @@ def clean_scopes(): scope._isolation_scope.set(None) scope._current_scope.set(None) - potel_scope.setup_initial_scopes() + setup_initial_scopes() @pytest.fixture(autouse=True) @@ -187,6 +190,7 @@ def inner(*a, **kw): kw.setdefault("transport", TestTransport()) client = sentry_sdk.Client(*a, **kw) sentry_sdk.get_global_scope().set_client(client) + setup_scope_context_management() if request.node.get_closest_marker("forked"): # Do not run isolation if the test is already running in diff --git a/tests/integrations/threading/test_threading.py b/tests/integrations/threading/test_threading.py index 0d14fae352..75b3b7eea1 100644 --- a/tests/integrations/threading/test_threading.py +++ b/tests/integrations/threading/test_threading.py @@ -35,11 +35,11 @@ def crash(): assert not events -@pytest.mark.parametrize("propagate_hub", (True, False)) -def test_propagates_hub(sentry_init, capture_events, propagate_hub): +@pytest.mark.parametrize("propagate_scope", (True, False)) +def test_propagates_scope(sentry_init, capture_events, propagate_scope): sentry_init( default_integrations=False, - integrations=[ThreadingIntegration(propagate_hub=propagate_hub)], + integrations=[ThreadingIntegration(propagate_scope=propagate_scope)], ) events = capture_events() @@ -65,25 +65,25 @@ def stage2(): assert exception["mechanism"]["type"] == "threading" assert not exception["mechanism"]["handled"] - if propagate_hub: + if propagate_scope: assert event["tags"]["stage1"] == "true" else: assert "stage1" not in event.get("tags", {}) -@pytest.mark.parametrize("propagate_hub", (True, False)) -def test_propagates_threadpool_hub(sentry_init, capture_events, propagate_hub): +@pytest.mark.parametrize("propagate_scope", (True, False)) +def test_propagates_threadpool_scope(sentry_init, capture_events, propagate_scope): sentry_init( traces_sample_rate=1.0, - integrations=[ThreadingIntegration(propagate_hub=propagate_hub)], + integrations=[ThreadingIntegration(propagate_scope=propagate_scope)], ) events = capture_events() def double(number): - with sentry_sdk.start_span(op="task", name=str(number)): + with sentry_sdk.start_span(op="task", name=str(number), only_if_parent=True): return number * 2 - with sentry_sdk.start_transaction(name="test_handles_threadpool"): + with sentry_sdk.start_span(name="test_handles_threadpool"): with futures.ThreadPoolExecutor(max_workers=1) as executor: tasks = [executor.submit(double, number) for number in [1, 2, 3, 4]] for future in futures.as_completed(tasks): @@ -91,7 +91,7 @@ def double(number): sentry_sdk.flush() - if propagate_hub: + if propagate_scope: assert len(events) == 1 (event,) = events assert event["spans"][0]["trace_id"] == event["spans"][1]["trace_id"] diff --git a/tests/test_scope.py b/tests/test_scope.py index 48b8782190..308c7bd6c5 100644 --- a/tests/test_scope.py +++ b/tests/test_scope.py @@ -15,13 +15,11 @@ ScopeType, should_send_default_pii, ) -from sentry_sdk.integrations.opentelemetry.integration import ( - _setup_scope_context_management, -) from sentry_sdk.integrations.opentelemetry.scope import ( PotelScope as Scope, use_scope, use_isolation_scope, + setup_scope_context_management, ) @@ -31,7 +29,7 @@ @pytest.fixture(autouse=True) def setup_otel_scope_management(): - _setup_scope_context_management() + setup_scope_context_management() def test_copying():