From e3bb1c0c8fd1cf8349bb27dc5d8ed4de00d7fbfd Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Tue, 27 Aug 2024 20:10:39 +0300 Subject: [PATCH 1/3] ref(tracing): Move `should_be_included` logic into function The change is required for unit testing the logic. --- sentry_sdk/tracing_utils.py | 46 +++++++++++------ tests/test_tracing_utils.py | 99 +++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 15 deletions(-) create mode 100644 tests/test_tracing_utils.py diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index 0df1ae5bd4..27f11ff226 100644 --- a/sentry_sdk/tracing_utils.py +++ b/sentry_sdk/tracing_utils.py @@ -25,13 +25,12 @@ _module_in_list, ) -from typing import TYPE_CHECKING +from typing import List, Optional, TYPE_CHECKING if TYPE_CHECKING: from typing import Any from typing import Dict from typing import Generator - from typing import Optional from typing import Union from types import FrameType @@ -180,6 +179,27 @@ def _get_frame_module_abs_path(frame): return None +def _should_be_included( + is_sentry_sdk_frame: bool, + namespace: Optional[str], + in_app_include: Optional[List[str]], + in_app_exclude: Optional[List[str]], + abs_path: Optional[str], + project_root: Optional[str], +) -> bool: + # in_app_include takes precedence over in_app_exclude + should_be_included = ( + not ( + _is_external_source(abs_path) or _module_in_list(namespace, in_app_exclude) + ) + ) or _module_in_list(namespace, in_app_include) + return ( + _is_in_project_root(abs_path, project_root) + and should_be_included + and not is_sentry_sdk_frame + ) + + def add_query_source(span): # type: (sentry_sdk.tracing.Span) -> None """ @@ -221,19 +241,15 @@ def add_query_source(span): "sentry_sdk." ) - # in_app_include takes precedence over in_app_exclude - should_be_included = ( - not ( - _is_external_source(abs_path) - or _module_in_list(namespace, in_app_exclude) - ) - ) or _module_in_list(namespace, in_app_include) - - if ( - _is_in_project_root(abs_path, project_root) - and should_be_included - and not is_sentry_sdk_frame - ): + should_be_included = _should_be_included( + is_sentry_sdk_frame=is_sentry_sdk_frame, + namespace=namespace, + in_app_include=in_app_include, + in_app_exclude=in_app_exclude, + abs_path=abs_path, + project_root=project_root, + ) + if should_be_included: break frame = frame.f_back diff --git a/tests/test_tracing_utils.py b/tests/test_tracing_utils.py new file mode 100644 index 0000000000..c31d4dcf5f --- /dev/null +++ b/tests/test_tracing_utils.py @@ -0,0 +1,99 @@ +from dataclasses import asdict, dataclass +from typing import Optional, List, Any + +from sentry_sdk.tracing_utils import _should_be_included +import pytest + + +def id_function(val: Any) -> str: + if isinstance(val, ShouldBeIncludedTestCase): + return val.id + + +@dataclass(frozen=True) +class ShouldBeIncludedTestCase: + id: str + is_sentry_sdk_frame: bool + namespace: Optional[str] = None + in_app_include: Optional[List[str]] = None + in_app_exclude: Optional[List[str]] = None + abs_path: Optional[str] = None + project_root: Optional[str] = None + + +@pytest.mark.parametrize( + "test_case, expected", + [ + ( + ShouldBeIncludedTestCase( + id="Frame from Sentry SDK", + is_sentry_sdk_frame=True, + ), + False, + ), + ( + ShouldBeIncludedTestCase( + id="Frame from Django installed in virtualenv inside project root", + is_sentry_sdk_frame=False, + abs_path="/home/username/some_project/.venv/lib/python3.12/site-packages/django/db/models/sql/compiler", + project_root="/home/username/some_project", + namespace="django.db.models.sql.compiler", + in_app_include=["django"], + ), + True, + ), + ( + ShouldBeIncludedTestCase( + id="Frame from project", + is_sentry_sdk_frame=False, + abs_path="/home/username/some_project/some_project/__init__.py", + project_root="/home/username/some_project", + namespace="some_project", + ), + True, + ), + ( + ShouldBeIncludedTestCase( + id="Frame from project module in `in_app_exclude`", + is_sentry_sdk_frame=False, + abs_path="/home/username/some_project/some_project/exclude_me/some_module.py", + project_root="/home/username/some_project", + namespace="some_project.exclude_me.some_module", + in_app_exclude=["some_project.exclude_me"], + ), + False, + ), + ( + ShouldBeIncludedTestCase( + id="Frame from system-wide installed Django", + is_sentry_sdk_frame=False, + abs_path="/usr/lib/python3.12/site-packages/django/db/models/sql/compiler", + project_root="/home/username/some_project", + namespace="django.db.models.sql.compiler", + ), + False, + ), + pytest.param( + ShouldBeIncludedTestCase( + id="Frame from system-wide installed Django with `django` in `in_app_include`", + is_sentry_sdk_frame=False, + abs_path="/usr/lib/python3.12/site-packages/django/db/models/sql/compiler", + project_root="/home/username/some_project", + namespace="django.db.models.sql.compiler", + in_app_include=["django"], + ), + True, + marks=pytest.mark.xfail( + reason="Bug, see https://github.com/getsentry/sentry-python/issues/3312" + ), + ), + ], + ids=id_function, +) +def test_should_be_included( + test_case: ShouldBeIncludedTestCase, expected: bool +) -> None: + """Checking logic, see: https://github.com/getsentry/sentry-python/issues/3312""" + kwargs = asdict(test_case) + kwargs.pop("id") + assert _should_be_included(**kwargs) == expected From c6ce9c7cf866b42cc2dfbd6aec3acb38c8d6e47d Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Thu, 18 Jul 2024 16:57:21 +0300 Subject: [PATCH 2/3] fix(tracing): Fix `add_query_source` with modules outside of project root Fix: https://github.com/getsentry/sentry-python/issues/3312 Previously, when packages added in `in_app_include` were installed to a location outside of the project root directory, span from those packages were not extended with OTel compatible source code information. Cases include running Python from virtualenv created outside of the project root directory or Python packages installed into the system using package managers. This resulted in an inconsistency: spans from the same project would be different, depending on the deployment method. In this change, the logic was slightly changed to avoid these discrepancies and conform to the requirements, described in the PR with better setting of in-app in stack frames: https://github.com/getsentry/sentry-python/pull/1894#issue-1579192436. --- sentry_sdk/tracing_utils.py | 16 +++++++--------- tests/test_tracing_utils.py | 5 +---- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index 27f11ff226..562b1a961d 100644 --- a/sentry_sdk/tracing_utils.py +++ b/sentry_sdk/tracing_utils.py @@ -188,15 +188,13 @@ def _should_be_included( project_root: Optional[str], ) -> bool: # in_app_include takes precedence over in_app_exclude - should_be_included = ( - not ( - _is_external_source(abs_path) or _module_in_list(namespace, in_app_exclude) - ) - ) or _module_in_list(namespace, in_app_include) - return ( - _is_in_project_root(abs_path, project_root) - and should_be_included - and not is_sentry_sdk_frame + should_be_included = _module_in_list(namespace, in_app_include) + should_be_excluded = _is_external_source(abs_path) or _module_in_list( + namespace, in_app_exclude + ) + return not is_sentry_sdk_frame and ( + should_be_included + or (_is_in_project_root(abs_path, project_root) and not should_be_excluded) ) diff --git a/tests/test_tracing_utils.py b/tests/test_tracing_utils.py index c31d4dcf5f..6e15a4db66 100644 --- a/tests/test_tracing_utils.py +++ b/tests/test_tracing_utils.py @@ -73,7 +73,7 @@ class ShouldBeIncludedTestCase: ), False, ), - pytest.param( + ( ShouldBeIncludedTestCase( id="Frame from system-wide installed Django with `django` in `in_app_include`", is_sentry_sdk_frame=False, @@ -83,9 +83,6 @@ class ShouldBeIncludedTestCase: in_app_include=["django"], ), True, - marks=pytest.mark.xfail( - reason="Bug, see https://github.com/getsentry/sentry-python/issues/3312" - ), ), ], ids=id_function, From b7c382f4c084f4c87620af39850bd5b6b1847e6c Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Mon, 23 Sep 2024 11:03:40 +0200 Subject: [PATCH 3/3] ref(tracing_utils): Use type comments instead of type annotations --- sentry_sdk/tracing_utils.py | 18 ++++++++++-------- tests/test_tracing_utils.py | 10 +++++----- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index 562b1a961d..fe9fe8ec36 100644 --- a/sentry_sdk/tracing_utils.py +++ b/sentry_sdk/tracing_utils.py @@ -25,12 +25,13 @@ _module_in_list, ) -from typing import List, Optional, TYPE_CHECKING +from typing import TYPE_CHECKING if TYPE_CHECKING: from typing import Any from typing import Dict from typing import Generator + from typing import Optional from typing import Union from types import FrameType @@ -180,13 +181,14 @@ def _get_frame_module_abs_path(frame): def _should_be_included( - is_sentry_sdk_frame: bool, - namespace: Optional[str], - in_app_include: Optional[List[str]], - in_app_exclude: Optional[List[str]], - abs_path: Optional[str], - project_root: Optional[str], -) -> bool: + is_sentry_sdk_frame, # type: bool + namespace, # type: Optional[str] + in_app_include, # type: Optional[list[str]] + in_app_exclude, # type: Optional[list[str]] + abs_path, # type: Optional[str] + project_root, # type: Optional[str] +): + # type: (...) -> bool # in_app_include takes precedence over in_app_exclude should_be_included = _module_in_list(namespace, in_app_include) should_be_excluded = _is_external_source(abs_path) or _module_in_list( diff --git a/tests/test_tracing_utils.py b/tests/test_tracing_utils.py index 6e15a4db66..239e631156 100644 --- a/tests/test_tracing_utils.py +++ b/tests/test_tracing_utils.py @@ -1,11 +1,12 @@ from dataclasses import asdict, dataclass -from typing import Optional, List, Any +from typing import Optional, List from sentry_sdk.tracing_utils import _should_be_included import pytest -def id_function(val: Any) -> str: +def id_function(val): + # type: (object) -> str if isinstance(val, ShouldBeIncludedTestCase): return val.id @@ -87,9 +88,8 @@ class ShouldBeIncludedTestCase: ], ids=id_function, ) -def test_should_be_included( - test_case: ShouldBeIncludedTestCase, expected: bool -) -> None: +def test_should_be_included(test_case, expected): + # type: (ShouldBeIncludedTestCase, bool) -> None """Checking logic, see: https://github.com/getsentry/sentry-python/issues/3312""" kwargs = asdict(test_case) kwargs.pop("id")