Uh oh!
There was an error while loading. Please reload this page.
feat(nestjs): Automatic instrumentation of nestjs middleware - #13065
Conversation
size-limit report 📦
|
4f394c4 to
6e35eb1CompareUh oh!
There was an error while loading. Please reload this page.
lforst
left a comment
There was a problem hiding this comment.
Great job! I left a few comments that we need to address.
Uh oh!
There was an error while loading. Please reload this page.
| if (typeof target.prototype.use === 'function') { | ||
| const originalUse = target.prototype.use; | ||
| target.prototype.use = function (req: unknown, res: unknown, next: (error?: unknown) => void): void { |
There was a problem hiding this comment.
m: We should make very sure that the this context stays intact. Also we should forward all arguments!
Maybe let's add a unit test?
There was a problem hiding this comment.
replaced this also with a proxy and pass all arguments along now.
| */ | ||
| private _getInjectableFileInstrumentation(versions: string[]): InstrumentationNodeModuleFile { | ||
| return new InstrumentationNodeModuleFile( | ||
| '@nestjs/common/decorators/core/injectable.decorator.js', |
There was a problem hiding this comment.
m: Since Injectable is exported from the top-level of @nestjs/common, I wonder if we can just do '@nestjs/common' here 🤔
There was a problem hiding this comment.
No doesn't work (changed it locally and it breaks the e2e tests)
There was a problem hiding this comment.
FWIW they also wrap specific files like this in the nestjs-core instrumentation (https://www.npmjs.com/package/@opentelemetry/instrumentation-nestjs-core)
There was a problem hiding this comment.
Kinda sucks because if they change the file structure this instantly won't work anymore :/ Stuff like this makes us need canary test otherwise we'll never catch breakages like that.
@nicohrubec can you investigate whether OTEL has some other functionality that we can use where we can point at '@nestjs/common'.
There was a problem hiding this comment.
Have played around with it but could not make anything else work so far. FWIW I think it's fine here. They have not moved this file in 6 years: https://github.com/nestjs/nest/commits/master/packages/common/decorators/core/injectable.decorator.ts
| }, | ||
| (span: Span) => { | ||
| // patch next to end span before next middleware is being called | ||
| const wrappedNext = (error?: Error | unknown): void => { |
There was a problem hiding this comment.
h: We should forward all arguments not just the first one. Ideally we even use a proxy like for example here:
Uh oh!
There was an error while loading. Please reload this page.
| return new SentryNestInstrumentation(); | ||
| }); | ||
| export const instrumentNest = Object.assign( |
There was a problem hiding this comment.
m: The object assign here seems unnecessary. This could just be a simple function.
There was a problem hiding this comment.
we need the id for filtering out stuff in preload!
There was a problem hiding this comment.
I have updated it to use no Object.assign explicitly but still set an id, which should work too. No strong feelings either way
There was a problem hiding this comment.
Going with the original version with Object.assign to not introduce side-effects
nicohrubec
commented
Jul 30, 2024
What should we put for instrumentationVersion? |
lforst
commented
Jul 30, 2024
We could do |
Adds middleware instrumentation to the
@sentry/nestjs. The implementation lives in@sentry/nodeso that both users using@sentry/nestjsdirectly as well as users still on@sentry/nodebenefit. The instrumentation is automatic without requiring any additional setup. The idea is to hook into the Injectable decorator (every class middleware is annotated with@Injectableand patch theusemethod if it is implemented.Caveat: This implementation only works for class middleware, which implements the
usemethod, which seems to be the standard for implementing middleware in nest. However, nest also provides functional middleware, for which this implementation does not work.Trace from my sample app:
Resolves#12769