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

[matthieuMay]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

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

5. comment the import of instrument.js or unset SENTRY_DSN variable
6. run npm run start:dev
7. see that event handler is called twice as expected

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

[mydea]

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

[MustagheesButt]

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

[Lms24]

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 :)

[matthieuMay]

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);
         };

[NiceChildHQ]

Facing same issue ~

[chargome]

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