Skip to content

feat: add Python 3.11 support to Dynamic Instrumentation#4854

Merged
Kyle-Verhoog merged 15 commits into
DataDog:1.xfrom
P403n1x87:feat/debugging-py311-support
Feb 3, 2023
Merged

feat: add Python 3.11 support to Dynamic Instrumentation#4854
Kyle-Verhoog merged 15 commits into
DataDog:1.xfrom
P403n1x87:feat/debugging-py311-support

Conversation

@P403n1x87

@P403n1x87 P403n1x87 commented Jan 3, 2023 •
edited by emmettbutler
Loading

Copy link
Copy Markdown
Contributor

Description

This change adds Python 3.11 support to Dynamic Instrumentation by adapting the bytecode required for injecting hooks and wrapping functions at the bytecode level. It also adapts the compiler of the DI expression language for conditional probes and expressions.

Checklist

Motivation

Python 3.11 has been released and support for it has been added in the bytecode dependency. This means that we can now add Python 3.11 support to every part of the library that directly depend on features exposed by said dependency.

Design

Python 3.11 support required consistent adaptation in the opcodes used to perform injection and wrapping. Most notably are the no-op instructions for specialisation and the new exception table for zero-cost exception handling.

Testing strategy

The existing test suite should already have enough test cases to provide a comprehensive coverage to ensure that all the necessary features have been adapted as necessary.

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.

@P403n1x87 P403n1x87 added the Dynamic Instrumentation Dynamic Instrumentation/Live Debugger label Jan 3, 2023
@P403n1x87
P403n1x87 requested review from a team as code owners January 3, 2023 12:02
@P403n1x87
P403n1x87 force-pushed the feat/debugging-py311-support branch 2 times, most recently from d4fab9a to 0dbc0b9 Compare January 3, 2023 12:10
@P403n1x87
feat: add Python 3.11 support to Dynamic Instrumentation
7e11658 
This change adds Python 3.11 support to Dynamic Instrumentation by
adapting the bytecode required for injecting hooks and wrapping
functions at the bytecode level. It also adapts the compiler of the
DI expression language for conditional probes and expressions.
@P403n1x87
P403n1x87 force-pushed the feat/debugging-py311-support branch from 0dbc0b9 to 7e11658 Compare January 3, 2023 15:16
@P403n1x87 P403n1x87 mentioned this pull request Jan 3, 2023
Closed
Yun-Kim
Yun-Kim reviewed Jan 3, 2023
View reviewed changes

@Yun-Kim Yun-Kim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM other than one small release note fix 🥳

Comment thread releasenotes/notes/feat-debugging-py311-support-2160a5590048c7c3.yaml Outdated
@P403n1x87
P403n1x87 requested a review from Yun-Kim January 4, 2023 11:17
Yun-Kim
Yun-Kim previously approved these changes Jan 5, 2023
View reviewed changes
mabdinur
mabdinur reviewed Jan 19, 2023
View reviewed changes
Comment thread releasenotes/notes/feat-debugging-py311-support-2160a5590048c7c3.yaml Outdated
majorgreys
majorgreys reviewed Jan 19, 2023
View reviewed changes
Comment thread ddtrace/internal/wrapping.py
P403n1x87
P403n1x87 commented Jan 19, 2023
View reviewed changes
Comment thread ddtrace/internal/wrapping.py
@P403n1x87
P403n1x87 force-pushed the feat/debugging-py311-support branch from 78f2117 to d74acf2 Compare January 20, 2023 18:03
@P403n1x87
P403n1x87 force-pushed the feat/debugging-py311-support branch 2 times, most recently from 5d1a5b4 to 49c02e8 Compare January 24, 2023 10:30
@P403n1x87
P403n1x87 force-pushed the feat/debugging-py311-support branch from 49c02e8 to 91586b3 Compare January 24, 2023 11:02
@Yun-Kim Yun-Kim mentioned this pull request Jan 25, 2023
Closed
emmettbutler
emmettbutler previously approved these changes Jan 27, 2023
View reviewed changes
Comment thread ddtrace/debugging/_expressions.py
Comment thread ddtrace/debugging/_expressions.py
Comment thread ddtrace/internal/injection.py
Comment thread tests/internal/py36/test_wrapping.py
Comment thread ddtrace/internal/wrapping.py
@P403n1x87
P403n1x87 requested a review from a team as a code owner January 31, 2023 11:15
@P403n1x87
P403n1x87 requested a review from majorgreys January 31, 2023 11:15
@P403n1x87
P403n1x87 requested review from a team, Yun-Kim, emmettbutler and mabdinur January 31, 2023 11:15
@codecov-commenter

codecov-commenter commented Feb 1, 2023 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 24.85207% with 127 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.90%. Comparing base (17b234b) to head (b66c9c0).
⚠️ Report is 1051 commits behind head on 1.x.

Files with missing lines Patch % Lines
tests/internal/test_wrapping.py 0.00% 50 Missing ⚠️
tests/internal/py36/test_wrapping.py 0.00% 49 Missing ⚠️
ddtrace/internal/wrapping.py 42.55% 27 Missing ⚠️
tests/internal/test_injection.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##              1.x    #4854      +/-   ##
==========================================
- Coverage   74.98%   74.90%   -0.09%     
==========================================
  Files         827      827              
  Lines       64051    64170     +119     
==========================================
+ Hits        48031    48064      +33     
- Misses      16020    16106      +86

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

@Kyle-Verhoog
Kyle-Verhoog enabled auto-merge (squash) February 2, 2023 03:31
brettlangdon
brettlangdon reviewed Feb 2, 2023
View reviewed changes

@brettlangdon brettlangdon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mabdinur since this modifies the bytecode wrapping code that we use for GraphQL is there any additional testing/concerns we should check/test for this?

That seems to be the "major" impact this change would have cross-product. Otherwise, most changes are isolated to DI.

@mabdinur

mabdinur commented Feb 2, 2023

Copy link
Copy Markdown
Contributor

@mabdinur since this modifies the bytecode wrapping code that we use for GraphQL is there any additional testing/concerns we should check/test for this?

That seems to be the "major" impact this change would have cross-product. Otherwise, most changes are isolated to DI.

We'll need to update our riot configurations to run graphql and graphene tests with py3.11:

dd-trace-py/riotfile.py

Line 1803 in 8a9fec4

name="graphql",
. If those tests pass we should be good. I'll add that as a follow up PR

@brettlangdon

brettlangdon commented Feb 2, 2023

Copy link
Copy Markdown
Member

@mabdinur since this modifies the bytecode wrapping code that we use for GraphQL is there any additional testing/concerns we should check/test for this?
That seems to be the "major" impact this change would have cross-product. Otherwise, most changes are isolated to DI.

We'll need to update our riot configurations to run graphql and graphene tests with py3.11:

dd-trace-py/riotfile.py

Line 1803 in 8a9fec4

name="graphql",

. If those tests pass we should be good. I'll add that as a follow up PR

Is there any manual testing Gab (or you) could do to confirm this PR is ok to merge?

I'd hate to merge this code, then find out it messed up GraphQL wrapping code after.

@mabdinur

mabdinur commented Feb 2, 2023

Copy link
Copy Markdown
Contributor

I ran graphql-core and graphene tests with py3.11 and the changes in this branch, and most of the tests passed ( uses an itertable that was removed in py3.11). The failing tests were due graphql-core<2.3 using a deprecated type.

I think we should be good to merge this change.

@Yun-Kim Yun-Kim mentioned this pull request Feb 3, 2023
Merged
13 tasks
@Kyle-Verhoog
Kyle-Verhoog merged commit 51f20e2 into DataDog:1.x Feb 3, 2023
@mabdinur mabdinur mentioned this pull request Feb 6, 2023
Closed
13 tasks
emmettbutler pushed a commit that referenced this pull request Feb 6, 2023
@Yun-Kim
chore(graphene,graphql): add Python 3.11 testing for graphene/graphql…
8c1d99a 
… tests (#5037)

## Checklist
Blocked by #4854.

This change adds Python 3.11 testing to our graphql and graphene test
suites.

- [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] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
are followed.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).


## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
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
@majorgreys majorgreys majorgreys approved these changes
@emmettbutler emmettbutler emmettbutler approved these changes
@mabdinur mabdinur mabdinur approved these changes
@Yun-Kim Yun-Kim Awaiting requested review from Yun-Kim

Assignees

No one assigned

Labels

Dynamic Instrumentation Dynamic Instrumentation/Live Debugger

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@P403n1x87 @codecov-commenter @mabdinur @brettlangdon @majorgreys @emmettbutler @Yun-Kim @Kyle-Verhoog