fix(tracing): Fix add_query_source with modules outside of project root
#3313
Conversation
|
Hey @rominf , |
rominf
force-pushed
the
rominf-tracing-fix-logic-in_app
branch
from
2d74969
to
a6921a5
Compare
|
FYI @rominf ignore the failing AWS tests, they need to be started by us for contributions from outside our organisation. |
|
I noticed some mistakes in the first commit message in the reproduction steps. Since I've fixed them in the issue, I'll push updated commits when/if needed, to avoid extra CI runs. |
❌ 4 Tests Failed:
View the full list of 3 ❄️ flaky tests
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
rominf
force-pushed
the
rominf-tracing-fix-logic-in_app
branch
3 times, most recently
from
d8702c8
to
7ff3956
Compare
rominf
force-pushed
the
rominf-tracing-fix-logic-in_app
branch
from
7ff3956
to
6b68fd1
Compare
Preparation for: getsentry#3313 Proposed in: getsentry#3313 (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.
Preparation for: getsentry#3313 Proposed in: getsentry#3313 (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.
* chore(tracing): Refactor `tracing_utils.py` Preparation for: #3313 Proposed in: #3313 (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. * ref: Further simplify `should_be_included` logic --------- Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io>
rominf
force-pushed
the
rominf-tracing-fix-logic-in_app
branch
3 times, most recently
from
5f7042d
to
6fa39db
Compare
|
Comments have been addressed, and the code has become much simpler than in the first iteration. Thank you, @szokeasaurusrex, for asking me to split my PR into two (refactoring and actual logic change)! |
The change is required for unit testing the logic.
…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).
rominf
force-pushed
the
rominf-tracing-fix-logic-in_app
branch
from
6fa39db
to
c6ce9c7
Compare
There was a problem hiding this comment.
Hey @rominf, I am a bit confused by this PR now that you split the PR into two pieces.
Has the logic changed at all in this PR, and if so, then how? Or, is this simply a refactor of the existing logic, plus a new test?
|
Hi @szokeasaurusrex. Yes, the logic has changed. Previously when packages added in In the first commit I introduce testing suite with one test marked with In the second commit I update the program logic to make it pass and remove I believe it is easier to review the logic in the commits by comparing the requirements with the code and checking that all cases are covered, but not by looking at the diff. If you think the logic is not fully covered by tests, please tell me about my oversight – I am open to add yet another test(s) for full coverage. |
* chore(tracing): Refactor `tracing_utils.py` Preparation for: getsentry#3313 Proposed in: getsentry#3313 (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. * ref: Further simplify `should_be_included` logic --------- Co-authored-by: Daniel Szoke <daniel.szoke@sentry.io>
github-actions
bot
removed
the
Trigger: tests using secrets
Fix: #3312
Previously, when packages added in
in_app_includewere 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.The first change moves
should_be_includedlogic into the function to enable unit testing.In the second change, the logic was slightly changed to avoid these discrepancies and conform to the requirements, described in the PR with a better setting of in-app in stack frames: #1894 (comment).
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 secretslabel. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.