aandrewww / pino-sentry

🚥 Load pino logs into Sentry
MIT License
61 stars 29 forks source link

pino-sentry not picking up scope from withScope #51

Closed segevfiner closed 2 years ago

segevfiner commented 2 years ago

Trying this bit of code, results in the log message being written without the tag from withScope:

// Refactor me plz ;P
test('Scope propogation', async () => {
  const options = {
    level: "info"
  };
  const mockSend = jest.fn(() => Promise.resolve());
  const stream = createWriteStream({dsn: SENTRY_DSN, transport() { return {send: mockSend, flush: jest.fn(() => Promise.resolve(true))}; }});
  const logger = pinoLogger(options, stream);

  Sentry.withScope((scope) => {
    scope.setTag('test', 'foobar');

    logger.error('test');
  });

  await new Promise<void>((resolve, reject) => {
    setImmediate(() => {
      try {
        expect(mockSend.mock.calls.length).toBe(1);
        resolve();
      } catch (error) {
        reject(error);
      }
    });
  });
});

If I apply the following change to pino-sentry, the tag then appears as expected.

diff --git a/src/transport.ts b/src/transport.ts
index ea3bf0e..f229e30 100644
--- a/src/transport.ts
+++ b/src/transport.ts
@@ -162,14 +162,14 @@ export class PinoSentryTransport {
     if (this.isSentryException(severity)) {
       const error = message instanceof Error ? message : new ExtendedError({ message, stack });

+      Sentry.captureException(error, scope);
       setImmediate(() => {
-        Sentry.captureException(error, scope);
         cb();
       });
     } else {
       // Capturing Messages
+      Sentry.captureMessage(message, scope);
       setImmediate(() => {
-        Sentry.captureMessage(message, scope);
         cb();
       });
     }

The Sentry SDK is already async internally so it is safe to just call it directly like that AFAIK. Not sure about cb so I left that in a setImmediate. Sentry's scope management is not "perfect" IMHO.

glensc commented 2 years ago

@segevfiner: can you send a PR, I think this is ok to merge.