Skip to content

Commit

Permalink
fix(tracing): Fix add_query_source with modules outside of project …
Browse files Browse the repository at this point in the history
…root (#3313)

Fix: #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:
#1894 (comment).

---------

Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io>
Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>
  • Loading branch information
3 people authored Oct 1, 2024
1 parent aed18d4 commit 4636afc
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 13 deletions.
42 changes: 29 additions & 13 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,26 @@ def _get_frame_module_abs_path(frame):
return None


def _should_be_included(
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(
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)
)


def add_query_source(span):
# type: (sentry_sdk.tracing.Span) -> None
"""
Expand Down Expand Up @@ -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
Expand Down
96 changes: 96 additions & 0 deletions tests/test_tracing_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
from dataclasses import asdict, dataclass
from typing import Optional, List

from sentry_sdk.tracing_utils import _should_be_included
import pytest


def id_function(val):
# type: (object) -> 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,
),
(
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,
),
],
ids=id_function,
)
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")
assert _should_be_included(**kwargs) == expected

0 comments on commit 4636afc

Please sign in to comment.