-
Notifications
You must be signed in to change notification settings - Fork 232
remove unnecessary call to AwsLambdaInstrumentor().instrument() in wrapper script #2069
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
base: main
Are you sure you want to change the base?
remove unnecessary call to AwsLambdaInstrumentor().instrument() in wrapper script #2069
Conversation
|
While removing explicit
|
@serkan-ozal Looks like the reasoning was given here in a comment which I accidentally missed: Looks like this was added by @NathanielRN However, I don't think the reasoning here is quite correct. Even IF lambda does reload the module being pointed to by (see https://docs.python.org/3/library/importlib.html#importlib.reload) Even the original approach doesn't accomplish the goal of instrumenting the lambda handler inside the wrapper script because the AWS Lambda instrumentation library is not excluded from being used by auto instrumentation. As a result, the lambda instrumentation library is called before the wrapper script is run and any subsequent attempts to instrument will be a no-op due to the short-circuiting logic inside the Python instrumentation library: I've tested these changes by building the auto instrumentation layer exactly how it is in this PR with Python 3.14, if you'd like, I can verify that this works on the older lambda runtimes. This also makes me think that we should at some point add an integration test suite that runs in AWS against all supported lambda runtimes - at least before releasing new layer versions. Let me know what you think, we can discuss details further in one of the FaaS SIG meetings. This is something I would be very happy to lead development on. |
|
@herin049 I would test with older (but currently available) Python Lambda runtimes too. AWS Lambda Python runtime clients (which includes |
|
@serkan-ozal I was able to test and manually verify that the changes work with Python |
Talked about this in the SIG meeting with @tylerbenson today as well. I have an issue open here to track this effort as well: #2079 |
Fixes: #2068