Skip to content
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

Path is removed from Handler #5429

Closed
ruiwei opened this issue Jun 29, 2023 · 15 comments
Closed

Path is removed from Handler #5429

ruiwei opened this issue Jun 29, 2023 · 15 comments
Labels
stage/waiting-for-release Fix has been merged to develop and is waiting for a release type/bug

Comments

@ruiwei
Copy link

ruiwei commented Jun 29, 2023

Description:

I'm using the "datadg-lambda-js" as a layer.
Handler: /opt/nodejs/node_modules/datadog-lambda-js/handler.handler
The latest version 1.89.0 removed the path and changed it to just "handler.handler"
Version 1.71.0 works

Additional environment details (Ex: Windows, Mac, Amazon Linux etc)

  1. OS: Linux
  2. sam --version: 1.89.0
  3. AWS region: us-east-1
@ruiwei ruiwei added the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Jun 29, 2023
@hnnasit
Copy link
Contributor

hnnasit commented Jun 29, 2023

Hi @ruiwei, could you elaborate more on the issue and provide example/steps for us to help further?

@hnnasit hnnasit added blocked/more-info-needed More info is needed from the requester. If no response in 14 days, it will become stale. and removed stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Jun 29, 2023
@ruiwei
Copy link
Author

ruiwei commented Jun 30, 2023

This is the partial of my template.yml.

Parameters:
  DataDogLayers:
    Description: DataDog layers 
    Type: CommaDelimitedList 
    Default: "arn:aws:lambda:us-east-1:464622532012:layer:Datadog-Node16-x:93, arn:aws:lambda:us-east-1:464622532012:layer:Datadog-Extension:44"   

Resources:
  functionName:
    Type: AWS::Serverless::Function
    Properties:
      Policies: !FindInMap [EnvTypeMap, !Ref EnvType, LambdaPolicyArn]
      Handler: /opt/nodejs/node_modules/datadog-lambda-js/handler.handler
      Environment: 
        Variables:
          DD_LAMBDA_HANDLER: src/handlers/institutions.handler
      Timeout: 180
      FunctionName: !Sub ${AWS::StackName}-institutions
      Layers: !Ref DataDogLayers
    Metadata:
      BuildMethod: esbuild
      BuildProperties:
        Minify: true
        Target: es2020
        Sourcemap: true
        EntryPoints:
          - src/handlers/institutions.ts  	

After deployed by version 1.89.0, "/opt/nodejs/node_modules/datadog-lambda-js/handler.handler" changed to just "handler.handler".

@mildaniel
Copy link
Contributor

Hi @ruiwei, thanks for reporting. Looks like you have been affected by this change #4216. The rationale behind that change was to address an issue users faced when nesting their handler more than one directory deep when using esbuild. Since the bundler gives us one or two artifacts in the root of the handler's output directory, the original folder structure isn't preserved. As such, we need to change the handler path to point to these artifacts.

With your example, the handler is pointing to the layer code location and not the function source, inadvertently affecting your use case.

As a workaround, can you try applying the Datadog wrapper in the function code instead as documented here: https://docs.datadoghq.com/serverless/guide/handler_wrapper/

@ruiwei
Copy link
Author

ruiwei commented Jul 4, 2023

Hey @mildaniel, Thanks for looking. I can try to use the wrapper in the code. I will notify DataDog about this issue.

@mildaniel
Copy link
Contributor

Hey @ruiwei, I don't think there's anything that Datadog needs to be notified of here.

In terms of switching to the code wrapper, this is probably the better choice too in terms of performance since you will also get the benefits of esbuild tree-shaking for the dependency as well.

@astuyve
Copy link

astuyve commented Jul 5, 2023

Hi @ruiwei - this appears to be a breaking change, correct?

I'm still working on reproducing the issue, but I know from past experience that a missing handler will crash the function entirely.

Based on this, wouldn't anyone upgrading to SAM 1.79 or greater will experience an issue where all of their functions immediately crash?

I can report back when I reproduce.

Thanks!

@ruiwei
Copy link
Author

ruiwei commented Jul 5, 2023

@astuyve I believe this is a breaking change. I'm not sure about the version above 1.79. I can confirm it is working with v1.71 and broken in 1.89.

      - name: setup sam
        uses: aws-actions/setup-sam@v2
        with:
          use-installer: true
          version: "1.71.0"

@astuyve
Copy link

astuyve commented Jul 5, 2023

Okay thanks for the context @ruiwei - that's very helpful.

I suggest 1.79 as @mildaniel mentioned #4216 likely introduced this change, and that was released in 1.79, but I have not had time to reproduce this yet.

@mildaniel
Copy link
Contributor

mildaniel commented Jul 5, 2023

Just to clarify, this is only impacting customers using the layer handler wrapper configuration of Datadog with esbuild.

There is another issue (#4082) which contradicts this one in that it requires the handler path to be updated so that it points to the correct build artifacts for esbuild functions. This use case requires the handler field to not be changed. The team is looking at ways of addressing both problems.

In the meantime, I still recommend using the function code integration as a fix. This type of integration should work (and potentially improve performance).

@mildaniel mildaniel added type/bug and removed blocked/more-info-needed More info is needed from the requester. If no response in 14 days, it will become stale. labels Jul 5, 2023
@astuyve
Copy link

astuyve commented Jul 5, 2023

Hi @mildaniel, thanks for replying!

Just to clarify, this is only impacting customers using the layer handler wrapper configuration of Datadog with esbuild.

It appears as though this would impact anyone using a layer as handler? This pattern is well established within the community, and is even used by AWS in the CodeGuru for Python.

I don't know if there's necessarily a contradiction between this and #4082, rather multiple customer requirements must be considered.

Would a fix for this issue be to consider the handler path with a bit more discretion?

I don't believe a handler in a layer would be considered a candidate for transpiling/bundling with esbuild as that code is only available at runtime (unless you pull all the code from the layer and then re-build the project, which may solve other issues including dependency squishing in Lambda, but I digress).

Perhaps this could be solved by only remapping the handler if it exists in the transpiled file?

Or even more simply by skipping the handler re-write logic if the handler path is rooted in /opt?

I'd prefer if we could find a fix for this issue in SAM, as opposed to asking all customers to modify their code due to this change which breaks their applications.

Thanks!

@mndeveci
Copy link
Contributor

mndeveci commented Jul 5, 2023

It appears as though this would impact anyone using a layer as handler?

Only the customers who is using esbuild to build their lambda functions.

We are started working on a fix now, we will resolve this issue once that is released. Thanks!

@mildaniel
Copy link
Contributor

A fix has been merged and will be distributed in an upcoming release.

@astuyve
Copy link

astuyve commented Jul 6, 2023

Thanks so much @mndeveci and @mildaniel!

@mndeveci mndeveci added the stage/waiting-for-release Fix has been merged to develop and is waiting for a release label Jul 7, 2023
@github-actions
Copy link
Contributor

Patch is released in v1.91.0. Closing

@astuyve
Copy link

astuyve commented Jul 18, 2023

Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/waiting-for-release Fix has been merged to develop and is waiting for a release type/bug
Projects
None yet
Development

No branches or pull requests

5 participants