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

(aws-lambda-nodejs): set NODE_OPTIONS=--enable-source-maps to environments if sourceMap: true #19067

Closed
1 of 2 tasks
yamatatsu opened this issue Feb 21, 2022 · 5 comments
Closed
1 of 2 tasks
Assignees
Labels
@aws-cdk/aws-lambda-nodejs feature-request A feature should be added or improved.

Comments

@yamatatsu
Copy link
Contributor

yamatatsu commented Feb 21, 2022

Description

Just providing sourceMap: true, error logs is put with stack trace that clearly shows the lines like at Runtime.handler (/functions/src/handlers/handler.ts:37:27) instead of meaningless stack trace like at async Runtime.handler (/var/task/index.js:73786:27) .

Use Case

In default, lambda functions deployed by NodejsFunction put error logs with meaningless stack trace /var/task/index.js:73679:5 as following:

MyError
    at new MyError (/var/task/index.js:73679:5)
    at handler (/var/task/index.js:73710:11)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at async Runtime.handler (/var/task/index.js:73786:27)

If users want to use source maps, it is needed to configure as following:

new NodejsFunction(this, 'lambda-function', {
  environment: {
    NODE_OPTIONS: '--enable-source-maps',
  },
  bundling: {
    sourceMap: true,
  },
});

But NODE_OPTIONS: '--enable-source-maps', is redundant because it is clear that sourceMap should be used when sourceMap: true was set.

Since Feb 14 2022, node v10.x cannot be used on lambda, so it is not needed that we consider about runtime that does not support NODE_OPTIONS=--enable-source-maps I think.
https://docs.aws.amazon.com/lambda/latest/dg/runtime-support-policy.html#:~:text=July%2030%2C%202021-,Feb%2014%2C%202022,-Node.js%208.10

Proposed Solution

When bundling. sourceMap is true, set NODE_OPTIONS=--enable-source-maps to environments as following.

class NodejsFunction extends lambda.Function {
  constructor(scope: Construct, id: string, props: NodejsFunctionProps = {}) {
    super(scope, id, {
      ...props,
      environment: getEnvironment(props),
    });
  }
}

function getEnvironment(props: NodejsFunctionProps): { [key: string]: string } | undefined {
  if (!props.bundling?.sourceMap) {
    return props.environment;
  }
  if (props.environment?.NODE_OPTIONS.includes('--enable-source-maps')) {
    return props.environment;
  }
  return {
    ...props.environment,
    NODE_OPTIONS: `${props.environment?.NODE_OPTIONS ?? ''} --enable-source-maps`.trim(),
  };
}

Other information

No response

Acknowledge

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@vvo
Copy link
Contributor

vvo commented Feb 28, 2022

Hey there, just wanted to say that there's a huge performance impact of using --enable-source-maps for bundled code.
https://twitter.com/vvoyer/status/1498436054851981320

@corymhall
Copy link
Contributor

@yamatatsu are you saying that if you do not provide the NODE_OPTIONS environment variable to the lambda function then the logs will not have the stack trace with .ts line numbers?

Because of the performance impact I'm hesitant to turn this on. It feels like this should just be added to the documentation and users can explicitly provide this if they really want it.

@corymhall corymhall removed the needs-triage This issue or PR still needs to be triaged. label Mar 4, 2022
@yamatatsu
Copy link
Contributor Author

@vvo Thanks for the information! I didn't know it 👀

@corymhall

are you saying that if you do not provide the NODE_OPTIONS environment variable to the lambda function then the logs will not have the stack trace with .ts line numbers?

Yup.

Because of the performance impact I'm hesitant to turn this on. It feels like this should just be added to the documentation and users can explicitly provide this if they really want it.

I agree it. And I close this issue and will open another issue.

Thank you!

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@MartinLoeper
Copy link

2024 and the performance hit is still horrendous for nodejs 18.X. In case someone is wondering if this still applies...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs feature-request A feature should be added or improved.
Projects
None yet
4 participants