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
AWS Lambda response streaming for Lambda URLs #11907
Conversation
Any thoughts on when/if this will get merged? Also any thoughts on if its safe enough to just patch on to my serverless project? It looks relatively simple, just not sure if its missing anything I should be aware of |
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.
@grakic that looks really good! I have just a few mostly cosmetic suggestions
@@ -215,7 +215,9 @@ functions: | |||
url: true | |||
``` | |||
|
|||
Alternatively, you can configure it as an object with the `authorizer` and/or `cors` properties. The `authorizer` property can be set to `aws_iam` to enable AWS IAM authorization on your function URL. | |||
Alternatively, you can configure it as an object with the `authorizer`, `cors` and/or `invoke` properties. |
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'd word it as:
Alternatively, you can configure it as an object, and provide values for
authorizer
andinvokeMode
options
@@ -287,6 +289,16 @@ functions: | |||
allowedHeaders: null | |||
``` | |||
|
|||
The `invoke` property can be set to `response_stream` to enable streaming response. If not specified, `buffered` invoke mode is assumed. |
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.
As outlined in issue, let's stick to invokeMode
.
Let's also support AWS values literally so RESPONSE_STREAM
and BUFFERED
if (invoke) { | ||
urlResource.Properties.InvokeMode = invoke; | ||
} |
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 think simpler would be to simply contain whole resolution here as:
if (url.invoke === 'RESPONSE_STREAM) urlResource.Properties.InvokeMode = url.invoke;
@@ -0,0 +1,9 @@ | |||
'use strict'; | |||
|
|||
// eslint-disable-next-line no-undef |
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.
Instead of doing that, let's maybe add awsLambda
as global into ESLint config, for the lambda fixtures.
ESlint config is maintained in package.json
feef40c
to
420a590
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.
Thank you @grakic it looks great. I have just one final small organization suggestion and we're ready to go
package.json
Outdated
@@ -132,6 +132,14 @@ | |||
"sourceType": "module" | |||
} | |||
}, | |||
{ |
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.
Let's reuse block at L117 (it's dedicated for fixtures)
Hey guys, Im frustrated that we've had lambda response streaming and SAM has had that feature for a while and Serverless Framework has lagged behind. I could be missing something, is there another way to add response streaming to this function or do I have to wait for this PR:
I can do it in my SAM code like this:
|
I ended up applying a patch using pnpm patching. It's for serverless version 3.30.1 but you should be able to make similar changes in whatever version your using.
In your package.json: "pnpm": {
"patchedDependencies": {
"serverless@3.30.1": "patches/serverless@3.30.1.patch"
}
}, Best of luck! |
thank you ill give this a shot |
@dested Hmm I tried your approach and added it to my package json. I also tried manually updating my node_modules/.bin/serverless and ran |
@harounansari-cj @dested it looks that this PR got abandoned, are you interested in finalizing this work? |
@medikoo Can you review proposed updates and resolve conversations? |
420a590
to
41cb47e
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.
Thank you @grakic, looks great 👍
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11907 +/- ##
==========================================
+ Coverage 86.68% 86.73% +0.04%
==========================================
Files 316 316
Lines 13440 13502 +62
==========================================
+ Hits 11651 11711 +60
- Misses 1789 1791 +2
☔ View full report in Codecov by Sentry. |
So is it |
What could be causing:
Lambda Handler looks like:
serverless.yml liiks like:
|
@sainiankit It seems that you are using serverless_sdk wrapper for your Lambda function. I don't think serverless_sdk is supporting stream invoke mode, please see discussion here: #11906 (comment) |
@grakic Sorry, Please could you explain it a little:
I have serverless.yml defined Is there a work around ?
|
You could have a look at my working example: https://github.com/tobilg/serverless-duckdb/blob/main/src/functions/streamingQuery.ts |
Closes: #11906