getsentry / sentry-javascript

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

NestJS SDK #12504

Open mydea opened 4 months ago

mydea commented 4 months ago

We have shipped @sentry/nestjs as Beta. However, there are some follow up tasks we want to do:

### Optional
- [ ] https://github.com/getsentry/sentry-javascript/issues/12770
- [ ] https://github.com/getsentry/sentry-javascript/issues/13216
- [ ] https://github.com/getsentry/sentry-javascript/issues/13167
- [ ] https://github.com/getsentry/sentry-javascript/issues/13133
- [ ] Upstream automatic instrumentation to open telemetry repo
- [ ] https://github.com/getsentry/sentry-javascript/issues/13169
- [ ] https://github.com/getsentry/sentry-javascript/issues/12823
- [ ] https://github.com/getsentry/sentry-javascript/issues/13287
- [ ] https://github.com/getsentry/sentry-javascript/issues/13516
- [ ] https://github.com/getsentry/sentry-javascript/issues/13517
- [ ] https://github.com/getsentry/sentry-javascript/issues/13589
- [ ] https://github.com/getsentry/sentry-javascript/issues/13590
- [ ] https://github.com/getsentry/sentry-javascript/issues/13810

ARCHIVE - All of this is already done

We do have support for Nest.js through @sentry/node today: https://docs.sentry.io/platforms/javascript/guides/nestjs/

However, there are some limitations to this, which we could overcome and improve upon if we had a dedicated package for Nest support.

### Must do
- [x] Implement basic package as wrapper on top of `@sentry/node` https://github.com/getsentry/sentry-javascript/pull/12613
- [ ] https://github.com/getsentry/sentry-javascript/issues/12768
- [x] Filter expected errors https://github.com/getsentry/sentry-javascript/pull/12695 https://github.com/getsentry/sentry-javascript/issues/12523
### Should do (Enhancements)
- [ ] https://github.com/getsentry/sentry-javascript/issues/12752
- [ ] https://github.com/getsentry/sentry-javascript/issues/12769
- [ ] https://github.com/getsentry/sentry-javascript/issues/13064
- [ ] https://github.com/getsentry/sentry-javascript/issues/13078
- [ ] https://github.com/getsentry/sentry-javascript/issues/13121
- [ ] https://github.com/getsentry/sentry-javascript/issues/13190
- [ ] https://github.com/getsentry/sentry-javascript/issues/13157
- [ ] https://github.com/getsentry/sentry-javascript/issues/13033
- [ ] https://github.com/getsentry/sentry-javascript/issues/12824
### Docs
- [x] Initial update of docs to reference new SDK https://github.com/getsentry/sentry-docs/pull/10514
- [ ] https://github.com/getsentry/sentry-javascript/issues/12819
- [ ] https://github.com/getsentry/sentry-javascript/issues/12820
### Post-release/GTM Tasks
- [x] Update docs according to the new package/structure https://github.com/getsentry/sentry-docs/pull/10514
- [x] Update the in-product onboarding to the new package/structure (https://github.com/getsentry/sentry/pull/73577)
- [x] Update static pages https://github.com/getsentry/static-sites/pull/2649
- [x] Changelog post
### Nest.js bugs
- [ ] https://github.com/getsentry/sentry-javascript/issues/12351
- [ ] https://github.com/getsentry/sentry-javascript/issues/12530
- [ ] https://github.com/getsentry/sentry-javascript/issues/12745
- [ ] https://github.com/getsentry/sentry-javascript/issues/12771
- [ ] https://github.com/getsentry/sentry-javascript/issues/13473
- [ ] https://github.com/getsentry/sentry-javascript/issues/13507
rubiin commented 1 month ago

Few things I would like to add

CC: https://github.com/ntegral/nestjs-sentry

lforst commented 1 month ago

@rubiin care to elaborate a bit? What kind of options should it take?

rubiin commented 1 month ago

we are using a separate file for initializing sentry. The module should take all the options required and do that initializing inside the module itself.

Here we are using instrument.js for that https://docs.sentry.io/platforms/javascript/guides/nestjs/ This would allow the support for .forRootAsync which is currently missing.

For reference you could look at the library https://github.com/ntegral/nestjs-sentry. Since this is the most used integration for nestjs and sentry, if the official integration made the implementation similar, the migration would be easier and also make future enhancements easy


import { Module } from '@nestjs-common';
import { SentryModule } from '@ntegral/nestjs-sentry';

@Module({
  imports: [
    SentryModule.forRoot({
      dsn: 'sentry_io_dsn',
      debug: true | false,
      environment: 'dev' | 'production' | 'some_environment',
      release: 'some_release', | null, // must create a release in sentry.io dashboard
      logLevels: ['debug'] //based on sentry.io loglevel //
    }),
  ],
})
export class AppModule {}
lforst commented 1 month ago

Hi, unfortunately, this approach will run into many many problems which I will not outline in detail here but TLDR in order to be able to automatically instrument a bunch of things Sentry needs to run as soon as possible. This is not possible with such an API. @ntegral/nestjs-sentry is depending on a very old SDK version that doesn't do a lot of the automatic magic that requires the SDK to run early.

In the end it comes down to aesthetic preference for you, and aesthetics isn't really what we're after with the SDK. But I still respect and appreciate the feedback! 🙏

rubiin commented 1 month ago

@lforst thats okay . Main concern was rather dependency injection of config service, the aesthetics don't matter rn Will try to cookup an example , perhaps that can be included on the docs