Skip to content

feat(core): Add default ignoreTransactions for Healthchecks #8191

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 7 commits into from
Jun 1, 2023
Merged

feat(core): Add default ignoreTransactions for Healthchecks #8191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed248df
feat: Add default ignoreTransactions
HazAT May 23, 2023
5628289
ref: Add disableDefaults option to inbound integration
HazAT May 25, 2023
47f8849
ref: Remove if
HazAT May 25, 2023
898e514
fix: Linter
HazAT May 25, 2023
c063be3
Update packages/core/src/integrations/inboundfilters.ts
HazAT May 25, 2023
7040d86
ref: Split vars
HazAT May 25, 2023
979f3ce
Apply suggestions from code review
HazAT May 31, 2023
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
20 changes: 18 additions & 2 deletions packages/core/src/integrations/inboundfilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,25 @@ import { getEventDescription, logger, stringMatchesSomePattern } from '@sentry/u
// 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$/];

const DEFAULT_IGNORE_TRANSACTIONS = [
Copy link
Member

@k-fish k-fish May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

fwiw, I've also seen ping

Copy link
Member Author

Choose a reason for hiding this comment

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

we decided to keep consistency with https://docs.sentry.io/product/performance/performance-at-scale/#health-checks

but we can also change it there

Copy link

Choose a reason for hiding this comment

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

had a customer email me today that ping was 13 TPM while the rest were all .01 TPM and below. said it ate up their quota.

Copy link

Choose a reason for hiding this comment

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

it was python

/^.*healthcheck.*$/,
/^.*healthy.*$/,
/^.*live.*$/,
/^.*ready.*$/,
/^.*heartbeat.*$/,
/^.*\/health$/,
/^.*\/healthz$/,
];

/** Options for the InboundFilters integration */
export interface InboundFiltersOptions {
allowUrls: Array<string | RegExp>;
denyUrls: Array<string | RegExp>;
ignoreErrors: Array<string | RegExp>;
ignoreTransactions: Array<string | RegExp>;
ignoreInternal: boolean;
disableErrorDefaults: boolean;
disableTransactionDefaults: boolean;
}

/** Inbound filters configurable by the user */
Expand Down Expand Up @@ -62,9 +74,13 @@ export function _mergeOptions(
ignoreErrors: [
...(internalOptions.ignoreErrors || []),
...(clientOptions.ignoreErrors || []),
...DEFAULT_IGNORE_ERRORS,
...(internalOptions.disableErrorDefaults ? [] : DEFAULT_IGNORE_ERRORS),
],
ignoreTransactions: [
...(internalOptions.ignoreTransactions || []),
...(clientOptions.ignoreTransactions || []),
...(internalOptions.disableTransactionDefaults ? [] : DEFAULT_IGNORE_TRANSACTIONS),
],
ignoreTransactions: [...(internalOptions.ignoreTransactions || []), ...(clientOptions.ignoreTransactions || [])],
ignoreInternal: internalOptions.ignoreInternal !== undefined ? internalOptions.ignoreInternal : true,
};
}
Expand Down
36 changes: 36 additions & 0 deletions packages/core/test/lib/integrations/inboundfilters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,21 @@ const TRANSACTION_EVENT_3: Event = {
type: 'transaction',
};

const TRANSACTION_EVENT_HEALTH: Event = {
transaction: 'GET /health',
type: 'transaction',
};

const TRANSACTION_EVENT_HEALTH_2: Event = {
transaction: 'GET /healthy',
type: 'transaction',
};

const TRANSACTION_EVENT_HEALTH_3: Event = {
transaction: 'GET /live',
type: 'transaction',
};

describe('InboundFilters', () => {
describe('_isSentryError', () => {
it('should work as expected', () => {
Expand Down Expand Up @@ -372,7 +387,28 @@ describe('InboundFilters', () => {

it('uses default filters', () => {
const eventProcessor = createInboundFiltersEventProcessor();
expect(eventProcessor(SCRIPT_ERROR_EVENT, {})).toBe(null);
expect(eventProcessor(TRANSACTION_EVENT, {})).toBe(TRANSACTION_EVENT);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH, {})).toBe(null);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH_2, {})).toBe(null);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH_3, {})).toBe(null);
});

it('disable default error filters', () => {
const eventProcessor = createInboundFiltersEventProcessor({ disableErrorDefaults: true });
expect(eventProcessor(SCRIPT_ERROR_EVENT, {})).toBe(SCRIPT_ERROR_EVENT);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH, {})).toBe(null);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH_2, {})).toBe(null);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH_3, {})).toBe(null);
});

it('disable default transaction filters', () => {
const eventProcessor = createInboundFiltersEventProcessor({ disableTransactionDefaults: true });
expect(eventProcessor(SCRIPT_ERROR_EVENT, {})).toBe(null);
expect(eventProcessor(TRANSACTION_EVENT, {})).toBe(TRANSACTION_EVENT);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH, {})).toBe(TRANSACTION_EVENT_HEALTH);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH_2, {})).toBe(TRANSACTION_EVENT_HEALTH_2);
expect(eventProcessor(TRANSACTION_EVENT_HEALTH_3, {})).toBe(TRANSACTION_EVENT_HEALTH_3);
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/nextjs/test/integration/test/client/tracingPageLoad.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Transaction } from '@sentry/types';

test('should report a `pageload` transaction', async ({ page }) => {
const transaction = await getMultipleSentryEnvelopeRequests<Transaction>(page, 1, {
url: '/healthy',
url: '/testy',
envelopeType: 'transaction',
});

Expand All @@ -16,5 +16,5 @@ test('should report a `pageload` transaction', async ({ page }) => {
},
});

expect(await countEnvelopes(page, { url: '/healthy', envelopeType: 'transaction', timeout: 4000 })).toBe(1);
expect(await countEnvelopes(page, { url: '/testy', envelopeType: 'transaction', timeout: 4000 })).toBe(1);
});