feat(core): Add `beforeSendTransaction` #6121

lobsterkatie · 2022-11-02T14:58:05Z

This adds a new beforeSendTransaction option to the SDK. As one would expect from the name, it works just like beforeSend (in other words, it's a guaranteed-to-be-last event processor), except that it acts upon transaction events, which the original beforeSend doesn't.

Docs PR to come.

Ref: getsentry/rfcs#19 (comment)

github-actions · 2022-11-02T15:07:52Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.5 KB (+0.06% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.37 KB (+0.04% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.15 KB (+0.19% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	53.71 KB (+0.07% 🔺)
@sentry/browser - Webpack (gzipped + minified)	19.89 KB (+0.17% 🔺)
@sentry/browser - Webpack (minified)	65.12 KB (-0.01% 🔽)
@sentry/react - Webpack (gzipped + minified)	19.92 KB (+0.19% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	45.81 KB (+0.06% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.25 KB (+0.05% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.65 KB (+0.12% 🔺)

packages/core/src/baseclient.ts

mydea · 2022-11-03T09:36:42Z

packages/core/test/lib/base.test.ts

@@ -887,7 +887,7 @@ describe('BaseClient', () => {
      expect(capturedEvent).toEqual(normalizedTransaction);
    });

-    test('calls beforeSend and uses original event without any changes', () => {
+    test('calls `beforeSend` and uses original event without any changes', () => {


l: While re-writing these tests, an idea would also be to maybe nest them into describe('beforeSend') and describe('beforeSendTransaction')?

I def think this test file could be streamlined/better organized more than I have here (some describe.each() or it.each() calls are probably in order), but for the sake of keeping it still relatively clear what changes are actually for beforeSendTransaction and which are for re-org purposes, IMHO that should happen in a separate PR. (I may actually pull the re-org changes here out into a separate PR as it is. UPDATE: I did: #6135.)

makes sense to me! 👍

This is a collection of a bunch of small and mostly dumb fixes pulled from #6121 (along with a few others I happened upon in the course of working on it), in order to make that PR easier to review. Some of the changes are cosmetic (formatting and wordsmithing test names, adding comments, etc.), some are refactors for clarity (mostly renaming of variables), one or two are test changes which will be necessary for the tests introduced in the aforementioned PR (in order to keep the `beforeSend` and `beforeSendTransaction` tests as parallel as possible, in case we ever want to parameterize them), one is pulling unneeded extra data out of a test to make it simpler, and two are actual fixes, namely: - In the outcomes test, we were recording a session being dropped by `beforeSend`, but sessions don't go through `beforeSend`, so it's now an error event. - In many places in the base client tests, we were using `.toBe()` rather than `toEqual()` for values (like numbers and strings) which as far as I know aren't guaranteed to be singletons (the way `true` and `false` are, for example). This switches to using `toEqual()` in those cases.

Lms24

LGTM, just some remarks for the tests and JSDoc

(Hope it's okay that I reviewed the PR while it was in Draft. I figured from the description you let it in Draft while you were working on docs. If you still wanted to make changes feel free to dismiss this review)

Lms24 · 2022-11-04T08:43:22Z

packages/core/test/lib/base.test.ts

+
+      client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' });
+
+      expect(TestClient.instance!.event!.transaction).toBe('/dogs/are/great');


m: I think we should add a expect(beforeSendTransaction).toHaveBeenCalled() assertion here to make sure that the function was actually called. The check we have in place here doesn't ensure this fully.

l to super-l: same in the other tests below but admittedly it's less of an issue there, so feel free to ignore there

Valid point, but if I do it for beforeSendTransaction, I should do it for beforeSend as well. Not opposed to doing so, but I'll do it in a separate PR just to keep things simple.

UPDATE: Done in #6150.

packages/core/test/lib/base.test.ts

packages/types/src/options.ts

OrkhanAlikhanov · 2022-11-08T13:08:08Z

Hello, should there be an entry in the docs for this feature as well? https://develop.sentry.dev/sdk/features/#before-send-hook

Lms24 · 2022-11-08T14:11:45Z

Hi @OrkhanAlikhanov, yes, we're working on that. For now, you should be able to follow the beforeSend instructions on in the docs as both functions do the same thing, just for different event types (errors and transactions)

@Lms24

…`beforeSendTransaction` tests (#6150) Per @Lms24's suggestion[1], this adds a check to make sure the relevant function is being called in our `beforeSend` and `beforeSendTransaction` tests. [1] #6121 (comment)

ehtb · 2022-11-22T11:01:47Z

Hi, can you please be more careful with introducing breaking changes like these:
const beforeSendProcessorName = isTransaction ? 'beforeSendTransaction' : 'beforeSend';

I could not find this change in the documentation, or the changelog. An API change would also warrant a major release, or at least an obsolete message somewhere.

Thanks

AbhiPrasad · 2022-11-22T11:13:47Z

Hey @ehtb, could you clarify how this was a breaking change? We did not change any public APIs/behavior for existing event processors - only added new functionality here.

ehtb · 2022-11-22T11:35:35Z

Hi @AbhiPrasad AFAICT previously the transaction events were send through beforeSend, but now they only land in beforeSendTransaction.

lobsterkatie · 2022-11-22T14:47:50Z

@ehtb - Transaction events have never gone through beforeSend. Notice here that in the previous version, we bailed before calling beforeSend if isTransaction was true.

ehtb · 2022-11-22T16:03:36Z

Thanks for checking @lobsterkatie.
I integrated with the sentry-opentelemetry package btw, that's where I think I saw them appearing under beforeSend, but maybe I am mistaken…

AbhiPrasad · 2022-11-22T16:27:07Z

Hey @ehtb, awesome to hear you just integrated with the OpenTelemetry package. I helped build it, do you have some time to sit down and walk through your experiences with the package and your OpenTelemetry set up? We can also try debugging your transaction/beforeSend situation.

vladanpaunovic assigned lobsterkatie Nov 2, 2022

lobsterkatie force-pushed the kmclb-add-beforeSendTransaction branch 2 times, most recently from 92d565a to ad5a70e Compare November 3, 2022 05:22

lobsterkatie marked this pull request as ready for review November 3, 2022 06:24

lobsterkatie requested review from a team, mydea and AbhiPrasad and removed request for a team November 3, 2022 06:25

mydea reviewed Nov 3, 2022

View reviewed changes

packages/core/src/baseclient.ts Outdated Show resolved Hide resolved

mydea reviewed Nov 3, 2022

View reviewed changes

mydea approved these changes Nov 3, 2022

View reviewed changes

lobsterkatie mentioned this pull request Nov 3, 2022

ref(core): Pre-beforeSendTransaction cleanup #6135

Merged

lobsterkatie changed the base branch from master to kmclb-pre-beforeSendTransaction-cleanup November 3, 2022 23:57

lobsterkatie force-pushed the kmclb-add-beforeSendTransaction branch from ad5a70e to c3af67c Compare November 3, 2022 23:58

lobsterkatie marked this pull request as draft November 3, 2022 23:59

lobsterkatie force-pushed the kmclb-add-beforeSendTransaction branch from c3af67c to 06c4664 Compare November 4, 2022 00:18

Base automatically changed from kmclb-pre-beforeSendTransaction-cleanup to master November 4, 2022 08:30

lobsterkatie force-pushed the kmclb-add-beforeSendTransaction branch from 06c4664 to 5820734 Compare November 4, 2022 08:36

Lms24 approved these changes Nov 4, 2022

View reviewed changes

lobsterkatie added 3 commits November 7, 2022 19:05

add beforeSendTransaction option

b736d2c

add beforeSendTransaction to comment in getIntegrationsToSetup

37e1598

add tests

84541f7

lobsterkatie force-pushed the kmclb-add-beforeSendTransaction branch from 5820734 to 84541f7 Compare November 8, 2022 03:06

Lms24 approved these changes Nov 8, 2022

View reviewed changes

lobsterkatie marked this pull request as ready for review November 8, 2022 07:12

lobsterkatie merged commit f214732 into master Nov 8, 2022

lobsterkatie deleted the kmclb-add-beforeSendTransaction branch November 8, 2022 07:12

lobsterkatie mentioned this pull request Nov 8, 2022

fix(core): Check that functions have been called in beforeSend and beforeSendTransaction tests #6150

Merged

cleptric mentioned this pull request Nov 9, 2022

Add beforeSendTransaction getsentry/sentry-php#1423

Closed

smeubank mentioned this pull request Nov 30, 2022

withSentry in nextjs can not be configured to scrub cookies (sensitive data) #4723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add `beforeSendTransaction` #6121

feat(core): Add `beforeSendTransaction` #6121

lobsterkatie commented Nov 2, 2022 •

edited

Loading

github-actions bot commented Nov 2, 2022 •

edited

Loading

mydea Nov 3, 2022

lobsterkatie Nov 3, 2022 •

edited

Loading

mydea Nov 4, 2022

Lms24 left a comment •

edited

Loading

Lms24 Nov 4, 2022

lobsterkatie Nov 8, 2022 •

edited

Loading

OrkhanAlikhanov commented Nov 8, 2022

Lms24 commented Nov 8, 2022 •

edited

Loading

ehtb commented Nov 22, 2022

AbhiPrasad commented Nov 22, 2022

ehtb commented Nov 22, 2022 •

edited

Loading

lobsterkatie commented Nov 22, 2022

ehtb commented Nov 22, 2022 •

edited

Loading

AbhiPrasad commented Nov 22, 2022


		client.captureEvent({ transaction: '/dogs/are/great', type: 'transaction' });

		expect(TestClient.instance!.event!.transaction).toBe('/dogs/are/great');

feat(core): Add beforeSendTransaction #6121

feat(core): Add beforeSendTransaction #6121

Conversation

lobsterkatie commented Nov 2, 2022 • edited Loading

github-actions bot commented Nov 2, 2022 • edited Loading

size-limit report 📦

mydea Nov 3, 2022

Choose a reason for hiding this comment

lobsterkatie Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

mydea Nov 4, 2022

Choose a reason for hiding this comment

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

Lms24 Nov 4, 2022

Choose a reason for hiding this comment

lobsterkatie Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

OrkhanAlikhanov commented Nov 8, 2022

Lms24 commented Nov 8, 2022 • edited Loading

ehtb commented Nov 22, 2022

AbhiPrasad commented Nov 22, 2022

ehtb commented Nov 22, 2022 • edited Loading

lobsterkatie commented Nov 22, 2022

ehtb commented Nov 22, 2022 • edited Loading

AbhiPrasad commented Nov 22, 2022

feat(core): Add `beforeSendTransaction` #6121

feat(core): Add `beforeSendTransaction` #6121

lobsterkatie commented Nov 2, 2022 •

edited

Loading

github-actions bot commented Nov 2, 2022 •

edited

Loading

lobsterkatie Nov 3, 2022 •

edited

Loading

Lms24 left a comment •

edited

Loading

lobsterkatie Nov 8, 2022 •

edited

Loading

Lms24 commented Nov 8, 2022 •

edited

Loading

ehtb commented Nov 22, 2022 •

edited

Loading

ehtb commented Nov 22, 2022 •

edited

Loading