fix(profiler): indirectly access frame objects for tracebacks for Python 3.11 #4895

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.

Sign up for GitHub

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

Jump to bottom

Closed

Yun-Kim wants to merge 14 commits into 1.x from yunkim/profiler-3-11-fix

Contributor

Yun-Kim commented Jan 12, 2023

Description

#4511 added support for Python 3.11 for the profiler, but missed some PyFrameObject-related upgrades required to be compatible with Python 3.11, most notably in the stack collector's traceback module. Specifically, PyFrameObjects and its member values are now lazily created/computed, which means we can no longer directly access its member values.
According to https://docs.python.org/3/whatsnew/3.11.html#pyframeobject-3-11-hiding, we need to use get functions to access its member values.

Checklist

Followed the library release note guidelines when writing a release note.
Add additional sections for feat and fix pull requests.
Library documentation and/or Datadog's documentation site is updated. Link to doc PR in description.

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.
Backports are identified and tagged with Mergifyio.

Yun-Kim force-pushed the yunkim/profiler-3-11-fix branch from a73455a to afb2b8e Compare

January 12, 2023 19:53


          Use getter functions to access frame members

3bd4a59

Yun-Kim force-pushed the yunkim/profiler-3-11-fix branch from afb2b8e to 3bd4a59 Compare

January 12, 2023 19:54

Yun-Kim commented

View reviewed changes

Contributor Author

Yun-Kim left a comment

@P403n1x87 At first glance it looks like it's only the _traceback.pyx module that works with PyFrameObjects, but to be fair this is the only obvious fix relating to Python 3.11. Would love your feedback/second look 🙇

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

Yun-Kim requested a review from P403n1x87

January 12, 2023 20:14


          Add cpython references

61dae51

Yun-Kim commented

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved


          CPython import PyFrameObject

8840ecb

jerr0328 mentioned this pull request

Python 3.11 profiling causes memory leaks #4899

Closed

P403n1x87 reviewed

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

mabdinur reviewed

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

Yun-Kim mentioned this pull request

fix(profiler): decrement refcount given strong reference of frame object in python 3.11 #4901

Merged

11 tasks

miketheman mentioned this pull request

remove ddtrace pypi/warehouse#12819

Closed

Yun-Kim force-pushed the yunkim/profiler-3-11-fix branch from ec59155 to 7bc7816 Compare

January 17, 2023 18:09


          Fix cpython import

259676f

Yun-Kim force-pushed the yunkim/profiler-3-11-fix branch 9 times, most recently from cb63bdd to d5eb8c6 Compare

January 17, 2023 21:41


          Fix C API imports

0004bd1

Yun-Kim force-pushed the yunkim/profiler-3-11-fix branch from d5eb8c6 to 0004bd1 Compare

January 17, 2023 21:45

Yun-Kim added 2 commits

January 17, 2023 16:56


          Remove typecasting argname, typecast frame into object

791c730


          Use tuple getter function and add decrefs

8f7ea30

Yun-Kim commented

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

brettlangdon reviewed

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

Yun-Kim commented

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated

    
                              lineno = PyFrame_GetLineNumber(<PyFrameObject*> frame)

                              lineno = 0 if lineno is None else lineno

                              frames.append(((<object>code).co_filename, lineno, (<object>code).co_name, _extract_class_name(frame)))

                          frame = <object>PyFrame_GetBack(<PyFrameObject*> frame)

Contributor Author

Yun-Kim Jan 17, 2023

PyFrame_GetBack() returns a strong reference. Should I be calling Py_XDECREF(frame) even though we are iterating through these references?

Member

brettlangdon Jan 18, 2023

the frame isn't actually needed anymore after L114 right? since we are creating/returning a list of tuples?

does the frame getting GC'd cause co_filename/co_name/etc to also get GC'd? are we appropriately incref/decref-ing those references?

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved


          Separate pre/post 3.11 code branches entirely

11d1344

mabdinur reviewed

View reviewed changes

releasenotes/notes/profiler-3-11-traceback-fix-4c98c3ce96833df6.yaml Outdated Show resolved Hide resolved


          Resolve type errors with PyObject / PyFrameObject

77e1561

brettlangdon reviewed

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated

    
                              frames.append(((<object>code).co_filename, lineno, (<object>code).co_name, _extract_class_name(<object>pyframe)))

                              Py_XDECREF(<PyObject*>code)

                          pyframe = PyFrame_GetBack(pyframe)

                          # FIXME: Where to Py_XDECREF(cframe)?

Member

brettlangdon Jan 19, 2023

s/cframe/pyframe/ ?

Contributor Author

Yun-Kim Jan 19, 2023

Thanks for the catch!

I am looking for some suggestions on where to call Py_XDECREF(pyframe) though. Since PyFrame_GetBack(pyframe) returns a strong reference of the PyFrameObject*, should I be decrementing the refcount for every time we call PyFrame_GetBack(pyframe)? But since we're using the result of the PyFrame_GetBack() in the while loop, I don't think I can decrement the refcount without interfering with the rest of the function.

Contributor

P403n1x87 Jan 20, 2023 •

edited

Loading

Py_XDECREF(pyframe) has to be called every time you are done with that particular pyframe reference, so within the loop, but before overwriting the variable's value. The cast to <object> is taking care of ref counting when passing values around in Cython, so as long as our ref counting for when we invoke the C API directly is correct we should be good.

ddtrace/profiling/collector/_traceback.pyx Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved


          Refactor _extract_class_name

7ad29b7

brettlangdon reviewed

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

ddtrace/profiling/collector/_traceback.pyx Outdated

Member

brettlangdon Jan 20, 2023

you might need to do something like:

cdef PyFrameObject* cur_frame = pyframe
pyframe = PyFrame_GetBack(cur_frame)
Py_XDECREF(cur_frame)

? and then a final Py_XDECREF(pyframe) after the while ?

Contributor Author

Yun-Kim Jan 20, 2023

I think this makes sense. Trying that now.

Contributor

P403n1x87 Jan 21, 2023 •

edited

Loading

I would have thought that

Py_XDECREF(pyframe)
pyframe = PyFrame_GetBack(pyframe)

is enough, since we are manipulating Python objects and hence we're holding the GIL. But @brettlangdon's suggestion works too.


          Add Py_XDECREF for _extract_class_name, cast code as <object>

0f9e710

Yun-Kim force-pushed the yunkim/profiler-3-11-fix branch from d5b0a33 to 0f9e710 Compare

January 20, 2023 20:02

Yun-Kim and others added 2 commits

January 23, 2023 14:21


          Change lineno check, add Py_XDECREF(frame) while iterating


          Merge branch '1.x' into yunkim/profiler-3-11-fix

f5c5803

mabdinur reviewed

View reviewed changes

ddtrace/profiling/collector/_traceback.pyx Outdated Show resolved Hide resolved

Contributor Author

Yun-Kim commented Jan 23, 2023

Currently running into a segmentation fault after adding the Py_XDECREF(pyframe) in pyframe_to_frames(). It appears that this segfault appears after pyframe_to_frames() is run with asyncio task frames as the argument:
https://github.com/Yun-Kim/dd-trace-py/blob/0cf1580cfa638155bb7055a1767ca9083f53c24f/ddtrace/profiling/collector/stack.pyx#L338
- https://github.com/Yun-Kim/dd-trace-py/blob/0cf1580cfa638155bb7055a1767ca9083f53c24f/ddtrace/profiling/collector/stack.pyx#L326
- https://github.com/Yun-Kim/dd-trace-py/blob/0cf1580cfa638155bb7055a1767ca9083f53c24f/ddtrace/profiling/collector/_task.pyx#L125
- https://github.com/Yun-Kim/dd-trace-py/blob/0cf1580cfa638155bb7055a1767ca9083f53c24f/ddtrace/profiling/collector/_task.pyx#L52


          Change imports to only with 3.11+

e2afa7d

Yun-Kim mentioned this pull request

chore(profiler): add known issue changelog for Python 3.11 PyFrameObject #4952

Merged

12 tasks

Yun-Kim added a commit that referenced this pull request


          chore(profiler): add known issue changelog for Python 3.11 PyFrameObj…

ac04ab6

…ect (#4952)

## Description
As #4895 has not been merged yet and we don't want to block the 1.8
release any longer, this PR adds a release note entry publishing the
known issue of the profiler running in Python 3.11 with the new
`PyFrameObject` opaque struct changes.

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

Yun-Kim mentioned this pull request

Support Python 3.11 #4149

Closed

emmettbutler pushed a commit that referenced this pull request


          chore(profiler): add known issue changelog for Python 3.11 PyFrameObj…

fb0b6e6

…ect (#4952)

## Description
As #4895 has not been merged yet and we don't want to block the 1.8
release any longer, this PR adds a release note entry publishing the
known issue of the profiler running in Python 3.11 with the new
`PyFrameObject` opaque struct changes.

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

Contributor Author

Yun-Kim commented Feb 3, 2023

@P403n1x87 Closing this PR until we can confirm that the issue persists after #5019, in which case I'll reopen this.

Yun-Kim closed this

Yun-Kim deleted the yunkim/profiler-3-11-fix branch

April 4, 2023 20:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet