feat(instrumentation-aws-lambda): support streaming handlers #2970
Conversation
|
|
msonnb
changed the title
| this._endSpan(span, undefined, () => resolve(value)) | ||
| ); | ||
| }, | ||
| (err: Error | string) => { |
There was a problem hiding this comment.
Is it intentional that the promise fails silently in this scenario?
There was a problem hiding this comment.
I'm not fully sure what you mean 🤔 we return a new promise that rejects with the error after the span is ended, so it shouldn't fail silently? (Also I didn't change this part, just extracted it into a function)
|
cc @jj22ee (component owner) |
|
Hey, thanks for the fix. Planning to take a look this week. |
| if (this._isStreamingHandler(original)) { | ||
| const patchedHandler = this._getPatchHandler( | ||
| original, | ||
| handlerLoadStartTime | ||
| ); | ||
|
|
||
| // Streaming handlers have special symbols that we need to copy over to the patched handler. | ||
| for (const symbol of Object.getOwnPropertySymbols(original)) { | ||
| (patchedHandler as unknown as Record<symbol, unknown>)[symbol] = ( | ||
| original as unknown as Record<symbol, unknown> | ||
| )[symbol]; | ||
| } | ||
|
|
||
| return patchedHandler; | ||
| } | ||
|
|
||
| return this._getPatchHandler(original, handlerLoadStartTime); |
There was a problem hiding this comment.
| if (this._isStreamingHandler(original)) { | |
| const patchedHandler = this._getPatchHandler( | |
| original, | |
| handlerLoadStartTime | |
| ); | |
| // Streaming handlers have special symbols that we need to copy over to the patched handler. | |
| for (const symbol of Object.getOwnPropertySymbols(original)) { | |
| (patchedHandler as unknown as Record<symbol, unknown>)[symbol] = ( | |
| original as unknown as Record<symbol, unknown> | |
| )[symbol]; | |
| } | |
| return patchedHandler; | |
| } | |
| return this._getPatchHandler(original, handlerLoadStartTime); | |
| const patchedHandler = this._getPatchHandler( | |
| original, | |
| handlerLoadStartTime | |
| ); | |
| if (this._isStreamingHandler(original)) { | |
| // Streaming handlers have special symbols that we need to copy over to the patched handler. | |
| for (const symbol of Object.getOwnPropertySymbols(original)) { | |
| (patchedHandler as unknown as Record<symbol, unknown>)[symbol] = ( | |
| original as unknown as Record<symbol, unknown> | |
| )[symbol]; | |
| } | |
| } | |
| return patchedHandler; |
| // Provide a temporary awslambda polyfill for CommonJS modules during loading | ||
| // This prevents ReferenceError when modules use awslambda.streamifyResponse at load time | ||
| if (typeof globalThis.awslambda === 'undefined') { |
There was a problem hiding this comment.
Not familiar with polyfill workarounds. Wondering why this is needed inside the Lambda instrumentation? I don't see this method referenced inside this package apart from unit-tests, as your patchedStreamingHandler will just invoke original.apply as a generic way to run the streamified handler, without needing to know it is instance of awslambda.streamifyResponse.
- Is this polyfill workaround just for the unit-tests?
- Why will AWS Lambda users will not encounter this
ReferenceErrorwhen not using Lambda Instrumentation, but will encounterReferenceErrorwhen using Lambda Instrumentation?
Which problem is this PR solving?
Fixes #2827
Short description of the changes
Adds support for AWS Lambda streaming handlers by checking if the handler has the special
aws.lambda.runtime.handler.streamingsymbol.