Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(angular): Set Angular ErrorHandler specific Exception Mechanism #3844

Merged
merged 7 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/angular/src/errorhandler.ts
@@ -1,6 +1,8 @@
import { HttpErrorResponse } from '@angular/common/http';
import { ErrorHandler as AngularErrorHandler, Inject, Injectable } from '@angular/core';
import * as Sentry from '@sentry/browser';
import { captureException } from '@sentry/browser';
import { addExceptionMechanism } from '@sentry/utils';

import { runOutsideAngular } from './zone';

Expand Down Expand Up @@ -40,7 +42,20 @@ class SentryErrorHandler implements AngularErrorHandler {
const extractedError = this._extractError(error) || 'Handled unknown error';

// Capture handled exception and send it to Sentry.
const eventId = runOutsideAngular(() => Sentry.captureException(extractedError));
const eventId = runOutsideAngular(() =>
captureException(extractedError, scope => {
scope.addEventProcessor(event => {
addExceptionMechanism(event, {
type: 'angular',
handled: false,
});

return event;
});

return scope;
}),
);

// When in development mode, log the error to console for immediate feedback.
if (this._options.logErrors) {
Expand Down
124 changes: 124 additions & 0 deletions packages/angular/test/errorhandler.test.ts
@@ -0,0 +1,124 @@
import { createErrorHandler, SentryErrorHandler } from '../src/errorhandler';

import * as SentryBrowser from '@sentry/browser';
import * as SentryUtils from '@sentry/utils';
import { HttpErrorResponse } from '@angular/common/http';
import { Scope } from '@sentry/browser';

const FakeScope = new Scope();

jest.mock('@sentry/browser', () => {
const original = jest.requireActual('@sentry/browser');
return {
...original,
captureException: (err: unknown, cb: (arg0?: unknown) => unknown) => {
cb(FakeScope);
return original.captureException(err, cb);
},
};
});

const captureExceptionSpy = jest.spyOn(SentryBrowser, 'captureException');

jest.spyOn(console, 'error').mockImplementation();

describe('SentryErrorHandler', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('`createErrorHandler `creates a SentryErrorHandler with an empty config', () => {
const errorHandler = createErrorHandler();

expect(errorHandler).toBeInstanceOf(SentryErrorHandler);
});

it('handleError method assigns the correct mechanism', () => {
const addEventProcessorSpy = jest.spyOn(FakeScope, 'addEventProcessor').mockImplementationOnce(callback => {
callback({}, { event_id: 'fake-event-id' });
return FakeScope;
});

const addExceptionMechanismSpy = jest.spyOn(SentryUtils, 'addExceptionMechanism');

const errorHandler = createErrorHandler();
errorHandler.handleError(new Error('test'));

expect(addEventProcessorSpy).toBeCalledTimes(1);
expect(addExceptionMechanismSpy).toBeCalledTimes(1);
expect(addExceptionMechanismSpy).toBeCalledWith({}, { handled: false, type: 'angular' });
});

it('handleError method extracts `null` error', () => {
createErrorHandler().handleError(null);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function));
});

it('handleError method extracts `undefined` error', () => {
createErrorHandler().handleError(undefined);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function));
});

it('handleError method extracts a string', () => {
const str = 'sentry-test';
createErrorHandler().handleError(str);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(str, expect.any(Function));
});

it('handleError method extracts an empty Error', () => {
const err = new Error();
createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(err, expect.any(Function));
});

it('handleError method extracts an Error with `ngOriginalError`', () => {
const ngErr = new Error('sentry-ng-test');
const err = {
ngOriginalError: ngErr,
};

createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(ngErr, expect.any(Function));
});

it('handleError method extracts an `HttpErrorResponse` with `Error`', () => {
const httpErr = new Error('sentry-http-test');
const err = new HttpErrorResponse({ error: httpErr });

createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(httpErr, expect.any(Function));
});

it('handleError method extracts an `HttpErrorResponse` with `ErrorEvent`', () => {
const httpErr = new ErrorEvent('http', { message: 'sentry-http-test' });
const err = new HttpErrorResponse({ error: httpErr });

createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith('sentry-http-test', expect.any(Function));
});

it('handleError method extracts an `HttpErrorResponse` with string', () => {
const err = new HttpErrorResponse({ error: 'sentry-http-test' });
createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(
'Server returned code 0 with body "sentry-http-test"',
expect.any(Function),
);
});
});
22 changes: 20 additions & 2 deletions packages/browser/src/eventbuilder.ts
@@ -1,4 +1,13 @@
import { Event, EventHint, Exception, Severity, SeverityLevel, StackFrame, StackParser } from '@sentry/types';
import {
Event,
EventHint,
Exception,
Mechanism,
Severity,
SeverityLevel,
StackFrame,
StackParser,
} from '@sentry/types';
import {
addExceptionMechanism,
addExceptionTypeValue,
Expand Down Expand Up @@ -149,8 +158,17 @@ export function eventFromException(
): PromiseLike<Event> {
const syntheticException = (hint && hint.syntheticException) || undefined;
const event = eventFromUnknownInput(stackParser, exception, syntheticException, attachStacktrace);
addExceptionMechanism(event); // defaults to { type: 'generic', handled: true }
const providedMechanism: Mechanism | undefined =
hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism;
const mechanism: Mechanism = providedMechanism || {
handled: true,
type: 'generic',
};

addExceptionMechanism(event, mechanism);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're using the scope eventprocessor - do we still need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default fallback mechanism seems unneeded at this point, removing it.

Not sure about the rest, though.
The original intent on this PR was to mimic how mechanisms are passed in Node SDK on browser SDK, but a lot changed since, so I don't know if that's still relevant.

Still, it looks like this can also be moved into scope.addEventProcessor to be consistent, if we are keeping it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to change the Node approach to how we do it in Express (and Angular). (But let's do this in another PR). Which makes me think that we don't need to query the hint here. The original call to addExceptionMechanism shouldn't be a problem because it won't overwrite the already set mechanism with the default value (see here)

So overall, I don't think we need this change here (unless I'm missing something)

event.level = 'error';

if (hint && hint.event_id) {
event.event_id = hint.event_id;
}
Expand Down