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.

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

Jump to bottom

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

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 4 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 17 additions & 2 deletions packages/core/src/integrations/inboundfilters.ts
View file
Open in desktop
Expand Up @@ -5,13 +5,24 @@ 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

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;
disableDefaults: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

m: Can we split this into disableErrorDefaults and disableTransactionDefaults? I can already see folks wanting to disable the one but not the other.

Copy link
Member

Choose a reason for hiding this comment

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

further m:
Are we okay with this being sort of hidden away for users?

To enable Health checks, they'd need to go from

Sentry.init({
  dsn: '...'
})

to

Sentry.init({
  dsn: '...',
  integrations: [new InboundFilters({disableTransactionDefaults: true})]
})

Copy link
Member

@Lms24 Lms24 May 25, 2023 •
edited

Choose a reason for hiding this comment

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

I guess the alternative would be something like this:

Sentry.init({
  dsn: '...',
  ignoreHealthCheckTransactions: false
})

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this again, we can go with the integration options for now and add the top level option if peole want it, I think that's okay for now.

}

/** Inbound filters configurable by the user */
Expand Down Expand Up @@ -62,9 +73,13 @@ export function _mergeOptions(
ignoreErrors: [
...(internalOptions.ignoreErrors || []),
...(clientOptions.ignoreErrors || []),
...DEFAULT_IGNORE_ERRORS,
...(internalOptions.disableDefaults === true ? [] : DEFAULT_IGNORE_ERRORS),
HazAT marked this conversation as resolved.
Show resolved Hide resolved
],
ignoreTransactions: [
...(internalOptions.ignoreTransactions || []),
...(clientOptions.ignoreTransactions || []),
...(internalOptions.disableDefaults === true ? [] : DEFAULT_IGNORE_TRANSACTIONS),
],
ignoreTransactions: [...(internalOptions.ignoreTransactions || []), ...(clientOptions.ignoreTransactions || [])],
ignoreInternal: internalOptions.ignoreInternal !== undefined ? internalOptions.ignoreInternal : true,
};
}
Expand Down
26 changes: 26 additions & 0 deletions packages/core/test/lib/integrations/inboundfilters.test.ts
View file
Open in desktop
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 @@ -373,6 +388,17 @@ describe('InboundFilters', () => {
it('uses default filters', () => {
const eventProcessor = createInboundFiltersEventProcessor();
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 filters', () => {
const eventProcessor = createInboundFiltersEventProcessor({ disableDefaults: true });
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
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);
});