Skip to content

chore(profiler): fix regression test for frame object garbage collection in Python 3.11#4937

Merged
Yun-Kim merged 3 commits into
DataDog:1.xfrom
Yun-Kim:yunkim/fix-regression-prof-3-11-test
Jan 19, 2023
Merged

chore(profiler): fix regression test for frame object garbage collection in Python 3.11#4937
Yun-Kim merged 3 commits into
DataDog:1.xfrom
Yun-Kim:yunkim/fix-regression-prof-3-11-test

Conversation

@Yun-Kim

@Yun-Kim Yun-Kim commented Jan 18, 2023

Copy link
Copy Markdown
Contributor

Description

#4901 added a fix to properly decrement frametype objects in Python 3.11 in our stack collector, with a regression test. We found that this regression test does not properly assert that frametype objects were all garbage collected, which this test fixes.

Specifically, the previous change was looking for only frame objects related to a function call _foo(). However, after reverting the fix from #4901 this test still passed, because all frametype objects have been garbage collected. This change makes a broader assertion instead that all frametype objects have been garbage collected, which does expectedly fail if the fix from #4901 is reverted.

Checklist

Motivation

Design

Testing strategy

Relevant issue(s)

Testing strategy

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added and follows the library release note guidelines, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Change contains telemetry where appropriate (logs, metrics, etc.).
  • Telemetry is meaningful, actionable and does not have the potential to leak sensitive data.

@Yun-Kim Yun-Kim added the changelog/no-changelog A changelog entry is not required for this PR. label Jan 18, 2023
@Yun-Kim
Yun-Kim requested a review from a team as a code owner January 18, 2023 19:31
brettlangdon
brettlangdon reviewed Jan 18, 2023
View reviewed changes
Comment thread tests/profiling/collector/test_stack.py
@codecov-commenter

codecov-commenter commented Jan 19, 2023 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.74%. Comparing base (f278483) to head (489a07e).
⚠️ Report is 1110 commits behind head on 1.x.

Files with missing lines Patch % Lines
tests/profiling/collector/test_stack.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##              1.x    #4937   +/-   ##
=======================================
  Coverage   74.73%   74.74%           
=======================================
  Files         817      817           
  Lines       63467    63465    -2     
=======================================
  Hits        47435    47435           
+ Misses      16032    16030    -2

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Yun-Kim
Yun-Kim merged commit 539f3f4 into DataDog:1.x Jan 19, 2023
@Yun-Kim
Yun-Kim deleted the yunkim/fix-regression-prof-3-11-test branch January 19, 2023 20:40
Yun-Kim added a commit to Yun-Kim/dd-trace-py that referenced this pull request Jan 23, 2023
@Yun-Kim @brettlangdon
chore(profiler): fix regression test for frame object garbage collect…
e59ca1c 
…ion in Python 3.11 (DataDog#4937)

## Description
DataDog#4901 added a fix to properly decrement frametype objects in Python 3.11
in our stack collector, with a regression test. We found that this
regression test does not properly assert that frametype objects were all
garbage collected, which this test fixes.

Specifically, the previous change was looking for only frame objects
related to a function call `_foo()`. However, after reverting the fix
from DataDog#4901 this test still passed, because all frametype objects have
been garbage collected. This change makes a broader assertion instead
that all frametype objects have been garbage collected, which does
expectedly fail if the fix from DataDog#4901 is reverted.

<!-- If this is a breaking change, explain why it is necessary. Breaking
changes must append `!` after the type/scope. See
https://ddtrace.readthedocs.io/en/stable/contributing.html for more
details. -->

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.

<!-- Copy and paste the relevant snippet based on the type of pull
request -->

<!-- START feat -->

## Motivation
<!-- Expand on why the change is required, include relevant context for
reviewers -->

## Design 
<!-- Include benefits from the change as well as possible drawbacks and
trade-offs -->

## Testing strategy
<!-- Describe the automated tests and/or the steps for manual testing.

<!-- END feat -->

<!-- START fix -->

## Relevant issue(s)
<!-- Link the pull request to any issues related to the fix. Use
keywords for links to automate closing the issues once the pull request
is merged. -->

## Testing strategy
<!-- Describe any added regression tests and/or the manual testing
performed. -->

<!-- END fix -->

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Change contains telemetry where appropriate (logs, metrics, etc.).
- [ ] Telemetry is meaningful, actionable and does not have the
potential to leak sensitive data.

Co-authored-by: Brett Langdon <[email protected]>
@Yun-Kim Yun-Kim mentioned this pull request Jan 23, 2023
Merged
12 tasks
mabdinur pushed a commit that referenced this pull request Jan 24, 2023
@Yun-Kim @brettlangdon
fix(profiler): decrement refcount given strong reference of frame obj…
99e5f4d 
…ect in python 3.11 [backport] (#4951)

## Description
Backports for #4901 and #4937 to 1.7.

<!-- If this is a breaking change, explain why it is necessary. Breaking
changes must append `!` after the type/scope. See
https://ddtrace.readthedocs.io/en/stable/contributing.html for more
details. -->

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.

<!-- Copy and paste the relevant snippet based on the type of pull
request -->

<!-- START feat -->

## Motivation
<!-- Expand on why the change is required, include relevant context for
reviewers -->

## Design 
<!-- Include benefits from the change as well as possible drawbacks and
trade-offs -->

## Testing strategy
<!-- Describe the automated tests and/or the steps for manual testing.

<!-- END feat -->

<!-- START fix -->

## Relevant issue(s)
<!-- Link the pull request to any issues related to the fix. Use
keywords for links to automate closing the issues once the pull request
is merged. -->

## Testing strategy
<!-- Describe any added regression tests and/or the manual testing
performed. -->

<!-- END fix -->

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Change contains telemetry where appropriate (logs, metrics, etc.).
- [ ] Telemetry is meaningful, actionable and does not have the
potential to leak sensitive data.

Co-authored-by: Brett Langdon <[email protected]>
emmettbutler pushed a commit that referenced this pull request Jan 30, 2023
@Yun-Kim @brettlangdon @emmettbutler
chore(profiler): fix regression test for frame object garbage collect…
284190c 
…ion in Python 3.11 (#4937)

## Description
#4901 added a fix to properly decrement frametype objects in Python 3.11
in our stack collector, with a regression test. We found that this
regression test does not properly assert that frametype objects were all
garbage collected, which this test fixes.

Specifically, the previous change was looking for only frame objects
related to a function call `_foo()`. However, after reverting the fix
from #4901 this test still passed, because all frametype objects have
been garbage collected. This change makes a broader assertion instead
that all frametype objects have been garbage collected, which does
expectedly fail if the fix from #4901 is reverted.

<!-- If this is a breaking change, explain why it is necessary. Breaking
changes must append `!` after the type/scope. See
https://ddtrace.readthedocs.io/en/stable/contributing.html for more
details. -->

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.

<!-- Copy and paste the relevant snippet based on the type of pull
request -->

<!-- START feat -->

## Motivation
<!-- Expand on why the change is required, include relevant context for
reviewers -->

## Design 
<!-- Include benefits from the change as well as possible drawbacks and
trade-offs -->

## Testing strategy
<!-- Describe the automated tests and/or the steps for manual testing.

<!-- END feat -->

<!-- START fix -->

## Relevant issue(s)
<!-- Link the pull request to any issues related to the fix. Use
keywords for links to automate closing the issues once the pull request
is merged. -->

## Testing strategy
<!-- Describe any added regression tests and/or the manual testing
performed. -->

<!-- END fix -->

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Change contains telemetry where appropriate (logs, metrics, etc.).
- [ ] Telemetry is meaningful, actionable and does not have the
potential to leak sensitive data.

Co-authored-by: Brett Langdon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettlangdon brettlangdon brettlangdon left review comments
@P403n1x87 P403n1x87 P403n1x87 approved these changes
@mabdinur mabdinur mabdinur approved these changes

Assignees

No one assigned

Labels

changelog/no-changelog A changelog entry is not required for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Yun-Kim @codecov-commenter @brettlangdon @P403n1x87 @mabdinur