Skip to content

ref(core): Reduce inboundfilters bundle size #4625

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Mar 23, 2022
Merged

ref(core): Reduce inboundfilters bundle size #4625

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5515dd8
ref(core): Reduce inboundfilters bundle size
AbhiPrasad Jan 31, 2022
a4e6425
add tests
AbhiPrasad Feb 23, 2022
48134b2
move into fixtures
AbhiPrasad Feb 23, 2022
174ffe2
yarn fix
AbhiPrasad Feb 23, 2022
c1e171d
address PR review
AbhiPrasad Feb 24, 2022
7f01599
move fixtures around
AbhiPrasad Feb 24, 2022
ecb6460
review comment
AbhiPrasad Feb 24, 2022
beffc9f
Merge branch 'master' into abhi-private-inboundfilter
AbhiPrasad Mar 18, 2022
f2bfe03
remove type alias
AbhiPrasad Mar 18, 2022
c277576
Merge branch 'master' into abhi-private-inboundfilter
AbhiPrasad Mar 21, 2022
94eac23
address PR review feedback
AbhiPrasad Mar 23, 2022
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
303 changes: 146 additions & 157 deletions packages/core/src/integrations/inboundfilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
import { Event, Integration, StackFrame } from '@sentry/types';
import { Event, EventProcessor, Hub, Integration, StackFrame } from '@sentry/types';
import { getEventDescription, isDebugBuild, isMatchingPattern, logger } from '@sentry/utils';

// "Script error." is hard coded into browsers for errors that it can't read.
// this is the result of a script being pulled in from an external domain and CORS.
const DEFAULT_IGNORE_ERRORS = [/^Script error\.?$/, /^Javascript error: Script error\.? on line 0$/];

/** JSDoc */
interface InboundFiltersOptions {
export interface InboundFiltersOptions {
allowUrls: Array<string | RegExp>;
denyUrls: Array<string | RegExp>;
ignoreErrors: Array<string | RegExp>;
Expand Down Expand Up @@ -36,190 +35,180 @@ export class InboundFilters implements Integration {
/**
* @inheritDoc
*/
public setupOnce(): void {
public setupOnce(addGlobalEventProcessor: (processor: EventProcessor) => void, getCurrentHub: () => Hub): void {
addGlobalEventProcessor((event: Event) => {
const hub = getCurrentHub();
if (!hub) {
return event;
}
const self = hub.getIntegration(InboundFilters);
if (self) {
const client = hub.getClient();
const clientOptions = client ? client.getOptions() : {};
// This checks prevents most of the occurrences of the bug linked below:
// https://github.com/getsentry/sentry-javascript/issues/2622
// The bug is caused by multiple SDK instances, where one is minified and one is using non-mangled code.
// Unfortunatelly we cannot fix it reliably (thus reserved property in rollup's terser config),
// as we cannot force people using multiple instances in their apps to sync SDK versions.
const options = typeof self._mergeOptions === 'function' ? self._mergeOptions(clientOptions) : {};
if (typeof self._shouldDropEvent !== 'function') {
return event;
if (hub) {
const self = hub.getIntegration(InboundFilters);
if (self) {
const client = hub.getClient();
const clientOptions = client ? client.getOptions() : {};
const options = _mergeOptions(self._options, clientOptions);
return _shouldDropEvent(event, options) ? null : event;
}
return self._shouldDropEvent(event, options) ? null : event;
}
return event;
});
}
}

/** JSDoc */
private _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
if (this._isSentryError(event, options)) {
isDebugBuild() &&
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
return true;
}
if (this._isIgnoredError(event, options)) {
isDebugBuild() &&
logger.warn(
`Event dropped due to being matched by \`ignoreErrors\` option.\nEvent: ${getEventDescription(event)}`,
);
return true;
}
if (this._isDeniedUrl(event, options)) {
isDebugBuild() &&
logger.warn(
`Event dropped due to being matched by \`denyUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${this._getEventFilterUrl(event)}`,
);
return true;
}
if (!this._isAllowedUrl(event, options)) {
isDebugBuild() &&
logger.warn(
`Event dropped due to not being matched by \`allowUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${this._getEventFilterUrl(event)}`,
);
return true;
}
return false;
/** JSDoc */
export function _mergeOptions(
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) Wondering if we should drop the underscore in the extracted function names...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave it here for now, but I'll remove the JSDoc as per your comments below.

I left it with the underscore initially to minimize the diff, and make it clear as possible how the functions moved.

internalOptions: Partial<InboundFiltersOptions> = {},
clientOptions: Partial<InboundFiltersOptions> = {},
): Partial<InboundFiltersOptions> {
return {
allowUrls: [
// eslint-disable-next-line deprecation/deprecation
...(internalOptions.whitelistUrls || []),
...(internalOptions.allowUrls || []),
// eslint-disable-next-line deprecation/deprecation
...(clientOptions.whitelistUrls || []),
...(clientOptions.allowUrls || []),
],
denyUrls: [
// eslint-disable-next-line deprecation/deprecation
...(internalOptions.blacklistUrls || []),
...(internalOptions.denyUrls || []),
// eslint-disable-next-line deprecation/deprecation
...(clientOptions.blacklistUrls || []),
...(clientOptions.denyUrls || []),
],
ignoreErrors: [
...(internalOptions.ignoreErrors || []),
...(clientOptions.ignoreErrors || []),
...DEFAULT_IGNORE_ERRORS,
],
ignoreInternal: internalOptions.ignoreInternal !== undefined ? internalOptions.ignoreInternal : true,
};
}

/** JSDoc */
export function _shouldDropEvent(event: Event, options: Partial<InboundFiltersOptions>): boolean {
if (_isSentryError(event, options.ignoreInternal)) {
isDebugBuild() &&
logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
return true;
}
if (_isIgnoredError(event, options.ignoreErrors)) {
isDebugBuild() &&
logger.warn(
`Event dropped due to being matched by \`ignoreErrors\` option.\nEvent: ${getEventDescription(event)}`,
);
return true;
}
if (_isDeniedUrl(event, options.denyUrls)) {
isDebugBuild() &&
logger.warn(
`Event dropped due to being matched by \`denyUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${_getEventFilterUrl(event)}`,
);
return true;
}
if (!_isAllowedUrl(event, options.allowUrls)) {
isDebugBuild() &&
logger.warn(
`Event dropped due to not being matched by \`allowUrls\` option.\nEvent: ${getEventDescription(
event,
)}.\nUrl: ${_getEventFilterUrl(event)}`,
);
return true;
}
return false;
}

/** JSDoc */
private _isSentryError(event: Event, options: Partial<InboundFiltersOptions>): boolean {
if (!options.ignoreInternal) {
return false;
}
/** JSDoc */
function _isIgnoredError(event: Event, ignoreErrors?: Array<string | RegExp>): boolean {
if (!ignoreErrors || !ignoreErrors.length) {
return false;
}

try {
// @ts-ignore can't be a sentry error if undefined
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return event.exception.values[0].type === 'SentryError';
} catch (e) {
// ignore
}
return _getPossibleEventMessages(event).some(message =>
ignoreErrors.some(pattern => isMatchingPattern(message, pattern)),
);
}

/** JSDoc */
function _isDeniedUrl(event: Event, denyUrls?: Array<string | RegExp>): boolean {
// TODO: Use Glob instead?
if (!denyUrls || !denyUrls.length) {
return false;
}
const url = _getEventFilterUrl(event);
return !url ? false : denyUrls.some(pattern => isMatchingPattern(url, pattern));
}

/** JSDoc */
private _isIgnoredError(event: Event, options: Partial<InboundFiltersOptions>): boolean {
if (!options.ignoreErrors || !options.ignoreErrors.length) {
return false;
}

return this._getPossibleEventMessages(event).some(message =>
// Not sure why TypeScript complains here...
(options.ignoreErrors as Array<RegExp | string>).some(pattern => isMatchingPattern(message, pattern)),
);
/** JSDoc */
function _isAllowedUrl(event: Event, allowUrls?: Array<string | RegExp>): boolean {
// TODO: Use Glob instead?
if (!allowUrls || !allowUrls.length) {
return true;
}
const url = _getEventFilterUrl(event);
return !url ? true : allowUrls.some(pattern => isMatchingPattern(url, pattern));
}

/** JSDoc */
private _isDeniedUrl(event: Event, options: Partial<InboundFiltersOptions>): boolean {
// TODO: Use Glob instead?
if (!options.denyUrls || !options.denyUrls.length) {
return false;
/** JSDoc */
function _getPossibleEventMessages(event: Event): string[] {
if (event.message) {
return [event.message];
}
if (event.exception) {
try {
const { type = '', value = '' } = (event.exception.values && event.exception.values[0]) || {};
return [`${value}`, `${type}: ${value}`];
} catch (oO) {
isDebugBuild() && logger.error(`Cannot extract message for event ${getEventDescription(event)}`);
return [];
}
const url = this._getEventFilterUrl(event);
return !url ? false : options.denyUrls.some(pattern => isMatchingPattern(url, pattern));
}
return [];
}

/** JSDoc */
private _isAllowedUrl(event: Event, options: Partial<InboundFiltersOptions>): boolean {
// TODO: Use Glob instead?
if (!options.allowUrls || !options.allowUrls.length) {
return true;
/** JSDoc */
function _isSentryError(event: Event, ignoreInternal?: boolean): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of the ignoreInternal argument? It seems unrelated to what the function is saying it does.

The if (_isSentryError(event, options.ignoreInternal)) { above would become if (!options.ignoreInternal && _isSentryError(event)) {. Might even save some bytes 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Good change, will do!

if (ignoreInternal) {
try {
// @ts-ignore can't be a sentry error if undefined
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return event.exception.values[0].type === 'SentryError';
} catch (e) {
// ignore
}
const url = this._getEventFilterUrl(event);
return !url ? true : options.allowUrls.some(pattern => isMatchingPattern(url, pattern));
}
return false;
}

/** JSDoc */
private _mergeOptions(clientOptions: Partial<InboundFiltersOptions> = {}): Partial<InboundFiltersOptions> {
return {
allowUrls: [
// eslint-disable-next-line deprecation/deprecation
...(this._options.whitelistUrls || []),
...(this._options.allowUrls || []),
// eslint-disable-next-line deprecation/deprecation
...(clientOptions.whitelistUrls || []),
...(clientOptions.allowUrls || []),
],
denyUrls: [
// eslint-disable-next-line deprecation/deprecation
...(this._options.blacklistUrls || []),
...(this._options.denyUrls || []),
// eslint-disable-next-line deprecation/deprecation
...(clientOptions.blacklistUrls || []),
...(clientOptions.denyUrls || []),
],
ignoreErrors: [
...(this._options.ignoreErrors || []),
...(clientOptions.ignoreErrors || []),
...DEFAULT_IGNORE_ERRORS,
],
ignoreInternal: typeof this._options.ignoreInternal !== 'undefined' ? this._options.ignoreInternal : true,
};
}
/** JSDoc */
function _getLastValidUrl(frames: StackFrame[] = []): string | null {
for (let i = frames.length - 1; i >= 0; i--) {
const frame = frames[i];

/** JSDoc */
private _getPossibleEventMessages(event: Event): string[] {
if (event.message) {
return [event.message];
if (frame && frame.filename !== '<anonymous>' && frame.filename !== '[native code]') {
return frame.filename || null;
}
if (event.exception) {
try {
const { type = '', value = '' } = (event.exception.values && event.exception.values[0]) || {};
return [`${value}`, `${type}: ${value}`];
} catch (oO) {
isDebugBuild() && logger.error(`Cannot extract message for event ${getEventDescription(event)}`);
return [];
}
}
return [];
}

/** JSDoc */
private _getLastValidUrl(frames: StackFrame[] = []): string | null {
for (let i = frames.length - 1; i >= 0; i--) {
const frame = frames[i];
return null;
}

if (frame && frame.filename !== '<anonymous>' && frame.filename !== '[native code]') {
return frame.filename || null;
}
/** JSDoc */
function _getEventFilterUrl(event: Event): string | null {
try {
if (event.stacktrace) {
return _getLastValidUrl(event.stacktrace.frames);
}

return null;
}

/** JSDoc */
private _getEventFilterUrl(event: Event): string | null {
let frames;
try {
if (event.stacktrace) {
return this._getLastValidUrl(event.stacktrace.frames);
}
let frames;
try {
// @ts-ignore we only care about frames if the whole thing here is defined
frames = event.exception.values[0].stacktrace.frames;
} catch (e) {
// ignore
}
return frames ? this._getLastValidUrl(frames) : null;
} catch (oO) {
isDebugBuild() && logger.error(`Cannot extract url for event ${getEventDescription(event)}`);
return null;
// @ts-ignore we only care about frames if the whole thing here is defined
frames = event.exception.values[0].stacktrace.frames;
} catch (e) {
// ignore
}
return frames ? _getLastValidUrl(frames) : null;
} catch (oO) {
isDebugBuild() && logger.error(`Cannot extract url for event ${getEventDescription(event)}`);
return null;
}
}
Loading