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

Instrumentation of NestJS middleware #12769

Closed
Tracked by #12504
nicohrubec opened this issue Jul 4, 2024 · 0 comments · Fixed by #13065
Closed
Tracked by #12504

Instrumentation of NestJS middleware #12769

nicohrubec opened this issue Jul 4, 2024 · 0 comments · Fixed by #13065
Assignees
Labels
Package: nestjs Issues related to the Sentry Nestjs SDK

Comments

@nicohrubec
Copy link
Contributor

nicohrubec commented Jul 4, 2024

https://docs.nestjs.com/middleware

With the new @SentryTraced decorator, spans for middleware can be obtained by decorating the use function. In this issue we investigate whether we can find a better way to instrument middleware. Ideally this would happen automatically.

Potential initial ideas:

  • Implement a custom middleware class subclassing the native NestMiddleware
  • Class level decorator: This is a bit nicer than @SentryTraced()on the use method directly, because we can only trace the actual execution of the middleware methods without whatever happens after calling next(). Still means manual annotations by users though.
  • Sentry.registerMiddleware(): A function to that takes in middleware classes/functions or maybe even a list of classes/functions and augments them to be traced. Then the augmented form can be registered in nest. This has a bit more flexibility than the decorator because people can either do it below their classes directly in the respective file or wherever they register their middleware as a list.
  • Automatic instrumentation with discovery service: Currently the nest SDK is setup by adding a root module to the user application. We can add a service in this module that uses the DiscoveryService to get all user defined middlewares and proxy the use method. However, right now it looks like users would need to add extra metadata on their middleware for this discovery to work properly in which case we might as well use decorators on the classes directly.
  • Automatic otel instrumentation: Ideally we want to use otel to automatically wrap the use method of any nest middleware. The problem I have so far is that NestMiddleware does not actually live as a class somewhere, but is rather just an interface that users can implement. One idea is to hook into the Injectable()decorator and then patch the use method in case it exists. This approach would be easily extendable to work for Interceptors too. However, middleware can also simply be a function, in which case this approach would not work.

Solution:

  • Ended up writing a custom open telemetry instrumentation hooking into the @Injectable decorator.

Limitations:

  • Middleware can be both a class as well as a function. This approach does not work for functions.
  • Although the nest docs say that class middleware should always have an @Injectable decorator, middleware works just fine without it unless it has dependencies on other services.
@nicohrubec nicohrubec changed the title Automatic instrumentation of middleware and/or interceptors Automatic instrumentation of NestJS middleware and/or interceptors Jul 4, 2024
@nicohrubec nicohrubec self-assigned this Jul 4, 2024
@AbhiPrasad AbhiPrasad added the Package: nestjs Issues related to the Sentry Nestjs SDK label Jul 4, 2024
@nicohrubec nicohrubec changed the title Automatic instrumentation of NestJS middleware and/or interceptors Automatic instrumentation of NestJS middleware Jul 24, 2024
@nicohrubec nicohrubec changed the title Automatic instrumentation of NestJS middleware Instrumentation of NestJS middleware Jul 24, 2024
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
None yet
Development

Successfully merging a pull request may close this issue.

2 participants