Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(tracing): fix logic for setting in_app flag #1

Closed
wants to merge 5 commits into from
Closed

fix(tracing): fix logic for setting in_app flag #1

wants to merge 5 commits into from

Conversation

rominf
Copy link
Owner

@rominf rominf commented Jul 18, 2024 •
edited
Loading

Previously, in case packages added in in_app_include were installed into a location outside of the project root directory, stack frames from those packages were not marked as in_app. 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 inconsistency: traces from the same project would have different in_app flags depending on the deployment method.

Steps to reproduce (virtualenv outside of project root):

$ docker run --replace --rm --name sentry-postgres -e POSTGRES_USER=sentry -e POSTGRES_PASSWORD=sentry -d -p 5432:5432 postgres
$ distrobox create -i ubuntu:24.04 -n sentry-test-in_app_include-venv
$ distrobox enter sentry-test-in_app_include-venv
$ python3 -m venv /tmp/.venv-test-in_app_include
$ source /tmp/.venv-test-in_app_include/bin/activate
$ pytest tests/integrations/django/test_db_query_data.py::test_query_source_with_in_app_include  # FAIL

Steps to reproduce (system packages):

$ docker run --replace --rm --name sentry-postgres -e POSTGRES_USER=sentry -e POSTGRES_PASSWORD=sentry -d -p 5432:5432 postgres
$ distrobox create -i ubuntu:24.04 -n sentry-test-in_app_include-os
$ distrobox enter sentry-test-in_app_include-os
$ sudo apt install python3-django python3-pytest python3-pytest-cov python3-pytest-django python3-jsonschema python3-urllib3 python3-certifi python3-werkzeug python3-psycopg2
$ pytest tests/integrations/django/test_db_query_data.py::test_query_source_with_in_app_include  # FAIL

In this change, the logic was slightly changed to avoid these discrepancies and conform to the requirements, described in the PR with in_app flag introduction: getsentry#1894 (comment). Note that the _module_in_list function returns False if name is None, hence extra check before function call can be omitted to simplify code.

General Notes

Thank you for contributing to sentry-python!

Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.

For maintainers

Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.

Before running sensitive test suites, please carefully check the PR. Then, apply the Trigger: tests using secrets label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.

@rominf rominf force-pushed the rominf-tracing-fix-logic-in_app branch 3 times, most recently from 2568b9c to 37f5dce Compare July 18, 2024 19:12
szokeasaurusrex and others added 3 commits July 19, 2024 12:14
@szokeasaurusrex
test: fix test_installed_modules (getsentry#3309)
6f814e6
@antonpirker
Sort breadcrumbs before sending (getsentry#3307)
8e3ddf9 
Make sure our breadcrumbs are sorted by timestamp before sending to Sentry.

Fixes getsentry#3306
@szokeasaurusrex
docs: Clarify that instrumenter is internal-only (getsentry#3299)
93a3242 
Adjust docstrings of all non-deprecated functions which take an `instrumenter` parameter to state that `instrumenter` is only meant to be used by the SDK, and that it is deprecated for client code. The docstrings also inform users that `instrumenter` will be removed in the next major release.
@rominf rominf force-pushed the rominf-tracing-fix-logic-in_app branch from 37f5dce to 2d74969 Compare July 19, 2024 12:05
@rominf
test(tracing): Test add_query_source with modules outside of projec…
dd8bc39 
…t root

When packages added in `in_app_include` are installed to a location
outside of the project root directory, span from those packages are 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 results in an inconsistency: spans from the same project
are be different, depending on the deployment method.

The change extends `test_query_source_with_in_app_include` to test the
simulation of Django installed outside of the project root.

The steps to manually reproduce the issue are as follows
(case: a virtual environment created outside of the project root):
```bash
docker run --replace --rm --detach \
  --name sentry-postgres \
  --env POSTGRES_USER=sentry \
  --env POSTGRES_PASSWORD=sentry \
  --publish 5432:5432 \
  postgres
distrobox create \
  --image ubuntu:24.04 \
  --name sentry-test-in_app_include-venv
distrobox enter sentry-test-in_app_include-venv
python3 -m venv /tmp/.venv-test-in_app_include
source /tmp/.venv-test-in_app_include/bin/activate
pytest tests/integrations/django/test_db_query_data.py \
  -k test_query_source_with_in_app_include  # FAIL
```

The steps to manually reproduce the issue are as follows
(case: Django is installed through system packages):
```bash
docker run --replace --rm --detach \
  --name sentry-postgres \
  --env POSTGRES_USER=sentry \
  --env POSTGRES_PASSWORD=sentry \
  --publish 5432:5432 \
  postgres
distrobox create \
  --image ubuntu:24.04 \
  --name sentry-test-in_app_include-os
distrobox enter sentry-test-in_app_include-os
sudo apt install \
  python3-django python3-pytest python3-pytest-cov \
  python3-pytest-django python3-jsonschema python3-urllib3 \
  python3-certifi python3-werkzeug python3-psycopg2
pytest tests/integrations/django/test_db_query_data.py \
  -k test_query_source_with_in_app_include  # FAIL
```
@rominf
fix(tracing): Fix add_query_source with modules outside of project …
a6921a5 
…root

Fix: getsentry#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:
getsentry#1894 (comment).
Note that the `_module_in_list` function returns `False` if `name` is
`None` or `items` are falsy, hence extra check before function call can
be omitted to simplify code.
@rominf rominf force-pushed the rominf-tracing-fix-logic-in_app branch from 2d74969 to a6921a5 Compare July 19, 2024 12:16
@rominf rominf closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rominf @szokeasaurusrex @antonpirker