-
Notifications
You must be signed in to change notification settings - Fork 401
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
chore(internal): bytecode wrapping context #9222
Conversation
2d92bf5
to
24e47ef
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9222 +/- ##
===========================================
- Coverage 76.17% 10.29% -65.88%
===========================================
Files 1287 1258 -29
Lines 121979 120643 -1336
===========================================
- Hits 92913 12425 -80488
- Misses 29066 108218 +79152 ☔ View full report in Codecov by Sentry. |
24e47ef
to
e62bdfd
Compare
6f33192
to
6fb1599
Compare
528268d
to
4585201
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly looks good, just some minor conversation pieces.
d0cb7b1
to
c14488c
Compare
We introduce a new mechanism of wrapping functions via a special context manager that is capable of capturing return values as well. The goal is to allow observability into the called functions, to have access to local variables on exit. This approach has the extra benefit of not introducing any extra frames in the call stack of the wrapped function.
c14488c
to
870576a
Compare
8e6e5d5
to
f8e0cfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few questions regarding this.
wc = BrokenExitWrappingContext(foo) | ||
wc.wrap() | ||
|
||
with pytest.raises(RuntimeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is a feature.
This seems to me that broken wrapping can change the function logic/return value.
Does that the current behaviour of wrapping logic? Or this is something new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to check that we get the right exception, in case __exit__
raises one. We basically test for the expected behaviour to ensure that exceptions don't get lost, or that we get the wrong one, making debugging harder
# the return value of the function. The __exit__ method is only called if the | ||
# function raises an exception. | ||
# | ||
# Because CPython 3.11 introduced zero-cost exceptions, we cannot nest try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use Python 3.11 method only to reduce complexity of the code. As I assume future versions will not go back on zero-cost exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would have to use different opcodes anyway, so this shouldn't be much of a deal. The code is already written, so there is no need to change that 🙂
We introduce a new mechanism of wrapping functions via a special context manager that is capable of capturing return values as well. The goal is to allow observability into the called functions, to have access to local variables on exit. This approach has the extra benefit of not introducing any extra frames in the call stack of the wrapped function.
Checklist
changelog/no-changelog
is set@DataDog/apm-tees
.Reviewer Checklist