search

getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.95k stars 1.57k forks source link

[@sentry/angular - @sentry/browser] Setting context data (tags, extra or context) for errors/exceptions is not working in an async context #5417

Closed rpanadero closed 1 year ago

rpanadero commented 2 years ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/angular

SDK Version

7.6.0

Framework Version

14.0.0

Link to Sentry event

No response

Steps to Reproduce

Hello,

We are facing some issues with the latest browser/angular SDK version (7.6.0) because the context data (tags, extra, etc), which we just set before capture the event by Sentry SDK, don't appear in the Sentry dashboard along with the error. This problem just shows up when the error happens in an async context like:

setTimeout(() => {
   const c = null;
   const v = c.pop;
}, 1000)

However, if the error happens in a sync context, the error context data does appear in the Sentry dashboard as expected.

In order to set a specific context for an error we have unsuccessfully tried all ways that the documentation says here: https://docs.sentry.io/platforms/javascript/enriching-events/context/#passing-context-directly

For example like that:

Sentry.withScope(scope => {
  scope.setTag('customTag', 'error');
  scope.setExtra('test', 'this is a test');
  scope.setContext('test', { value: 'test context' });
  Sentry.captureException(extractedError);
});

Or like that: 

Sentry.captureException(error, {
  tags: {
    section: "articles",
  },
});

For our surprise, this just happens in SDK version higher than 7.0, since we have also tried to do the same with the latest 6x SDK version and it is working property over it (all data is shown up in the Sentry dashboard as expected).

Please, is there some official workaround to deal with this issue?

Thanks in advance,

Expected Result

Sentry SDK should record tags, extra and context we set for the error captured by Sentry SDK and they also should be shown up in the Sentry dashboard.

Actual Result

Sentry SDK doesn't record properly the error tags and context data when the error happens in an async context. It seems that all data set in the error scope is lost.

Lms24 commented 2 years ago

Hi @rpanadero and thanks for reporting! Also thanks for the good description. Would you be able to provide a simple reproduction for us to work on this? I don't think we changed something regarding scopes and contexts in v7 so it would be interesting to find out what's wrong here. Thanks!

rpanadero commented 2 years ago

@Lms24 , of course. I have just created a demo project to reproduce the issue.

https://github.com/rpanadero/sentry-issue-5417

The main project page contains two buttons to raise an error from an async or sync context.

And I will also share with you the details of the error reported in the Sentry Dashboard.

Async error details https://sentry.io/share/issue/d16a069ce14f486a8aa757649d5060c3/

No tags or other error context data are present in error report.

Sync error details https://sentry.io/share/issue/ab36f92248f84eb89551f1ccc53b86b0/

Tags and other context data are present in error report.

This issue is breaking our application reports and our customers are complaining, please I would really appreciate a fix or workaround asap. I have played with Hubs and Scope to try to fix the issue, but all workarounds I have found make us loose other important error data.

Thanks!

Lms24 commented 2 years ago

I took a quick look at the repro and I saw that you're using a custom ErrorHandler. I suspect that one of the reasons for doing this is because you want to attach the tags/context before the errors are sent to Sentry, right?

If so, I have two suggestions:

It'd be interesting as to why this apparently started happening with v7 and not with v6 but we're aware that there are certain issues with scopes and async contexts so this might be one of them (but those date back pre-v7).

Hope this helps a little!

rpanadero commented 2 years ago

@Lms24 , you're right, I need to add some tags and context data before sending the errors to Sentry.

Regarding your suggestions, firstly, I have tried to capture the exception outside the Angular zone but this hasn't solved the issue unfortunately (check the demo project again if you need it). While your first workaround is not valid for our use case, since we have to add some context data, which we just have at the moment of capturing the error by Sentry SDK and not in the beforeSend hook.

Thanks for your suggestions, but have you tried to reproduce the issue in v7, verify that it's not happening in v6, and go go deep in this issue? I agree with you that it's really interesting to get to know why this is happening from v6... Like that, we'll probably find a workaround or a fix.

Thanks for your effort.

Lms24 commented 2 years ago

Thanks for getting back to me. I agree, this needs a more thorough investigation. Right now though, our priorities are on other tasks but I'll backlog this with high priority. In the meantime, if you happen to find a solution/workaround, please feel free to share it here; maybe we can incorporate it into the SDK then.

To everyone reading this issue: Please react to the issue if you're having the same problem. PRs are always welcome!

rpanadero commented 2 years ago

Thanks! I hope to hear good news from you asap.

janvennemann commented 2 years ago

@Lms24 i think i'm experiencing the same issue with @sentry/node and the express handlers. I've created a bunch of very simple test cases here: https://github.com/janvennemann/sentry-callback-issue. I'm running Node.js v16.15.0 on MacOS 12.3.1.

As soon as there is a callback based async API involved Sentry seems to not send any additional info for the HTTP request. Interestingly enough, when doing the same using Promise based APIs, it works again. See the mongodb examples in my repo. This is currently a blocker for us using Sentry in our Nodejs backend.

I've also found https://github.com/getsentry/sentry-javascript/issues/1939 which looks similar since i first noticed the issues with passport and mongodb. However, since that issue is quite old i think this is something new.

Luis-Palacios commented 2 years ago

I've run into the same issue on Express I've tried every way to add custom data but to no avail.

My code is like this:

sentry.init(sentryConfig.get(app));

app.use(sentry.Handlers.requestHandler());

app.use((err, req, res, next) => {
  if (err.name === 'SequelizeDatabaseError') {

    sentry.configureScope(function(scope) {
      scope.setTag('bad-sql', 'Some bad sql');
      scope.setExtra('bad-sql-extra', 'Some extra bad sql');
    });
  }
  next(err);
});

app.use(sentry.Handlers.errorHandler());

The exception gets logged but I can't see the my custom tag Have you guys find a workaround?

Lms24 commented 1 year ago

Hi @rpanadero sorry for the delayed response on this issue and thanks again for the excellent reproduction!

I used your repro app, reproduced the problem and I think I know why the tags are not applied and how you can work around this:

To fix this, I'd advise to disable the TryCatch default integration. It should be safe to do so as the Angular error handler seems to catch timing-related/async errors. For your repro app, here's the adjusted code:

Sentry.init({
  dsn: '__DSN__',

  // filter out the try/catch integration and add the BrowserTracing integration
  integrations: (defaultIntegrations) => {
    return [
      ...defaultIntegrations.filter((i) => i.name !== 'TryCatch'),
      new BrowserTracing({/*...*/}),
    ];
  },
  // Set tracesSampleRate to 1.0 to capture 100%
  // of transactions for performance monitoring.
  // We recommend adjusting this value in production
  tracesSampleRate: 1.0,
});

We can think about disabling TryCatch by default for the Angular SDK but this has the downside that folks who only partially set up the SDK and don't use an Angular-internal error handler won't see these errors at all.

Let me know if this solution works for you :)

llgarrido commented 1 year ago

Hi @rpanadero sorry for the delayed response on this issue and thanks again for the excellent reproduction!

I used your repro app, reproduced the problem and I think I know why the tags are not applied and how you can work around this:

* In the sync case, everything works as expected and your error handler (or our error handler if it was used) would catch the error and report it to Sentry. So far so good!

* For functions that are executed in timer functions (like `setTimeout`), we use an integration called [`TryCatch`](https://docs.sentry.io/platforms/javascript/guides/angular/configuration/integrations/default/#trycatch) to instrument the callback, catch errors and send them directly to Sentry. This seems to happen _before_ the Angular-internal error handler is called. Hence, your tags are not applied in time.

To fix this, I'd advise to disable the TryCatch default integration. It should be safe to do so as the Angular error handler seems to catch timing-related/async errors. For your repro app, here's the adjusted code:

Sentry.init({
  dsn: '__DSN__',

  // filter out the try/catch integration and add the BrowserTracing integration
  integrations: (defaultIntegrations) => {
    return [
      ...defaultIntegrations.filter((i) => i.name !== 'TryCatch'),
      new BrowserTracing({/*...*/}),
    ];
  },
  // Set tracesSampleRate to 1.0 to capture 100%
  // of transactions for performance monitoring.
  // We recommend adjusting this value in production
  tracesSampleRate: 1.0,
});

We can think about disabling TryCatch by default for the Angular SDK but this has the downside that folks who only partially set up the SDK and don't use an Angular-internal error handler won't see these errors at all.

Let me know if this solution works for you :)

I have been experiencing the same problem from my own ErrorHandler implementation.

In my case you have solved the issue.

Lms24 commented 1 year ago

Thanks for reporting! I'm going to close the issue then in the meantime as the workaround seems to be sufficient. In case folks are still experiencing problems we can always reopen it.

sparky-raccoon commented 7 months ago

@Lms24 I have the same issue as @janvennemann, using promise based apis don't seem to change a thing (I would rather continue to use async anyway). Something like this below : 

partnersRouter.get('/debug-sentry', [], async function handler (req, res) {
  await new Promise((resolve) => {
    setTimeout(() => {
      resolve()
    }, 10000)
  })

  throw new Error('debug sentry')
})

Produces an error without HTTP context, which is unfortunate. The demo project provided by jan illustrates this, have you seen it ?