Skip to content

feat: add Python 3.11 support to Dynamic Instrumentation #4854

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

Merged
merged 15 commits into from
Feb 3, 2023

Conversation

P403n1x87
Copy link
Contributor

@P403n1x87 P403n1x87 commented Jan 3, 2023

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
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.
Copy link
Contributor

@Yun-Kim Yun-Kim left a comment

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 🥳

@P403n1x87 P403n1x87 requested a review from Yun-Kim January 4, 2023 11:17
Yun-Kim
Yun-Kim previously approved these changes Jan 5, 2023
@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
emmettbutler
emmettbutler previously approved these changes Jan 27, 2023
@P403n1x87 P403n1x87 requested review from Yun-Kim, mabdinur, emmettbutler and a team January 31, 2023 11:15
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Merging #4854 (c7bab68) into 1.x (a6ff0c3) will decrease coverage by 0.10%.
The diff coverage is 24.85%.

@@            Coverage Diff             @@
##              1.x    #4854      +/-   ##
==========================================
- Coverage   74.95%   74.85%   -0.10%     
==========================================
  Files         824      824              
  Lines       63924    64043     +119     
==========================================
+ Hits        47914    47942      +28     
- Misses      16010    16101      +91     
Impacted Files Coverage Δ
ddtrace/debugging/_debugger.py 85.67% <ø> (+0.65%) ⬆️
tests/internal/py35/test_wrapping.py 0.00% <ø> (ø)
tests/internal/py36/test_wrapping.py 0.00% <0.00%> (ø)
tests/internal/test_injection.py 0.00% <0.00%> (ø)
tests/internal/test_wrapping.py 0.00% <0.00%> (ø)
ddtrace/internal/wrapping.py 75.81% <42.55%> (-1.05%) ⬇️
ddtrace/debugging/_expressions.py 79.88% <100.00%> (+0.22%) ⬆️
ddtrace/internal/injection.py 77.14% <100.00%> (+2.14%) ⬆️
ddtrace/internal/utils/inspection.py 100.00% <100.00%> (ø)
tests/debugging/test_expressions.py 100.00% <100.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Kyle-Verhoog Kyle-Verhoog enabled auto-merge (squash) February 2, 2023 03:31
Copy link
Member

@brettlangdon brettlangdon left a comment

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
Copy link
Contributor

mabdinur commented Feb 2, 2023

@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:

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

@brettlangdon
Copy link
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:

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
Copy link
Contributor

mabdinur commented Feb 2, 2023

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.

@Kyle-Verhoog Kyle-Verhoog merged commit 51f20e2 into DataDog:1.x Feb 3, 2023
emmettbutler pushed a commit that referenced this pull request Feb 6, 2023
… 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
Labels
Dynamic Instrumentation Dynamic Instrumentation/Live Debugger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants