Skip to content

feat(core): Remove health check transaction filters #10818

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 2 commits into from
Feb 28, 2024
Merged

feat(core): Remove health check transaction filters #10818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbf6532
feat(core): Remove health check transaction filters
Lms24 Feb 27, 2024
82a0c85
migration
Lms24 Feb 27, 2024
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
6 changes: 6 additions & 0 deletions MIGRATION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Upgrading from 7.x to 8.x

## Removal of Client-Side health check transaction filters

The SDK no longer filters out health check transactions by default. Instead, they are sent to Sentry but still dropped
by the Sentry backend by default. You can disable dropping them in your Sentry project settings. If you still want to
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a note here that they are dropped by default for projects created after X? As I guess that is the case here...? And add a note like "If your project was created before X, you can enable the inbound filter for this at Y"?

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 think the server side inbound filter was turned on for all projects 🤔 I checked for projects in Sentry that were created way before we introduced the filter and there it's also turned on.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, I'm not too sure anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so even the projects in my private org have the filter turned on and I definitely didn't flip any switches there. I didn't find additional PRs that would have changed the defaults in older project but I asked in slack (#discuss-issues) to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked to the issues team; the health check transaction filter is on by default for all (old and new) projects. So we should be good to go @mydea 👍

drop specific transactions within the SDK you can either use the `ignoreTransactions` SDK option.

## Removal of the `MetricsAggregator` integration class and `metricsAggregatorIntegration`

The SDKs now support metrics features without any additional configuration.
Expand Down
17 changes: 1 addition & 16 deletions packages/core/src/integrations/inboundfilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,6 @@ import { convertIntegrationFnToClass, defineIntegration } from '../integration';
// 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 = [
/^.*\/healthcheck$/,
/^.*\/healthy$/,
/^.*\/live$/,
/^.*\/ready$/,
/^.*\/heartbeat$/,
/^.*\/health$/,
/^.*\/healthz$/,
];

/** Options for the InboundFilters integration */
export interface InboundFiltersOptions {
allowUrls: Array<string | RegExp>;
Expand All @@ -26,7 +16,6 @@ export interface InboundFiltersOptions {
ignoreTransactions: Array<string | RegExp>;
ignoreInternal: boolean;
disableErrorDefaults: boolean;
disableTransactionDefaults: boolean;
}

const INTEGRATION_NAME = 'InboundFilters';
Expand Down Expand Up @@ -77,11 +66,7 @@ function _mergeOptions(
...(clientOptions.ignoreErrors || []),
...(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
42 changes: 0 additions & 42 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 @@ -216,21 +216,6 @@ 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 @@ -408,39 +393,12 @@ describe('InboundFilters', () => {
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);
});

it.each(['/delivery', '/already', '/healthysnacks'])(
"doesn't filter out transactions that have similar names to health check ones (%s)",
transaction => {
const eventProcessor = createInboundFiltersEventProcessor();
const evt: Event = {
transaction,
type: 'transaction',
};
expect(eventProcessor(evt, {})).toBe(evt);
},
);
});

describe('denyUrls/allowUrls', () => {
Expand Down