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.

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

feat(onboarding): early exit conditions in lib-injection [backport 2.9] #10563

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

emmettbutler
Copy link
Collaborator

This pull request adds "guardrails" to the "library injection" process. These are early exit conditions from the instrumentation process intended to avoid sending any traces when undefined behavior is likely. The code makes this determination on the basis of software versions present in the application environment, both of Python packages and the Python runtime itself.

The biggest risk here is that instrumentation is disabled when it's not intended to be. I think existing tests in tests/lib-injection cover this pretty well. There's a new test added that verifies instrumentation was cancelled when an unsupported package version is present.

Contains changes from #9418 Related RFC: "[RFC] One Step Guardrails"

  • minimum package version checks

  • Testing

  • replace envvars with inject_force

  • figure out what to use instead of pkg_resources

  • replace local file path with DD_TELEMETRY_FORWARDER_PATH

  • Change(s) are motivated and described in the PR description

  • Testing strategy is described if automated tests are not included in the PR

  • Risks are described (performance impact, potential for breakage, maintainability)

  • Change is maintainable (easy to change, telemetry, documentation)

  • Library release note guidelines are followed or label changelog/no-changelog is set

  • Documentation is included (in-code, generated user docs, public corp docs)

  • Backport labels are set (if applicable)

  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

  • Title is accurate

  • All changes are related to the pull request's stated goal

  • Description motivates each change

  • Avoids breaking API changes

  • Testing strategy adequately addresses listed risks

  • Change is maintainable (easy to change, telemetry, documentation)

  • Release note makes sense to a user of the library

  • Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment

  • Backport labels are set in a manner that is consistent with the release branch maintenance
    policy

This pull request adds "guardrails" to the "library injection" process.
These are early exit conditions from the instrumentation process
intended to avoid sending any traces when undefined behavior is likely.
The code makes this determination on the basis of software versions
present in the application environment, both of Python packages and the
Python runtime itself.

The biggest risk here is that instrumentation is disabled when it's not
intended to be. I think existing tests in `tests/lib-injection` cover
this pretty well. There's a new test added that verifies instrumentation
was cancelled when an unsupported package version is present.

Contains changes from #9418
Related RFC: "[RFC] One Step Guardrails"

- [x] minimum package version checks
- [x] Testing
- [x] replace envvars with inject_force
- [x] figure out what to use instead of pkg_resources
- [x] replace local file path with `DD_TELEMETRY_FORWARDER_PATH`
- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: Emmett Butler <[email protected]>
(cherry picked from commit 0c38e09)
@emmettbutler emmettbutler enabled auto-merge (squash) September 9, 2024 14:47
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Sep 9, 2024

Datadog Report

Branch report: backport-9323-to-2.9
Commit report: d4db6ef
Test service: dd-trace-py

❌ 1 Failed (0 Known Flaky), 160353 Passed, 15474 Skipped, 8h 58m 36.28s Total duration (2h 1m 12.31s time saved)
⌛ 11 Performance Regressions

❌ Failed Tests (1)

  • test_load_new_empty_config_and_remove_targets_file_same_product - test_remoteconfiguration.py - Details

    Expand for error
     expected call not found.
     Expected: _appsec_rules_data({'asm': {'enabled': <tests.appsec.utils.Either object at 0x7f4e9817ba30>}, 'data': [{'x': 1}], 'data2': [{'y': 2}]}, None)
     Actual: _appsec_rules_data({'data': [{'x': 1}], 'data2': [{'y': 2}]}, None)
    

⌛ Performance Regressions vs Default Branch (11)

This report shows up to 5 performance regressions.

  • test_unspecified_operation_name_schematization_v1 - test_tornado_web.py 4.07s (+3.35s, +463%) - Details
  • test_unspecified_service_name_schematization_v0 - test_tornado_web.py 3.82s (+3.1s, +432%) - Details
  • test_service_name_schematization_default - test_tornado_web.py 3.89s (+3.17s, +441%) - Details
  • test_service_name_schematization_v1 - test_tornado_web.py 3.76s (+3.04s, +422%) - Details
  • test_unspecified_service_name_schematization_default - test_tornado_web.py 3.82s (+3.1s, +432%) - Details

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.15%. Comparing base (acd1bf5) to head (d4db6ef).
Report is 7 commits behind head on 2.9.

Additional details and impacted files
@@            Coverage Diff             @@
##              2.9   #10563      +/-   ##
==========================================
- Coverage   10.22%   10.15%   -0.07%     
==========================================
  Files        1258     1258              
  Lines      120899   120919      +20     
==========================================
- Hits        12356    12281      -75     
- Misses     108543   108638      +95     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Sep 9, 2024

Benchmarks

Benchmark execution time: 2024-09-09 15:48:24

Comparing candidate commit ea3ee4e in PR branch backport-9323-to-2.9 with baseline commit f20d79c in branch 2.9.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 209 metrics, 9 unstable metrics.

@emmettbutler emmettbutler merged commit 7af0d1f into 2.9 Sep 9, 2024
169 of 171 checks passed
@emmettbutler emmettbutler deleted the backport-9323-to-2.9 branch September 9, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants