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

Exclude optional performance instrumentation by default for AWS Serverless #11991

Closed
jd-carroll opened this issue May 12, 2024 · 7 comments · Fixed by #11998
Closed

Exclude optional performance instrumentation by default for AWS Serverless #11991

jd-carroll opened this issue May 12, 2024 · 7 comments · Fixed by #11998

Comments

@jd-carroll
Copy link

Problem Statement

Opentelemetry adds a massive amount of overhead. The screen shot below represents a minimized / treeshook lambda built with esbuild that has very little of its own code.
image

I understand some of the challenges here (ie. nothing is free or magic), but Sentry + Opentelemetry is > 50% of the bundled lambda.

For opentelemetry specifically, it bundles a whole series of instrumentation hooks which will never be consumed.
image

Of the 16 bundled instrumentation hooks, we will only use three of them:

  • instrumentation-aws-sdk
  • instrumentation-http
  • instrumentation-aws-lambda

There is a guide which mentions "customizing" the opentelemetry configuration:
https://docs.sentry.io/platforms/javascript/guides/node/migration/v7-to-v8/v8-opentelemetry/#custom-opentelemetry-setup

But it seems incomplete. It is not clear how or where I would instantiate the three instrumentation providers.

Solution Brainstorm

It would be great if there was a complete example of an aws lambda which configures only three instrumentation providers:

  • instrumentation-aws-sdk
  • instrumentation-http
  • instrumentation-aws-lambda

This should be a "gold-standard" for how to configure sentry for aws-lambda, because even with full ESM + treeshaking + minification the size of Sentry + Opentelemetry is a substantial problem.

@mydea
Copy link
Member

mydea commented May 13, 2024

Thank you for raising this, this is a valid point. We will adjust this so that we do not include all integrations by default for serverless, meaning you'll have to manually add the DB integrations etc. you care about.

mydea added a commit that referenced this issue May 13, 2024
…11998)

In order to keep bundle size compact, performance integrations (except
for http & fetch) have to be manually added for serverless packages.

This means that users will have to do e.g. this if they want to have
mysql instrumented:

```js
import * as Sentry from '@sentry/aws-serverless';

Sentry.init({
  integrations: [Sentry.mysqlIntegration()]
});
```

Closes #11991

---------

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@mydea
Copy link
Member

mydea commented May 13, 2024

We just shipped v8.0.0, where we do not automatically add all auto Instrumentation anymore. This should help to reduce the default bundle size!

@jd-carroll
Copy link
Author

Amazing, thank you for getting this in last minute ❤️

Separately, it would be nice to have a little more detail to the instrumentation customization guide.

@jd-carroll
Copy link
Author

@mydea I just tried v8.0.0 🎊 ... and the problem still exists.

The core of the issue is that they are "statically" imported and still on a valid code path:
image

Here is the latest screenshot from esbuild output:
image
image

So there is no difference, despite the changes you included in v8.

Would it be possible to create a "node-core" package that is most of the current "node" package? The difference being, using @sentry/node would be an instantiation of "node-core" and contain references to all of the standard instrumentation. But then "serverless" would be an instantiation of "node-core" (instead of "node") and only have reference to:

  • instrumentation-aws-sdk
  • instrumentation-http
  • instrumentation-aws-lambda

Then you would be required to add any additional if you wanted.

@mydea
Copy link
Member

mydea commented May 14, 2024

So just the presence/exporting of certain things should normally not affect the bundle size. As long as the code is not used, it should be tree-shaken away. How do you build your lambda function, if I may ask? Maybe there is some config option missing to ensure tree shaking works as expected 🤔

EDIT: Ignore this, I've seen there is a new issue with newer info.

@mydea
Copy link
Member

mydea commented May 14, 2024

Actually, I just noticed my "fix" did not really fix this. I will make sure to actually fix the bundle size, sorry about that...

@mydea mydea reopened this May 14, 2024
@mydea mydea changed the title Guide to Minimize Opentelemetry Foot Print Exclude optional performance instrumentation by default for AWS Lambda May 14, 2024
@mydea mydea changed the title Exclude optional performance instrumentation by default for AWS Lambda Exclude optional performance instrumentation by default for AWS Serverless May 14, 2024
andreiborza pushed a commit that referenced this issue May 16, 2024
…11998)

In order to keep bundle size compact, performance integrations (except
for http & fetch) have to be manually added for serverless packages.

This means that users will have to do e.g. this if they want to have
mysql instrumented:

```js
import * as Sentry from '@sentry/aws-serverless';

Sentry.init({
  integrations: [Sentry.mysqlIntegration()]
});
```

Closes #11991

---------

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@jd-carroll
Copy link
Author

This is resolved, thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants