Skip to content

Nest-js instrumentation breaks @nestjs/event-emitter when using multiple @OnEvent on the same method #15218

New issue
New issue
Closed
#16306
Closed
Nest-js instrumentation breaks @nestjs/event-emitter when using multiple @OnEvent on the same method#15218
#16306
Assignees
chargome
Labels
BugNest.js

Description

@matthieuMay
matthieuMay
opened on Jan 29, 2025

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/nestjs

SDK Version

8.52.0

Framework Version

Nestjs 10.4.7

Link to Sentry event

No response

Reproduction Example/SDK Setup

I set up a minimal repro here :
https://github.com/matthieuMay/sentry_nest_event_repro

This is a minimal nestjs + nestjs/event-emitter + sentry configuration.

It contains:

  • instrument.ts : sentry initialization
  • app.module.ts: main nestjs module
  • app.controller.ts: main nestjs module
  • app.service.ts: service setting a event listener on two events and emitting the two events when application is bootstrapped

Steps to Reproduce

  1. npm install
  2. set environnement SENTRY_DSN variable
  3. run npm run start:dev
  4. see that event handler is called only once
Image
  1. comment the import of instrument.js or unset SENTRY_DSN variable
  2. run npm run start:dev
  3. see that event handler is called twice as expected
Image

Expected Result

We should see the handler being called twice

Explanation of the bug

Nestjs initialize in two steps : first the decorators set metadata on the decorated functions and once all decorators have been read, this metadata is traducted in method modifications.

Sentry instrumentation overrides the OnEvent decorator in SentryNestEventInstrumentation._createWrapOnEvent method. This new decorator ovverides descriptor.value on every call.
Therefore, when decorating the same handler with multiple @onevent, descriptor.value changes on every call changing the target of the metadata on every call. As a consequence, only the last called decorator is actually applied

Actual Result

Image

Activity

  1. getsantry
    added
    Waiting for: Product Owner
    on Jan 29, 2025
  2. getsantry
    added this to GitHub Issues with 👀 3on Jan 29, 2025
  3. getsantry
    moved this to Waiting for: Product Owner in GitHub Issues with 👀 3on Jan 29, 2025
  4. github-actions
    added
    Package: nestjs
    on Jan 29, 2025

  5. mydea commented on Jan 29, 2025

    @mydea
    mydea
    on Jan 29, 2025
    Member

    Hey, thanks for writing in. We will look into it!

  6. getsantry
    removed
    Waiting for: Product Owner
    on Jan 29, 2025
  7. getsantry
    moved this from Waiting for: Product Owner to No status in GitHub Issues with 👀 3on Jan 29, 2025
  8. mydea
    assigned
    chargome
    on Jan 29, 2025

  9. MustagheesButt commented on Mar 11, 2025

    @MustagheesButt
    MustagheesButt
    on Mar 11, 2025

    Facing same issue. Using a custom decorator for multiple OnEvent like:

    import { OnEvent } from '@nestjs/event-emitter';

export const OnEvents = (events: string[]) =>
  applyDecorators(...events.map((e) => OnEvent(e)));

    When used like @OnEvents(['eventA', 'eventB']), only the last works when instrument.js is imported.

    Thanks @matthieuMay , I could've never guessed sentry sdk was causing it. How do you even debug something like this

  10. getsantry
    added
    Waiting for: Product Owner
    on Mar 11, 2025
  11. getsantry
    moved this to Waiting for: Product Owner in GitHub Issues with 👀 3on Mar 11, 2025

  12. Lms24 commented on Mar 12, 2025

    @Lms24
    Lms24
    on Mar 12, 2025
    Member

    Hi, thanks for reminding us of this issue. cc @chargome I see you're already assigned but maybe you could take another look at this?

    Heass-up: Most team members are out for the rest of the week so might only get to it next week.

    If anyone has ideas how to fix this, PRs are also welcome :)

  13. getsantry
    removed
    Waiting for: Product Owner
    on Mar 12, 2025
  14. getsantry
    moved this from Waiting for: Product Owner to No status in GitHub Issues with 👀 3on Mar 12, 2025

  15. matthieuMay commented on Mar 12, 2025

    @matthieuMay
    matthieuMay
    on Mar 12, 2025
    Author

    Hi all

    I came up with a patch package but it is not perfect as all spans are merged into one span.
    Not a good enough fix to be released but it may give some ideas to fix this

    diff --git a/node_modules/@sentry/node/build/cjs/integrations/tracing/nest/sentry-nest-event-instrumentation.js b/node_modules/@sentry/node/build/cjs/integrations/tracing/nest/sentry-nest-event-instrumentation.js
index 0a0bb50..f673733 100644
--- a/node_modules/@sentry/node/build/cjs/integrations/tracing/nest/sentry-nest-event-instrumentation.js
+++ b/node_modules/@sentry/node/build/cjs/integrations/tracing/nest/sentry-nest-event-instrumentation.js
@@ -64,7 +64,7 @@ class SentryNestEventInstrumentation extends instrumentation.InstrumentationBase
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
       return function wrappedOnEvent(event, options) {
         const eventName = Array.isArray(event)
-          ? event.join(',')
+          ? event.map(e => e.toString()).join(',')
           : typeof event === 'string' || typeof event === 'symbol'
             ? event.toString()
             : '<unknown_event>';
@@ -83,21 +83,31 @@ class SentryNestEventInstrumentation extends instrumentation.InstrumentationBase
           // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
           const handlerName = originalHandler.name || propertyKey;
 
-          // Instrument the handler
-          // eslint-disable-next-line @typescript-eslint/no-explicit-any
-          descriptor.value = async function (...args) {
-            return core.startSpan(helpers.getEventSpanOptions(eventName), async () => {
-              try {
-                // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-                const result = await originalHandler.apply(this, args);
-                return result;
-              } catch (error) {
-                // exceptions from event handlers are not caught by global error filter
-                core.captureException(error);
-                throw error;
-              }
-            });
-          };
+          const isOnEventInstrumented= Reflect.getMetadata('SentryNestEventInstrumentation.isOnEventInstrumented', descriptor.value) || false;
+
+          if (!isOnEventInstrumented) {
+            // Instrument the handler
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            descriptor.value = async function (...args) {
+              return core.startSpan(helpers.getEventSpanOptions(eventName), async () => {
+                try {
+                  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+                  const result = await originalHandler.apply(this, args);
+                  return result;
+                } catch (error) {
+                  // exceptions from event handlers are not caught by global error filter
+                  core.captureException(error);
+                  throw error;
+                }
+              });
+            };
+          }
+          else {
+            console.error(
+              'Cannot apply multiple nestjs OnEvent decorators on the same function when using sentry instrumentation. All spans will be merged into one span'
+            );
+          }
+
 
           // Preserve the original function name
           Object.defineProperty(descriptor.value, 'name', {
@@ -105,6 +115,9 @@ class SentryNestEventInstrumentation extends instrumentation.InstrumentationBase
             configurable: true,
           });
           
+          Reflect.defineMetadata('SentryNestEventInstrumentation.isOnEventInstrumented', true, descriptor.value);
+        
+
           // Apply the original decorator
           return decoratorResult(target, propertyKey, descriptor);
         };
diff --git a/node_modules/@sentry/node/build/esm/integrations/tracing/nest/sentry-nest-event-instrumentation.js b/node_modules/@sentry/node/build/esm/integrations/tracing/nest/sentry-nest-event-instrumentation.js
index 5d95019..358932a 100644
--- a/node_modules/@sentry/node/build/esm/integrations/tracing/nest/sentry-nest-event-instrumentation.js
+++ b/node_modules/@sentry/node/build/esm/integrations/tracing/nest/sentry-nest-event-instrumentation.js
@@ -62,7 +62,7 @@ class SentryNestEventInstrumentation extends InstrumentationBase {
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
       return function wrappedOnEvent(event, options) {
         const eventName = Array.isArray(event)
-          ? event.join(',')
+          ? event.map(e => e.toString()).join(',')
           : typeof event === 'string' || typeof event === 'symbol'
             ? event.toString()
             : '<unknown_event>';
@@ -81,21 +81,30 @@ class SentryNestEventInstrumentation extends InstrumentationBase {
           // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
           const handlerName = originalHandler.name || propertyKey;
 
-          // Instrument the handler
-          // eslint-disable-next-line @typescript-eslint/no-explicit-any
-          descriptor.value = async function (...args) {
-            return startSpan(getEventSpanOptions(eventName), async () => {
-              try {
-                // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-                const result = await originalHandler.apply(this, args);
-                return result;
-              } catch (error) {
-                // exceptions from event handlers are not caught by global error filter
-                captureException(error);
-                throw error;
-              }
-            });
-          };
+          const isOnEventInstrumented= Reflect.getMetadata('SentryNestEventInstrumentation.isOnEventInstrumented', descriptor.value) || false;
+
+          if (!isOnEventInstrumented) {
+            // Instrument the handler
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            descriptor.value = async function (...args) {
+              return startSpan(getEventSpanOptions(eventName), async () => {
+                try {
+                  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+                  const result = await originalHandler.apply(this, args);
+                  return result;
+                } catch (error) {
+                  // exceptions from event handlers are not caught by global error filter
+                  captureException(error);
+                  throw error;
+                }
+              });
+            };
+          }
+          else {
+            console.error(
+              'Cannot apply multiple nestjs OnEvent decorators on the same function when using sentry instrumentation. All spans will be merged into one span'
+            );
+          }
 
           // Preserve the original function name
           Object.defineProperty(descriptor.value, 'name', {
@@ -103,6 +112,9 @@ class SentryNestEventInstrumentation extends InstrumentationBase {
             configurable: true,
           });
 
+          Reflect.defineMetadata('SentryNestEventInstrumentation.isOnEventInstrumented', true, descriptor.value);
+
+        
           // Apply the original decorator
           return decoratorResult(target, propertyKey, descriptor);
         };

  16. 9 remaining items

  17. stephanie-anderson
    removed the
    Bug
    issue type on Apr 30, 2025
  18. stephanie-anderson
    added
    Nest.js
    and removed
    Package: nestjs
    on May 2, 2025
  19. mydea
    added
    Waiting for: Product Owner
    on May 7, 2025
  20. getsantry
    moved this to Waiting for: Product Owner in GitHub Issues with 👀 3on May 7, 2025
  21. mydea
    removed
    Waiting for: Product Owner
    on May 7, 2025
  22. getsantry
    moved this from Waiting for: Product Owner to No status in GitHub Issues with 👀 3on May 7, 2025

  23. HeyChengdu commented on May 13, 2025

    @HeyChengdu
    HeyChengdu
    on May 13, 2025

    Facing same issue ~

  24. getsantry
    added
    Waiting for: Product Owner
    on May 13, 2025
  25. getsantry
    moved this to Waiting for: Product Owner in GitHub Issues with 👀 3on May 13, 2025

  26. chargome commented on May 14, 2025

    @chargome
    chargome
    on May 14, 2025
    Member

    @NiceChildHQ thanks for reporting, sorry this fell under the radar – contributions are welcome if anyone wants a go at this!

  27. getsantry
    removed
    Waiting for: Product Owner
    on May 14, 2025
  28. getsantry
    moved this from Waiting for: Product Owner to No status in GitHub Issues with 👀 3on May 14, 2025
  29. chargome
    closed this as completedin #16306on May 19, 2025

  30. github-actions commented on May 20, 2025

    @github-actions
    github-actionsBot
    on May 20, 2025 – with GitHub Actions
    Contributor

    A PR closing this issue has just been released 🚀

    This issue was referenced by PR #16306, which was included in the 9.21.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

BugNest.js

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions