diff --git a/ddtrace/profiling/collector/_lock.py b/ddtrace/profiling/collector/_lock.py index 8bc80767012..863e871ff56 100644 --- a/ddtrace/profiling/collector/_lock.py +++ b/ddtrace/profiling/collector/_lock.py @@ -29,6 +29,12 @@ from ddtrace.trace import Tracer +ACQUIRE_RELEASE_CO_NAMES: frozenset[str] = frozenset(["_acquire", "_release"]) +ENTER_EXIT_CO_NAMES: frozenset[str] = frozenset( + ["acquire", "release", "__enter__", "__exit__", "__aenter__", "__aexit__"] +) + + def _current_thread() -> Tuple[int, str]: thread_id: int = _thread.get_ident() return thread_id, _threading.get_thread_name(thread_id) @@ -91,7 +97,12 @@ def _acquire(self, inner_func: Callable[..., Any], *args: Any, **kwargs: Any) -> try: self._maybe_update_self_name() self._flush_sample(start, end, is_acquire=True) + except AssertionError: + if config.enable_asserts: + # AssertionError exceptions need to propagate + raise except Exception: + # Instrumentation must never crash user code pass # nosec def release(self, *args: Any, **kwargs: Any) -> Any: @@ -202,26 +213,16 @@ def _maybe_update_self_name(self) -> None: # 3: caller frame if config.enable_asserts: frame: FrameType = sys._getframe(1) - # TODO: replace dict with list - if frame.f_code.co_name not in {"_acquire", "_release"}: - raise AssertionError("Unexpected frame %s" % frame.f_code.co_name) + if frame.f_code.co_name not in ACQUIRE_RELEASE_CO_NAMES: + raise AssertionError(f"Unexpected frame in stack: '{frame.f_code.co_name}'") + frame = sys._getframe(2) - if frame.f_code.co_name not in { - "acquire", - "release", - "__enter__", - "__exit__", - "__aenter__", - "__aexit__", - }: - raise AssertionError("Unexpected frame %s" % frame.f_code.co_name) - frame = sys._getframe(3) + if frame.f_code.co_name not in ENTER_EXIT_CO_NAMES: + raise AssertionError(f"Unexpected frame in stack: '{frame.f_code.co_name}'") # First, look at the local variables of the caller frame, and then the global variables - self._self_name = self._find_self_name(frame.f_locals) or self._find_self_name(frame.f_globals) - - if not self._self_name: - self._self_name = "" + frame = sys._getframe(3) + self._self_name = self._find_self_name(frame.f_locals) or self._find_self_name(frame.f_globals) or "" class FunctionWrapper(wrapt.FunctionWrapper): diff --git a/releasenotes/notes/fixed-silent-suppression-of-AssertionError-exceptions-in-Lock-profiler-20645079ab43b23d.yaml b/releasenotes/notes/fixed-silent-suppression-of-AssertionError-exceptions-in-Lock-profiler-20645079ab43b23d.yaml new file mode 100644 index 00000000000..62d55c1c48b --- /dev/null +++ b/releasenotes/notes/fixed-silent-suppression-of-AssertionError-exceptions-in-Lock-profiler-20645079ab43b23d.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + profiling: This fix resolves an issue where AssertionError exceptions were silently suppressed in the `_acquire` method of the Lock profiler (note: this only occurs when assertions are enabled.) diff --git a/tests/profiling_v2/collector/test_threading.py b/tests/profiling_v2/collector/test_threading.py index 6a9de6fa3d9..11bfc1acfce 100644 --- a/tests/profiling_v2/collector/test_threading.py +++ b/tests/profiling_v2/collector/test_threading.py @@ -360,6 +360,70 @@ def validate_and_cleanup() -> None: validate_and_cleanup() +@pytest.mark.subprocess(env=dict(DD_PROFILING_ENABLE_ASSERTS="true")) +def test_assertion_error_raised_with_enable_asserts(): + """Ensure that AssertionError is propagated when config.enable_asserts=True.""" + import threading + + import mock + import pytest + + from ddtrace.internal.datadog.profiling import ddup + from ddtrace.profiling.collector.threading import ThreadingLockCollector + + # Initialize ddup (required before using collectors) + assert ddup.is_available, "ddup is not available" + ddup.config(env="test", service="test_asserts", version="1.0", output_filename="/tmp/test_asserts") + ddup.start() + + with ThreadingLockCollector(capture_pct=100): + lock = threading.Lock() + + # Patch _maybe_update_self_name to raise AssertionError + lock._maybe_update_self_name = mock.Mock(side_effect=AssertionError("test: unexpected frame in stack")) + + with pytest.raises(AssertionError): + # AssertionError should be propagated when enable_asserts=True + lock.acquire() + + +@pytest.mark.subprocess(env=dict(DD_PROFILING_ENABLE_ASSERTS="false")) +def test_all_exceptions_suppressed_by_default() -> None: + """ + Ensure that exceptions are silently suppressed in the `_acquire` method + when config.enable_asserts=False (default). + """ + import threading + + import mock + + from ddtrace.internal.datadog.profiling import ddup + from ddtrace.profiling.collector.threading import ThreadingLockCollector + + # Initialize ddup (required before using collectors) + assert ddup.is_available, "ddup is not available" + ddup.config(env="test", service="test_exceptions", version="1.0", output_filename="/tmp/test_exceptions") + ddup.start() + + with ThreadingLockCollector(capture_pct=100): + lock = threading.Lock() + + # Patch _maybe_update_self_name to raise AssertionError + lock._maybe_update_self_name = mock.Mock(side_effect=AssertionError("Unexpected frame in stack: 'fubar'")) + lock.acquire() + lock.release() + + # Patch _maybe_update_self_name to raise RuntimeError + lock._maybe_update_self_name = mock.Mock(side_effect=RuntimeError("Some profiling error")) + lock.acquire() + lock.release() + + # Patch _maybe_update_self_name to raise Exception + lock._maybe_update_self_name = mock.Mock(side_effect=Exception("Wut happened?!?!")) + lock.acquire() + lock.release() + + class BaseThreadingLockCollectorTest: # These should be implemented by child classes @property