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

NestJS SDK #12504

Open
27 of 37 tasks
mydea opened this issue Jun 17, 2024 · 5 comments
Open
27 of 37 tasks

NestJS SDK #12504

mydea opened this issue Jun 17, 2024 · 5 comments
Labels
Package: nestjs Issues related to the Sentry Nestjs SDK

Comments

@mydea
Copy link
Member

mydea commented Jun 17, 2024

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

Optional

  1. Package: nestjs
    nicohrubec
  2. Type: Tests
    nicohrubec
  3. Package: nestjs
  4. Package: nestjs
  5. Package: nestjs
  6. Package: nestjs
  7. Package: nestjs
  8. Package: nestjs Type: Tests
    nicohrubec
  9. Package: nestjs Type: Improvement
  10. Package: nestjs Type: Improvement
  11. Package: nestjs

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

  1. Package: nestjs
    nicohrubec

Should do (Enhancements)

  1. nicohrubec
  2. Package: nestjs
    nicohrubec
  3. Package: nestjs
    nicohrubec
  4. Package: nestjs
    nicohrubec
  5. Package: nestjs
    nicohrubec
  6. Package: nestjs Type: Improvement
    nicohrubec
  7. Package: nestjs
    nicohrubec
  8. Package: nestjs
    nicohrubec
  9. Package: nestjs

Docs

  1. Package: nestjs
    nicohrubec
  2. Package: nestjs
    nicohrubec

Post-release/GTM Tasks

Nest.js bugs

  1. Package: nestjs Package: node Type: Bug
    nicohrubec
  2. Integration: graphql Integration: nest.js Package: node
  3. Package: node Type: Bug
    nicohrubec
  4. Package: nestjs Type: Bug
  5. Package: nestjs
    nicohrubec
  6. Package: nestjs
    nicohrubec
@mydea mydea mentioned this issue Jun 17, 2024
@AbhiPrasad AbhiPrasad added the Package: nestjs Issues related to the Sentry Nestjs SDK label Jun 17, 2024
@nicohrubec nicohrubec self-assigned this Jun 24, 2024
@mydea mydea changed the title Implement @sentry/nest package for full Nest.js support Nest SDK Jun 26, 2024
@mydea mydea pinned this issue Jun 26, 2024
@mydea mydea unpinned this issue Aug 19, 2024
@lforst lforst changed the title Nest SDK NestJS SDK Aug 26, 2024
@lforst lforst pinned this issue Aug 26, 2024
@rubiin
Copy link

rubiin commented Aug 28, 2024

Few things I would like to add

  • SentryModule.forRoot() should take option
  • Also add SentryModule.forRootAsync that would allow working with config service

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

@lforst
Copy link
Member

lforst commented Aug 28, 2024

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

@rubiin
Copy link

rubiin commented Aug 28, 2024

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
Copy link
Member

lforst commented Aug 29, 2024

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
Copy link

rubiin commented Aug 29, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nestjs Issues related to the Sentry Nestjs SDK
Projects
Status: No status
Development

No branches or pull requests

5 participants