Skip to content

Conversation

@sanchda
Copy link
Contributor

@sanchda sanchda commented Jul 23, 2024

PROF-10280

Previously, Profiling was injecting its own configuration during lib-injection handling. This was actually a little brittle.

Instead, we move to a model where the profiler detects (in its normal configuration) whether the library was injected. Hopefully this is more durable and correct.

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, 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

Copy link
Contributor Author

sanchda commented Jul 23, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @sanchda and the rest of your teammates on Graphite Graphite

@github-actions
Copy link
Contributor

github-actions bot commented Jul 23, 2024

CODEOWNERS have been resolved as:

releasenotes/notes/profiling-fix-autoinject-detection-fa5035fb0c4719cc.yaml  @DataDog/apm-python
tests/lib-injection/dd-lib-python-init-test-protobuf-old/Dockerfile     @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-protobuf-old/Readme.md      @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-protobuf-old/addressbook_pb2.py  @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-protobuf-old/requirements.txt  @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-protobuf-old/test.py        @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-protobuf-old/validate_telemetry.py  @DataDog/apm-core-python
.github/workflows/lib-inject-prune.yml                                  @DataDog/python-guild @DataDog/apm-core-python
.gitlab-ci.yml                                                          @DataDog/apm-core-python
ddtrace/settings/profiling.py                                           @DataDog/profiling-python
lib-injection/sitecustomize.py                                          @DataDog/apm-core-python
pyproject.toml                                                          @DataDog/python-guild
tests/.suitespec.json                                                   @DataDog/python-guild @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-django-unsupported-package-force/Dockerfile  @DataDog/apm-core-python
tests/lib-injection/dd-lib-python-init-test-django/Dockerfile           @DataDog/apm-core-python

@sanchda sanchda marked this pull request as ready for review July 23, 2024 03:56
@sanchda sanchda requested review from a team as code owners July 23, 2024 03:56
@sanchda sanchda requested a review from brettlangdon July 23, 2024 03:56
@sanchda sanchda force-pushed the 07-23-fix_profiling_better_autoinject_detection branch from 7441ae3 to 61a2f15 Compare July 23, 2024 04:00
@sanchda sanchda requested a review from a team as a code owner July 23, 2024 04:00
@sanchda sanchda requested a review from Yun-Kim July 23, 2024 04:00
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Jul 23, 2024

Datadog Report

Branch report: 07-23-fix_profiling_better_autoinject_detection
Commit report: d896677
Test service: dd-trace-py

✅ 0 Failed, 122120 Passed, 56300 Skipped, 3h 55m 46.05s Total duration (5h 20m 19.43s time saved)

@sanchda sanchda force-pushed the 07-23-fix_profiling_better_autoinject_detection branch 3 times, most recently from de372ba to 00b1a64 Compare July 23, 2024 14:37
@sanchda sanchda requested a review from Kyle-Verhoog July 23, 2024 14:38
@sanchda sanchda force-pushed the 07-23-fix_profiling_better_autoinject_detection branch from 00b1a64 to 5a48fd7 Compare July 23, 2024 14:41
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one thought, otherwise good to me!

@sanchda sanchda force-pushed the 07-23-fix_profiling_better_autoinject_detection branch from 5a48fd7 to cf467b4 Compare July 25, 2024 12:19
@sanchda sanchda enabled auto-merge (squash) July 25, 2024 12:20
@sanchda sanchda force-pushed the 07-23-fix_profiling_better_autoinject_detection branch from f8700dc to 778da61 Compare July 25, 2024 12:29
@sanchda sanchda force-pushed the 07-23-fix_profiling_better_autoinject_detection branch from 778da61 to 1ac1a6c Compare July 25, 2024 12:31
@pr-commenter
Copy link

pr-commenter bot commented Jul 25, 2024

Benchmarks

Benchmark execution time: 2024-08-07 13:32:43

Comparing candidate commit 920c8e7 in PR branch 07-23-fix_profiling_better_autoinject_detection with baseline commit ab8588c in branch main.

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

@taegyunkim taegyunkim self-requested a review July 25, 2024 20:00
…hub.com:DataDog/dd-trace-py into 07-23-fix_profiling_better_autoinject_detection
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 10.55%. Comparing base (120ba6d) to head (d3934f4).
Report is 4 commits behind head on main.

Files Patch % Lines
ddtrace/settings/profiling.py 0.00% 19 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9904       +/-   ##
===========================================
- Coverage   73.57%   10.55%   -63.03%     
===========================================
  Files        1421     1381       -40     
  Lines      131945   129203     -2742     
===========================================
- Hits        97078    13635    -83443     
- Misses      34867   115568    +80701     

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

@sanchda sanchda merged commit d638a7b into main Aug 7, 2024
@sanchda sanchda deleted the 07-23-fix_profiling_better_autoinject_detection branch August 7, 2024 13:41
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 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-9904-to-2.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d638a7b92d1ef01b78fad6cb1d44c27543c47059
# Push it to GitHub
git push --set-upstream origin backport-9904-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-9904-to-2.9.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2024

The backport to 2.10 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.10 2.10
# Navigate to the new working tree
cd .worktrees/backport-2.10
# Create a new branch
git switch --create backport-9904-to-2.10
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d638a7b92d1ef01b78fad6cb1d44c27543c47059
# Push it to GitHub
git push --set-upstream origin backport-9904-to-2.10
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.10

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

github-actions bot pushed a commit that referenced this pull request Aug 7, 2024
Previously, Profiling was injecting its own configuration during
lib-injection handling. This was actually a little brittle.

Instead, we move to a model where the profiler detects (in its normal
configuration) whether the library was injected. Hopefully this is more
durable and correct.

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [X] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit d638a7b)
sanchda added a commit that referenced this pull request Aug 14, 2024
Backport d638a7b from #9904 to 2.11.

[PROF-10280]

Previously, Profiling was injecting its own configuration during
lib-injection handling. This was actually a little brittle.

Instead, we move to a model where the profiler detects (in its normal
configuration) whether the library was injected. Hopefully this is more
durable and correct.

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)


[PROF-10280]:
https://datadoghq.atlassian.net/browse/PROF-10280?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

Co-authored-by: David Sanchez <838104+sanchda@users.noreply.github.com>
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