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 #9323

Merged
merged 101 commits into from
Jun 11, 2024

Conversation

ZStriker19
Copy link
Contributor

@ZStriker19 ZStriker19 commented May 20, 2024

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"

Checklist

  • 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.

Reviewer Checklist

  • 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

@datadog-dd-trace-py-rkomorn
Copy link

Datadog Report

Branch report: zachg/guardrails_poc
Commit report: 40a4449
Test service: dd-trace-py

✅ 0 Failed, 115914 Passed, 59925 Skipped, 2h 49m 50.84s Total duration (6h 10m 30.05s time saved)

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.27%. Comparing base (cd92beb) to head (2e80831).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9323       +/-   ##
===========================================
- Coverage   75.89%   10.27%   -65.62%     
===========================================
  Files        1315     1285       -30     
  Lines      124966   123127     -1839     
===========================================
- Hits        94837    12650    -82187     
- Misses      30129   110477    +80348     

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

@pr-commenter
Copy link

pr-commenter bot commented May 20, 2024

Benchmarks

Benchmark execution time: 2024-06-11 14:03:03

Comparing candidate commit 43bdbc3 in PR branch zachg/guardrails_poc with baseline commit 6a72268 in branch main.

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

scenario:otelspan-start

  • 🟩 max_rss_usage [-6.009MB; -5.942MB] or [-13.137%; -12.991%]

@ZStriker19 ZStriker19 force-pushed the zachg/guardrails_poc branch from 8f51944 to 4f47c1b Compare May 21, 2024 18:03
@ZStriker19 ZStriker19 force-pushed the zachg/guardrails_poc branch from f94cf14 to c3f9073 Compare May 21, 2024 23:23
@emmettbutler emmettbutler changed the title onboarding: guardrails poc feat(onboarding): early exit conditions in lib-injection May 29, 2024
@emmettbutler emmettbutler disabled auto-merge June 10, 2024 18:38
@emmettbutler emmettbutler enabled auto-merge (squash) June 10, 2024 18:41
auto-merge was automatically disabled June 10, 2024 19:04

Pull Request is not mergeable

@emmettbutler emmettbutler enabled auto-merge (squash) June 10, 2024 19:38
lib-injection/sitecustomize.py Outdated Show resolved Hide resolved
lib-injection/sitecustomize.py Show resolved Hide resolved
@emmettbutler emmettbutler merged commit 0c38e09 into main Jun 11, 2024
168 checks passed
@emmettbutler emmettbutler deleted the zachg/guardrails_poc branch June 11, 2024 14:08
github-actions bot pushed a commit that referenced this pull request Jun 11, 2024
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"

## Checklist

- [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`.

## Reviewer Checklist

- [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 pushed a commit that referenced this pull request Jun 11, 2024
…10] (#9512)

Backport 0c38e09 from #9323 to 2.10.

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"

## Checklist

- [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`.

## Reviewer Checklist

- [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: Zachary Groves <[email protected]>
Copy link
Contributor

github-actions bot commented Sep 9, 2024

The backport to 2.9 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.9 2.9
# Navigate to the new working tree
cd .worktrees/backport-2.9
# Create a new branch
git switch --create backport-9323-to-2.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0c38e09db92cceedc8affff4a217357fc2a1bcac
# Push it to GitHub
git push --set-upstream origin backport-9323-to-2.9
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.9

Then, create a pull request where the base branch is 2.9 and the compare/head branch is backport-9323-to-2.9.

emmettbutler pushed a commit that referenced this pull request Sep 9, 2024
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)
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.

6 participants