getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.7k stars 1.5k forks source link

[v8] Changes to serverless package #10976

Closed AbhiPrasad closed 5 days ago

AbhiPrasad commented 2 months ago

In https://github.com/getsentry/sentry-javascript/pull/10540 we explored refactoring the serverless package for v8, and we found some more structural issues that needed to be solved. This was further expanded with https://github.com/getsentry/sentry-javascript/pull/10561.

This is a brief proposal for the changes I think the serverless package should take.

Current State

Right now @sentry/serverless:

  1. re-exports everything from @sentry/node (aka @sentry/node-experimental) under main package exports
import { captureException } from '@sentry/serverless';
  1. Exports 4 functions init, getDefaultIntegrations, wrapHandler, tryPatchHandler and re-exports all of @sentry/node under AWSLambda namespace
import { AWSLambda } from '@sentry/serverless';

AWSLambda.init(...);

AWSLambda.wrapHandler(...);

AWSLambda.captureException(...);
  1. Exports 5 functions init, getDefaultIntegrations, wrapCloudEventHandler, wrapEventFunction, wrapHttpFunction under the GCPFunction namespace
import { GCPFunction } from '@sentry/serverless';

GCPFunction.init(...);

GCPFunction.wrapCloudEventHandler(...);

There is some logic in awslambda-auto.ts that is used for the lambda layer.

The main export also contains awsServicesIntegration export. There is a googleCloudGrpcIntegration and googleCloudHttpIntegration that exists in the package, but is not exported 😢.

Problem

  1. Namespace exports like AWSLambda and GCPFunction are not treeshakable, and bundle size really matters for serverless. This is not that bad for GCPFunction, but given AWSLambda re-exports all of the node sdk, that means anyone using AWSLambda and bundling pulls in the entire node sdk!
  2. It is very confusing to have three different init exported from the same package, each with their own behaviours (node init, aws init, gcp init)
  3. AWS and GCP code share nothing in common with each other (the utils file in serverless is only used by gcp related code), and the structure is confusing for maintainers.

Solution

I propose we split @sentry/serverless into two packages, @sentry/aws and @sentry/google-cloud. This means we only export a single init, and for users the onboarding becomes way easier. In the future we can easily add support for @sentry/azure or similar! This makes sure we can optimize for that specific serverless package.

We have two options:

  1. Make the change in v7 and deprecate the @sentry/serverless package (easier upgrade path)
  2. Just split the packages up in v8 and make the breaking change for every serverless sdk user to switch to using either @sentry/aws or @sentry/google-cloud (lambda layer users will not be impacted in anyway.

The biggest change is to change package name, but we aren't breaking any of the top level exports so migration shouldn't be too bad. Lambda functions are also small, so I can't see it being too much work to go through people's codebase and change things.

Addendum

In https://github.com/getsentry/sentry-javascript/pull/10561 I explored using subpath exports as an alternative to new packages. I'm happier with the separate packages (more accurate download counts too!), but we can change the proposal if sentiments have changed.

Work

### Tasks
- [x] https://github.com/getsentry/sentry-javascript/pull/10993
- [x] remove all google cloud logic from `@sentry/serverless`
- [ ] https://github.com/getsentry/sentry-javascript/pull/11052
- [ ] https://github.com/getsentry/sentry-javascript/pull/11065
- [ ] https://github.com/getsentry/sentry-javascript/issues/11066
- [x] Update the path for the env var that Sentry sets in peoples lambdas https://github.com/getsentry/sentry/blob/b5f2368068033e3a32b049cba4ff2d7f20612f48/src/sentry/integrations/aws_lambda/utils.py#L242
- [ ] https://github.com/getsentry/sentry/pull/70137
### After Release
- [ ] Remove deprecated lambda container image code - https://docs.sentry.io/platforms/node/guides/aws-lambda/container-image/
mydea commented 2 months ago

generally sounds good to me - should we still have a (generic?) serverless package? is there anything that could be in there? What about non-aws/gcp serverless environments, could they/would they so far have used the serverless package?

Lms24 commented 2 months ago

Agreed, sounds like a good change! Should the AWS/GCP packages inherit from @sentry/serverless which in turn continues to be based on @sentry/node?

On that note, are we sure that serverless environments work with Otel? I guess as long as we're in a Node-compatible runtime they should work but not sure how well we investigated this yet.

lforst commented 2 months ago

@mydea @Lms24 I don't particularly see the value behind a generic @sentry/serverless package. What would be in there? So far any other serverless envs would have just used the Node pkg.

I think purely splitting it up into provider-specific packages is the way to go. Let's kill @sentry/serverless.

mydea commented 2 months ago

If we don't have any "generic" serverless code (that could not also just live in core) then yes, let's delete it!

Lms24 commented 2 months ago

yeah that's fine. I was just under the impression that there's no direct replacement for the default init from @sentry/serverless but that's probably fine. Also remembered that we tell folks to use the Node SDK in the Azure functions guide anyway.

AbhiPrasad commented 2 months ago

On that note, are we sure that serverless environments work with Otel? I guess as long as we're in a Node-compatible runtime they should work but not sure how well we investigated this yet.

With CJS they should work pretty fine! But we need to investigate esm (also the serverless packages use monkeypatching require right now, that will break).

I'm gonna move forward with Option 2 - splitting the package up! Created a tasklist to reflect the work.