Skip to content

ref(core): Switch all internal uses of captureException to use @sentry/core #6488

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

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/angular/ng-package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
"entryFile": "src/index.ts",
"umdModuleIds": {
"@sentry/browser": "Sentry",
"@sentry/core": "Sentry.core",
"@sentry/utils": "Sentry.util"
}
},
"whitelistedNonPeerDependencies": ["@sentry/browser", "@sentry/utils", "@sentry/types", "tslib"],
"whitelistedNonPeerDependencies": ["@sentry/browser", "@sentry/core", "@sentry/utils", "@sentry/types", "tslib"],
"assets": ["README.md", "LICENSE"]
}
1 change: 1 addition & 0 deletions packages/angular/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
},
"dependencies": {
"@sentry/browser": "7.24.2",
"@sentry/core": "7.24.2",
"@sentry/types": "7.24.2",
"@sentry/utils": "7.24.2",
"tslib": "^2.0.0"
Expand Down
4 changes: 3 additions & 1 deletion packages/angular/src/errorhandler.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
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 { captureException } from '@sentry/core';
import { addExceptionMechanism } from '@sentry/utils';

import { runOutsideAngular } from './zone';
Expand Down Expand Up @@ -55,8 +55,10 @@ class SentryErrorHandler implements AngularErrorHandler {
captureException(extractedError, scope => {
scope.addEventProcessor(event => {
addExceptionMechanism(event, {
// TODO: Should this be 'instrument'?
type: 'angular',
handled: false,
// will default to `caughtByUser: false`
});

return event;
Expand Down
9 changes: 5 additions & 4 deletions packages/angular/test/errorhandler.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { HttpErrorResponse } from '@angular/common/http';
import * as SentryBrowser from '@sentry/browser';
import { Scope } from '@sentry/browser';
import * as SentryBrowser from '@sentry/browser';
import * as SentryCore from '@sentry/core';
import * as SentryUtils from '@sentry/utils';

import { createErrorHandler, SentryErrorHandler } from '../src/errorhandler';

const FakeScope = new Scope();

jest.mock('@sentry/browser', () => {
const original = jest.requireActual('@sentry/browser');
jest.mock('@sentry/core', () => {
const original = jest.requireActual('@sentry/core');
return {
...original,
captureException: (err: unknown, cb: (arg0?: unknown) => unknown) => {
Expand All @@ -18,7 +19,7 @@ jest.mock('@sentry/browser', () => {
};
});

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

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

Expand Down
44 changes: 26 additions & 18 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,23 +121,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
public captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined {
// ensure we haven't captured this very object before
if (checkOrSetAlreadyCaught(exception)) {
__DEBUG_BUILD__ && logger.log(ALREADY_SEEN_ERROR);
return;
}

let eventId: string | undefined = hint && hint.event_id;

this._process(
this.eventFromException(exception, hint)
.then(event => this._captureEvent(event, hint, scope))
.then(result => {
eventId = result;
}),
);

return eventId;
return this._captureException(exception, hint, scope);
}

/**
Expand Down Expand Up @@ -579,6 +563,30 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
}
}

/**
* Internal implementation of public `captureException` method
*/
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/no-explicit-any
protected _captureException(exception: any, hint?: EventHint, scope?: Scope): string | undefined {
// ensure we haven't captured this very object before
if (checkOrSetAlreadyCaught(exception)) {
__DEBUG_BUILD__ && logger.log(ALREADY_SEEN_ERROR);
return;
}

let eventId: string | undefined = hint && hint.event_id;

this._process(
this.eventFromException(exception, hint)
.then(event => this._captureEvent(event, hint, scope))
.then(result => {
eventId = result;
}),
);

return eventId;
}

/**
* Processes the event and logs an error in case of rejection
* @param event
Expand Down Expand Up @@ -699,7 +707,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
throw reason;
}

this.captureException(reason, {
this._captureException(reason, {
data: {
__sentry__: true,
},
Expand Down
14 changes: 14 additions & 0 deletions packages/core/src/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,23 @@ import { Scope } from './scope';
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
export function captureException(exception: any, captureContext?: CaptureContext): ReturnType<Hub['captureException']> {
// eslint-disable-next-line @sentry-internal/sdk/captureException-from-core -- This is the one place this is allowed
return getCurrentHub().captureException(exception, { captureContext });
}

/**
* A version of `captureException` which takes a hint as its second parameter, primarily for internal use.
*
* @param exception An exception-like object
* @param hint Metadata for use in capturing event
* @returns The generated event id
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
export function captureExceptionWithHint(exception: any, hint: EventHint): ReturnType<Hub['captureException']> {
// eslint-disable-next-line @sentry-internal/sdk/captureException-from-core -- This is the one place this is allowed
return getCurrentHub().captureException(exception, hint);
}

/**
* Captures a message event and sends it to Sentry.
*
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ export class Hub implements HubInterface {
const eventId = (this._lastEventId = hint && hint.event_id ? hint.event_id : uuid4());
const syntheticException = new Error('Sentry syntheticException');
this._withClient((client, scope) => {
// eslint-disable-next-line @sentry-internal/sdk/captureException-from-core
client.captureException(
exception,
{
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export type { Carrier, Layer } from './hub';
export {
addBreadcrumb,
captureException,
captureExceptionWithHint,
captureEvent,
captureMessage,
configureScope,
Expand Down
3 changes: 2 additions & 1 deletion packages/core/test/lib/base.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,8 @@ describe('BaseClient', () => {

const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
const client = new TestClient(options);
const captureExceptionSpy = jest.spyOn(client, 'captureException');
// @ts-ignore Need to spy on protected method
const captureExceptionSpy = jest.spyOn(client, '_captureException');
const loggerWarnSpy = jest.spyOn(logger, 'warn');
const scope = new Scope();
const exception = new Error('sorry');
Expand Down
4 changes: 4 additions & 0 deletions packages/eslint-config-sdk/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ module.exports = {
// We want to prevent async await usage in our files to prevent uncessary bundle size. Turned off in tests.
'@sentry-internal/sdk/no-async-await': 'error',

// Enforce internal use of `captureException` from `@sentry/core` rather than `@sentry/browser`, `@sentry/node`,
// or any wrapper SDK thereof. This prevents manual-usage mechanism data from getting erroneously included.
'@sentry-internal/sdk/captureException-from-core': 'error',

// JSDOC comments are required for classes and methods. As we have a public facing codebase, documentation,
// even if it may seems excessive at times, is important to emphasize. Turned off in tests.
'jsdoc/require-jsdoc': [
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin-sdk/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ module.exports = {
rules: {
'no-async-await': require('./rules/no-async-await'),
'no-eq-empty': require('./rules/no-eq-empty'),
'captureException-from-core': require('./rules/captureException-from-core'),
},
};
Loading