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

fix(profiling): export lock acquire/release call location and variable name [backport 2.10] #10031

Merged
merged 6 commits into from
Aug 2, 2024

Conversation

taegyunkim
Copy link
Contributor

Manual backport of #9615 to 2.10

We used to export ":<line_number>" for the name of a lock. Whenever lock is initialized, we traverse the stack frame to populate these. However, we can't get the variable name of the lock as we haven't yet fully initialized and assigned it to a variable.

Now we traverse the frame whenever _acquire/_release is called to get the caller information. Also, when _acquire is called for the first time, we inspect local and global variables in the caller frame to find the name of this lock variable. With
DD_PROFILING_LOCK_NAME_INSPECT_DIR enabled, the lock profiler also inspects attributes of local and global variables to find the name of the lock. This behavior can be turned off with the flag.

Checklist

  • 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

…e name (#9615)

We used to export "<filename>:<line_number>" for the name of a lock.
Whenever lock is initialized, we traverse the stack frame to populate
these. However, we can't get the variable name of the lock as we haven't
yet fully initialized and assigned it to a variable.

Now we traverse the frame whenever `_acquire/_release` is called to get
the caller information. Also, when `_acquire` is called for the first
time, we inspect local and global variables in the caller frame to find
the name of this lock variable. With
`DD_PROFILING_LOCK_NAME_INSPECT_DIR` enabled, the lock profiler also
inspects attributes of local and global variables to find the name of
the lock. This behavior can be turned off with the flag.

- [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)
@taegyunkim taegyunkim added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 1, 2024
@taegyunkim taegyunkim marked this pull request as ready for review August 1, 2024 16:03
@taegyunkim taegyunkim requested review from a team as code owners August 1, 2024 16:03
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Aug 1, 2024

Datadog Report

Branch report: backport-9615-to-2.10
Commit report: 1197bdc
Test service: dd-trace-py

✅ 0 Failed, 391 Passed, 1199 Skipped, 33m 24.37s Total duration (47m 23.42s time saved)

@taegyunkim taegyunkim enabled auto-merge (squash) August 1, 2024 16:18
@pr-commenter
Copy link

pr-commenter bot commented Aug 1, 2024

Benchmarks

Benchmark execution time: 2024-08-02 13:05:44

Comparing candidate commit 88f94aa in PR branch backport-9615-to-2.10 with baseline commit c4d6251 in branch 2.10.

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

@taegyunkim taegyunkim added the Profiling Continous Profling label Aug 1, 2024
@taegyunkim taegyunkim closed this Aug 1, 2024
auto-merge was automatically disabled August 1, 2024 22:07

Pull request was closed

@taegyunkim taegyunkim reopened this Aug 1, 2024
@taegyunkim taegyunkim enabled auto-merge (squash) August 1, 2024 22:36
@taegyunkim taegyunkim changed the title fix(profiling): export lock acquire/release call location and variable name fix(profiling): export lock acquire/release call location and variable name [backport 2.10] Aug 2, 2024
@taegyunkim taegyunkim merged commit 337a2d5 into 2.10 Aug 2, 2024
96 of 113 checks passed
@taegyunkim taegyunkim deleted the backport-9615-to-2.10 branch August 2, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR. Profiling Continous Profling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants