-
Notifications
You must be signed in to change notification settings - Fork 196
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
Python: Allow injecting Lambda Context #1985
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
55713da
to
e5d0f8e
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
...re/amazon/smithy/rust/codegen/server/python/smithy/generators/PythonServerModuleGenerator.kt
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-http-server-python/examples/pokemon_service.py
Outdated
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
5494991
to
75bd902
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
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.
LGTM
# Inject Lambda context if service is running on Lambda | ||
# NOTE: All the values that will be injected by the framework should be wrapped with `Optional` | ||
lambda_ctx: Optional[LambdaContext] = None |
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.
Just for my understanding: if the lambda_ctx
of type Optional[LambdaContext]
is not defined here, the injection will be just avoided right?
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.
Yes that's correct
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.
Nice documentation and test coverage
@@ -195,6 +200,18 @@ def do_nothing(_: DoNothingInput) -> DoNothingOutput: | |||
def get_pokemon_species( | |||
input: GetPokemonSpeciesInput, context: Context | |||
) -> GetPokemonSpeciesOutput: | |||
if context.lambda_ctx is not None: | |||
logging.debug( | |||
"Lambda Context: %s", |
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.
Logging in python is capitalized?
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.
AFAIK there is not a consensus. I have always used capitalized logging in Python, but I don't have a reason for it.
rust-runtime/aws-smithy-http-server-python/src/context/testing.rs
Outdated
Show resolved
Hide resolved
let py_lambda_ctx = req | ||
.extensions() | ||
.get::<LambdaContext>() | ||
.cloned() |
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.
Do we want to .cloned()
this? What's the difference if we .remove::<LambdaContext>()
?
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 didn't want to use remove
because other layers might want to use LambdaContext
, is it a good practice to remove extensions?
if context.lambda_ctx is not None: | ||
logging.debug( | ||
"Lambda Context: %s", | ||
dict( |
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.
It's a shame there's no interface on the logging
package to do structured logging, just serializing this big dict seems wrong.
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.
There is, but it is something the customer should provide through their own logging format (I have used a lot JSON lines in the past).
So to be as compatible as possible, we just log the entire dict.
75bd902
to
27d25e9
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
27d25e9
to
7e063e4
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
7e063e4
to
152b8e2
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
34703cb
to
bfce15f
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
Co-authored-by: Matteo Bigoi <[email protected]>
d35d6c2
to
ccb3046
Compare
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
This PR allows users to inject Lambda context to their custom context class by type-hinting
LambdaContext
:Closes #1815
Description
This PR introduces
PyContext
class to wrap raw Python object provided by the users. At the startup we inspect raw Python object to detect injectable fields, after thatAddPyContextLayer
(a Tower layer), populatesPyContext
from incoming request (i.e. createsPyLambdaContext
fromLambdaContext
) and then addsPyContext
to request extensions, which then gets injected by the handlers and then passed to Python handler (thanks toToPyObject
implementation ofPyContext
).TODOs
Rebase Introduce anWe decided to remove feature flag because we don't have codegen support for feature flags.aws-lambda
feature. #2035 and make Lambda specific types feature gated.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.