Instrument incorrectly wrapping error causing sentry functions to fail

[Sammaye]

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 package are you using?

@sentry/angular

SDK Version

7.12.1

Framework Version

Angular 10.2.5

Link to Sentry event

https://sentry.io/organizations/delenta/issues/3583358067/?query=is%3Aunresolved

Steps to Reproduce

Used this in my main.ts:

if (environment.ENABLE_SENTRY) {
  let dialogShownTimeout: any;

  Sentry.init({
    environment: environment.ENV,
    dsn: "https://xxx",
    integrations: [
      new GlobalHandlers({
        onerror: false,
        onunhandledrejection: false,
      }),
      new BrowserTracing({
        tracingOrigins: [
          "localhost",
          environment.API_SERVICE_BASE
        ],
        routingInstrumentation: Sentry.routingInstrumentation,
      }),
    ],
    beforeSend(event, hint) {
      // Check if it is an exception
      if (event.exception) {
        const userId = null;
        const name = null;
        const email =null;

        if (userId) {
          Sentry.setUser({
            id: userId,
            username: name,
            email
          });
        }

        event.fingerprint = [
          '{{ default }}',
          event.message ?? (
            hint.originalException instanceof Error
              ? hint.originalException.message
              : hint.originalException
          ),
          event.request.url,
        ];

        // Timeout ensures only the last dialog is sent if multiple errors are received
        if (dialogShownTimeout) {
          clearTimeout(dialogShownTimeout);
        }

        dialogShownTimeout = setTimeout(() => {
          Sentry.showReportDialog({
            eventId: event.event_id,
            user: {
              name,
              email,
            }
          });
        }, 250);

        return html2canvas(document.body, {logging: false}).then(async (canvas) => {
          const screenshotData = convertDataURIToBinary(canvas.toDataURL("image/jpeg", 0.5));
          hint.attachments = [{filename: "screenshot.jpg", data: screenshotData}];
          return event;
        }).catch(() => {
          // We failed to get a screenshot still give us error
          return event;
        });
      }
    },
    // Set tracesSampleRate to 1.0 to capture 100%
    // of transactions for performance monitoring.
    // We recommend adjusting this value in production
    tracesSampleRate: 1.0,
    ignoreErrors: [
      // Random plugins/extensions
      'top.GLOBALS',
      // See: http://blog.errorception.com/2012/03/tale-of-unfindable-js-error.html
      'originalCreateNotification',
      'canvas.contentDocument',
      'MyApp_RemoveAllHighlights',
      'http://tt.epicplay.com',
      'Can\'t find variable: ZiteReader',
      'jigsaw is not defined',
      'ComboSearch is not defined',
      'http://loading.retry.widdit.com/',
      'atomicFindClose',
      // Facebook borked
      'fb_xd_fragment',
      // ISP "optimizing" proxy - `Cache-Control: no-transform` seems to reduce this. (thanks @acdha)
      // See http://stackoverflow.com/questions/4113268/how-to-stop-javascript-injection-from-vodafone-proxy
      'bmi_SafeAddOnload',
      'EBCallBackMessageReceived',
      // See http://toolbar.conduit.com/Developer/HtmlAndGadget/Methods/JSInjection.aspx
      'conduitPage',
      // Generic error code from errors outside the security sandbox
      // You can delete this if using raven.js > 1.0, which ignores these automatically.
      'Script error.',
      // Avast extension error
      "_avast_submit",
      // This seems to get thrown when we get errors in the error handler that are not constructed right
      'Non-Error exception captured',
    ],
    denyUrls: [
      // Google Adsense
      /pagead\/js/i,
      // Facebook flakiness
      /graph\.facebook\.com/i,
      // Facebook blocked
      /connect\.facebook\.net\/en_US\/all\.js/i,
      // Woopra flakiness
      /eatdifferent\.com\.woopra-ns\.com/i,
      /static\.woopra\.com\/js\/woopra\.js/i,
      // Chrome extensions
      /extensions\//i,
      /^chrome:\/\//i,
      // Other plugins
      /127\.0\.0\.1:4001\/isrunning/i,  // Cacaoweb
      /webappstoolbarba\.texthelp\.com\//i,
      /metrics\.itunes\.apple\.com\.edgesuite\.net\//i
    ]
  });
}

function convertDataURIToBinary(dataURI) {
  var base64Index = dataURI.indexOf(';base64,') + ';base64,'.length;
  var base64 = dataURI.substring(base64Index);
  var raw = window.atob(base64);
  var rawLength = raw.length;
  var array = new Uint8Array(new ArrayBuffer(rawLength));

  for(let i = 0; i < rawLength; i++) {
    array[i] = raw.charCodeAt(i);
  }
  return array;
}

With a app module:

@NgModule({
  declarations: [
    AppComponent,
  ],
  imports: [
    BrowserModule,
    BrowserAnimationsModule,
    HttpClientModule,
    HttpClientJsonpModule,
    AppRoutingModule,
    SharedComponentModule,
  ],
  providers: [
    PreloadStrategy,
    ConfigInit(),
    {
      provide: ErrorHandler,
      useClass: GlobalErrorHandler
    },
    {
      provide: Sentry.TraceService,
      deps: [Router],
    },
    {
      provide: APP_INITIALIZER,
      useFactory: () => () => {},
      deps: [Sentry.TraceService],
      multi: true,
    },
    {
      provide: HTTP_INTERCEPTORS,
      useClass: HttpInterceptorService,
      multi: true
    },
  ],
  bootstrap: [AppComponent]
})
export class AppModule {
}

With a error handler like:

export class GlobalErrorHandler implements ErrorHandler {

  constructor(private injector: Injector) { }

  handleError(error: Error | HttpErrorResponse): void {
    const chunkFailedMessage = /Loading chunk [\d]+ failed/;
    if (chunkFailedMessage.test(error.message)) {
      if (navigator.onLine) {
        // Only reload if the user is online, otherwise a message will show
        // to tell them they are offline and the app will not work right

        // We do not ask for confirmation for this since this is considered
        // an unrecoverable state, the user is navigating, causing bundles to
        // try to load, and they cannot progress since their cache is dead and
        // the version they are seeking no longer exists
        window.location.reload();
      }
    } else {
      if (error instanceof HttpErrorResponse && !navigator.onLine) {
        // we don't care abut errors while offline even if we could get them
        return;
      }

      console.error(error);

      const errorStatus = error instanceof HttpErrorResponse ? error.status : 0;

      if (errorStatus === 0 || errorStatus >= 500) {
        // This will log it to sentry and show an error message asking user to leave feedback
        // You can find the configuration for this in the main.ts file
        Sentry.captureException(this._extractError(error));
      }
    }
  }

  /**
   * Used to pull a desired value that will be used to capture an event out of the raw value captured by ErrorHandler.
   */
  protected _extractError(error: unknown): unknown {
    return this._defaultExtractor(error);
  }

  /**
   * Default implementation of error extraction that handles default error wrapping, HTTP responses, ErrorEvent and few other known cases.
   */
  protected _defaultExtractor(errorCandidate: unknown): unknown {
    let error = errorCandidate;

    // Try to unwrap zone.js error.
    // https://github.com/angular/angular/blob/master/packages/core/src/util/errors.ts
    if (error && (error as { ngOriginalError: Error }).ngOriginalError) {
      error = (error as { ngOriginalError: Error }).ngOriginalError;
    }

    // We can handle messages and Error objects directly.
    if (typeof error === 'string' || error instanceof Error) {
      return error;
    }

    // If it's http module error, extract as much information from it as we can.
    if (error instanceof HttpErrorResponse) {
      // The `error` property of http exception can be either an `Error` object, which we can use directly...
      if (error.error instanceof Error) {
        return error.error;
      }

      // ... or an`ErrorEvent`, which can provide us with the message but no stack...
      if (error.error instanceof ErrorEvent && error.error.message) {
        return error.error.message;
      }

      // ...or the request body itself, which we can use as a message instead.
      if (typeof error.error === 'string') {
        return `Server returned code ${error.status} with body "${error.error}"`;
      }

      // If we don't have any detailed information, fallback to the request message itself.
      return error.message;
    }

    // Nothing was extracted, fallback to default error message.
    return null;
  }
}

I used a function like:

		this.dashboardService.getDashboardStatistic('dashboard').subscribe(statistics => {
      const g = statistics.t.y;

			this.statistic = <Statistic>statistics;
			this.isDataLoaded = true;
		});

Where by assignment of g would clearly raise an error.

Expected Result

Sentry would let the global error handler take this, as it works when I remove sentry

Actual Result

Sentry seems to throw this error somewhere else and avoid my global error handler altogether, I suspect the instrument.js and not only that but certain functions, while many others, do not work, like setUser(). In my main.ts everything there works but setUser when this happens, for every other error setUser works just fine.

This also worries me since I have a slightly custom error handler which checks for unrecoverable app state and does custom logic for that, I am concerned this could interfere with customer workflows if I release sentry to our live servers.

I suspect this to be a bug since it seems to break some of sentry's own functions when it happens, such as setUser(), as I said.

[lobsterkatie]

Hi, @Sammaye.

Would you be able to provide a minimal repro of this issue (including all the parts needed to run the app)? It would help us a lot with debugging, since not everyone on the team is an angular expert.

(At a quick glance, nothing in what you've done jumps out at me as obviously wrong.)

Thanks!

[Sammaye]

I'll try, though very busy

[lobsterkatie]

Great, thanks, @Sammaye.

@Lms24, when you're back, mind taking a look at this?

[Sammaye]

Ok I have produced one and I was wrong, it does call my error handler, but it still says in Sentry that the instrument was the mechanism and it still will not attach the user to the error but will in the modal, code will follow

[Sammaye]

test.zip

Here is the code. Basically you will see in the global error handler I put a line like console.log('in error handler'); and this proves that it actually calls my global error handler and even calls the capture, but this is the result https://sentry.io/organizations/delenta/issues/3586573703/?query=is%3Aunresolved

[Sammaye]

Something else I have noticed there as well the stack trace is messed up, take this error from my live app https://sentry.io/organizations/delenta/issues/3586583907/?query=is%3Aunresolved where it easily shows what module it crashed in and, if I upload my source maps, would instantly tell me the line it crashed on

[Sammaye]

As a note: I did make the test in angular 14 and not 10, which while proves it isn't my angular version also mean the stack trace difference is likely from version but still quite valid since if I upgrade I might lose useful stack traces if that's true

[Sammaye]

Ok further update, I thought I would test the scenario that does assign user for me in the new app and it doesn't: https://sentry.io/organizations/delenta/issues/3586723234/events/3eb54a8fbc26473f9e45326c22ba2212/?project=6736456&query=is%3Aunresolved and here is a screenshot of the process of using setUser, I commented out the if statement around it as well just in case

[Sammaye]

And here is an error of the same kind form my live app, no nodejs server running, but it attaches the user correctly https://sentry.io/organizations/delenta/issues/3586740920/?query=is%3Aunresolved and the only real difference I can find it maybe angular or rxjs version, this is really quite inconsistent

[Sammaye]

Any updates?

[lobsterkatie]

Our angular expert is currently out with covid. ☹️ I've asked him to take a look when he gets back, though.

[Lms24]

Hi @Sammaye apologies for only getting back to you now. Thanks for providing a reproduction example and the links to your events!

So just for me to confirm/summarize all of your observations (please feel free to correct and add whatever I forgot):

• It seems like you initialize the SDK correctly but instead of using the ErrorHandler we provide in the SDK, you have your own, custom GlobalErrorHandler (which is okay, this should work but of course this means that you have to take care of sending stuff to Sentry).
• You disabled our global onerror/onunhandledrejection instrumentation, so our SDK won't capture stuff that bubbles up to these global handlers.
• The main problem is that the events that are sent to Sentry do not contain user data, although you set them in beforeSend, correct?
• However, apart from that it seems like stack traces are incomplete or not what you'd expect, correct?

I'm still looking into what you could do to make this better. Just our of curiosity: Have you tried using Sentry's ErrorHandler that ships with the Angular SDK? If yes, does this improve the situation?

I have a feeling that your problem might be related to what has been reported in #5417. Given this, my gut feeling currently is that this has something to do with scope bleed in async contexts which is a hard problem to solve (we've been working on this for a long time).

Anyway, I'm still looking if there is something else we can do here, so I'll keep you posted. Let me know if you've found more clues or workarounds in the meantime :)

[Sammaye]

The main problem is that the events that are sent to Sentry do not contain user data, although you set them in beforeSend, correct?

Yes

However, apart from that it seems like stack traces are incomplete or not what you'd expect, correct?

Indeed

Have you tried using Sentry's ErrorHandler that ships with the Angular SDK? If yes, does this improve the situation?

I think I tried it and it didn't, tbh my error handler is actually an extension of yours, just with a few custom little bit added on for me.

I have a feeling that your problem might be related to what has been reported in #5417.

Thing is, other properties work, so if you were to make tags settable the same as other property that do work, like attachments, then this would solve it entirely

Edit: Sorry I mean user, tags can be set like that

[Lms24]

Hi @Sammaye

I think I found the problem for the missing user data: In your beforeSend hook, you're calling Sentry.setUser to set the user data. Internally in the SDK, this will set the User data on the currently active scope. However, scope data is applied to the event passed in beforeSend before beforeSend is invoked. Hence, the call to Sentry.setUser is too late here. Meaning, for the error event that is in beforeSend, user data passed to Sentry.setUser is not applied anymore. However, for subsequent events, it should again be applied. To confirm this, I took a quick look at the pageload transactions your repro app is sending and there, it seems like all events have the correct user.

I think you have two options to work around this:

• Call Sentry.setUser as soon as you actually have user data. I don't know how your app works but in a typical SPA this might be after some sort of successful login when you retrieve user data.
• If this doesn't apply to your app, or you simply only want to only set it in beforeSend, do it like so:

event.user = {
  id: userId,
  username: name,
  email,
};

Please note the this second option does not set the user globally in the Sentry SDK, meaning that it only adds it to the event that's going through beforeSend. If you want to add user data to your transactions (as it currently works), you still have to call Sentry.setUser somewhere.

In regards to incomplete stack traces: In the repro you provided, the exception I get in the console is a net::ERR_CONNECTION_REFUSED because the API I'm trying to call is obv not running on my system.

If I console log this error, I can see that error is of instance HttpErrorResponse and error.error is an instance of ProgressEvent which causes your ErrorHandler's default extractor (which seems to be identical to our code) to return a string.

This in turn means that a string is passed to Sentry.captureException which will work but give you an error without a stack trace.

Now, we could try to handle this ProgressEvent a little better but it seems like neither ProgressEvent nor the object wrapped around it have a stacktrace so there's nothing we can really do in this situation.

I hope this helps a little (at least the user problem should be fixable). Let me know if you have further questions!

[Sammaye]

If you want to add user data to your transactions (as it currently works), you still have to call Sentry.setUser somewhere.

Hmm, I didn't know there was a difference, is there any documentation related to this?

One thing that would concern m,e about setting it globally is catching start up errors and stuff, so I would probably want to do it both, set it globally if it can and then set it each error as a backup for obscure errors.

In regards to incomplete stack traces: In the repro you provided, the exception I get in the console

This was actually comparing two dev instances:

V10

V13

Re-reading my comments, I think this is since angular v13+ actually compiles my dev to 4 files, mian.js being the app itself, I guess this might be since the test app has no lazy loaded modules, maybe some change in Angular itself. I guess as long as I suddenly won't lose sight of errors by upgrading angular that's fine

[Sammaye]

The only docs I can find on setting a user are setting them globally https://docs.sentry.io/platforms/javascript/enriching-events/identify-user/ so yeah, not sure what the difference is

[Lms24]

Hmm, I didn't know there was a difference, is there any documentation related to this

The main thing here is that beforeSend is not called transaction events but only for errors. In your case it seems to work out because the error event is sent first (where you called Sentry.setUser which set it globally) before the pageload transaction finishes. (In the repro app at least.) Our documentation should be updated to reflect this, you're right.

Note that we definitely want to change this to give users a way of controlling sending of transaction events just like error events. We're currently evaluating how to do this best and we opened an RFC for it.

In the meantime, to edit (or discard) transaction events, you could register an EventProcessor on the scope to make modifications to or manually drop transactions

One thing that would concern me about setting it globally is catching start up errors and stuff, so I would probably want to do it both, set it globally if it can and then set it each error as a backup for obscure errors.

I think that's a timing problem we can't do much about, if I understand you correctly, because surely, errors can occur before you even know the user data. But overall, setting user data globally and for each event shouldn't hurt. Just not sure if it improves this timing problem.

[Lms24]

The only docs I can find on setting a user are setting them globally https://docs.sentry.io/platforms/javascript/enriching-events/identify-user/ so yeah, not sure what the difference is

If you set the user globally (Sentry.setUser), it will be attached to all outgoing events (errors, transactions, etc)

[Sammaye]

I think that's a timing problem we can't do much about, if I understand you correctly, because surely, errors can occur before you even know the user data.

In this case no, since it happens within a page that can only be accessed by logged in users, so angular knows of the user, I am guessing you mean it is a timing issue between Sentry.setUser and dispatch of the event?

[Lms24]

In this case no, since it happens within a page that can only be accessed by logged in users, so angular knows of the user

Ahh alright.

guessing you mean it is a timing issue between Sentry.setUser and dispatch of the event

Since your app already knows the user data, then yes, that's the only timing problem left. But I think if you call Sentry.setUser in main.ts (directly after Sentry.init) you should be good. Though, calling Sentry.setUser globally and setting it explicitly in beforeSend shouldn't cause problems if you encounter timing issues.

[Sammaye]

So the fact that it is could mean a deeper issue, ok I will try and few things a bit later and see if I can get any more insight

[Lms24]

I'll go ahead and close this issue since it seems like we found a solution. Feel free to comment if you still have questions.

[Sammaye]

Yeah I just tested t, if I set it first thing in main.ts this error now works with full user info, so there is defo some kind of issue between Sentry.setUser and the beforeSend of an event, but it;s not vital, I will use globally since using it in beforeSend likely will mess up transactions as well like you said