ref(nestjs): extract shared span helpers#22175
Conversation
isaacs
requested review from
chargome,
mydea and
nicohrubec
and removed request for
a team
size-limit report 📦
|
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
overall lgtm, I just have two small questions/remarks
isaacs
force-pushed
the
isaacs/node-move-default-integration-swap
branch
from
7b12bae to
63a580f
Compare
isaacs
requested review from
JPeer264 and
andreiborza
and removed request for
a team
isaacs
force-pushed
the
isaacs/nestjs-extract-helpers
branch
from
1eb7633 to
fa0de85
Compare
isaacs
force-pushed
the
isaacs/node-move-default-integration-swap
branch
from
63a580f to
89c3b4c
Compare
isaacs
force-pushed
the
isaacs/nestjs-extract-helpers
branch
from
fa0de85 to
dd92103
Compare
isaacs
force-pushed
the
isaacs/node-move-default-integration-swap
branch
from
89c3b4c to
1d877df
Compare
isaacs
force-pushed
the
isaacs/nestjs-extract-helpers
branch
from
dd92103 to
b74dcbd
Compare
isaacs
force-pushed
the
isaacs/node-move-default-integration-swap
branch
from
1d877df to
5d4d988
Compare
isaacs
force-pushed
the
isaacs/nestjs-extract-helpers
branch
from
b74dcbd to
9315716
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
force-pushed
the
isaacs/node-move-default-integration-swap
branch
from
5d4d988 to
b2a369d
Compare
isaacs
force-pushed
the
isaacs/nestjs-extract-helpers
branch
from
9315716 to
28b5141
Compare
Pull the span-emitting/patching logic out of the OTel `InstrumentationBase` classes into `wrap-components`, `wrap-handlers`, and `wrap-route` so it can be reused by the diagnostics-channel integration. Refactor only. There is still only the OTel implementation, which still emits the same spans as before. However, this will be needed when the next implementation is added.
isaacs
force-pushed
the
isaacs/node-move-default-integration-swap
branch
from
b2a369d to
56289c4
Compare
isaacs
force-pushed
the
isaacs/nestjs-extract-helpers
branch
from
28b5141 to
915478f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 915478f. Configure here.
| * captured from the decorator factory. | ||
| */ | ||
| function deriveEventName(handler: AnyFn, fallbackEvent: unknown): string { | ||
| const R = Reflect as unknown as ReflectWithMetadata; |
There was a problem hiding this comment.
Uncommented unsafe type casts
Low Severity
New as unknown as double-casts on Reflect appear without a comment explaining why a safer type isn’t possible. The PR review guidelines require that explanation for each such cast in SDK source (similar to the existing reflect-metadata notes elsewhere in this package).
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 915478f. Configure here.
| if (process && typeof process === 'function' && !target?.__SENTRY_INTERNAL__ && !isWrapped(process)) { | ||
| const wrapped = wrapBullMQProcess(process, queueName); | ||
| markWrapped(wrapped); | ||
| target.prototype!.process = wrapped; |
There was a problem hiding this comment.
Uncommented non-null assertion
Low Severity
target.prototype!.process uses a non-null assertion without a comment explaining why a safer type isn’t possible. The PR review guidelines require that explanation for new ! assertions in SDK source, and the preceding optional checks already make a non-asserting write feasible.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 915478f. Configure here.
| const wrapped = wrapBullMQProcess(process, queueName); | ||
| markWrapped(wrapped); | ||
| target.prototype!.process = wrapped; | ||
| } |
There was a problem hiding this comment.
Lost BullMQ process name
Low Severity
patchProcessorTarget replaces prototype.process with an anonymous wrapper and never preserves the original function name. The previous Proxy wrapper kept the name process, and patchMethodDescriptor still preserves names for schedule/event handlers, so BullMQ stacks and introspection now regress inconsistently.
Reviewed by Cursor Bugbot for commit 915478f. Configure here.
| apply: (originalHandle, thisArgHandle, argsHandle) => { | ||
| beforeSpan.end(); | ||
| const run = (): unknown => { | ||
| const handleReturn = Reflect.apply(originalHandle, thisArgHandle, argsHandle); | ||
| if (!seenContexts.has(context)) { | ||
| seenContexts.add(context); | ||
| afterSpan = startInactiveSpan( | ||
| getMiddlewareSpanOptions(target, 'Interceptors - After Route', 'interceptor'), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: In stacked async interceptors, a beforeSpan can be ended twice because the shared ExecutionContext prevents subsequent interceptors from creating an afterSpan, leading to a second end() call.
Severity: MEDIUM
Suggested Fix
Restore the guard from the previous implementation. In the .then() callback for the async interceptor path, add an early return if afterSpan is not defined. This will prevent the code from reaching the second beforeSpan.end() call, aligning the behavior with the pre-refactor logic which correctly handled stacked interceptors.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/nestjs/src/integrations/wrap-components.ts#L37-L46
Potential issue: When multiple asynchronous NestJS interceptors are stacked on a single
route, a span can be ended twice. NestJS shares the same `ExecutionContext` across all
interceptors in a chain. The first interceptor adds the context to `seenContexts` and
creates an `afterSpan`. Subsequent interceptors see the context in `seenContexts` and do
not create their own `afterSpan`. This causes the subsequent interceptor to call
`beforeSpan.end()` once in its handle proxy, and then a second time in its async
`.then()` callback because `afterSpan` is undefined. This behavior was prevented in
previous code by an early return, which has been removed in the refactor. This could
lead to errors or incorrect telemetry if `span.end()` is not idempotent.
Also affects:
packages/nestjs/src/integrations/wrap-components.ts:66~72
2 participants
Pull the span-emitting/patching logic out of the OTel
InstrumentationBaseclasses intowrap-components,wrap-handlers, andwrap-routeso it can be reused by the diagnostics-channel integration.Refactor only. There is still only the OTel implementation, which still emits the same spans as before. However, this will be needed when the next implementation is added.