feat(integration): Ensure `LinkedErrors` integration runs before all event processors #8956

mydea · 2023-09-06T09:55:39Z

While looking through our existing integrations, I noticed that the LinkedErrors integration in node had some weird/custom code to manually run the context lines integration after it processed, as we cannot guarantee any order etc.

I figured it would be much cleaner to solve this with a proper hook (I went with preprocessEvent), as it actually makes sense for this to generally run before all other event processors run, IMHO, and we can decouple these integrations from each other.

While this initially was quite straightforward to implement, I noticed a failing node test, which lead me down a rabbit hole where I realized that the integrations are more fundamentally flawed currently, because setupOnce will only be run the very first time an integration is setup, and will not run at all for consecutive clients. This made this completely break down 😬

My attempt to "fix" this was to add an optional setup hook to integrations that is always run, not just once.
Based on what we talked about yesterday in out v8 Integrations meeting, I think that makes sense generally also going forward. Eventually we can/should probably update all integrations to use setup + getCurrentHub().getScope().addEventProcessor() instead of a global event processor (another flaw I noticed with the current design - we always add processors for all clients). But for now, this only updates the one LinkedErrors integration.

Happy for pointers/ideas if we should have a different API for setup(). I figured it would make sense/be easy to pass the client in as an option, but am also happy to not do that and rely on doing getCurrentHub().getClient() inside of the setup hook, which should have the same effect as we can assume that at this point (when setup() is called) the current hub is for the client we just setup. 🤔

github-actions · 2023-09-06T10:06:21Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	75.37 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	31.31 KB (+0.1% 🔺)
@sentry/browser - Webpack (gzipped)	21.91 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	70.07 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	28.43 KB (+0.13% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	20.51 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	221.4 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	85.93 KB (+0.13% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	60.77 KB (+0.18% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	31.31 KB (+0.1% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	75.39 KB (+0.05% 🔺)
@sentry/react - Webpack (gzipped)	21.94 KB (+0.11% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	93.26 KB (+0.05% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	50.87 KB (+0.06% 🔺)

lforst · 2023-09-06T12:22:08Z

packages/types/src/client.ts

   */
-  emit?(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void;


Can we make this two separate hooks? If necessary by ignoring the lint rules. Upside: Separate JSDoc.

I split them up and ignored this eslint rule there!

Lms24

I generally like the intent of the change and I think we should do it.

Don't want to bike shed on this too long but since we're talking about new APIs, I'd like to propose a slightly different approach, based on our conversations around a hook-based integration API.

Two things first:

I'm assuming that what we change here will become the default in v8, to avoid having to change things twice. If this is not a concern, feel free to go with the current approach.
I believe we still need a setup to register the integration but setup shouldn't be used to contain event processing logic.

My proposal would be that - for this kind of logic, i.e. modifying an event as early as possible, we introduce an optional preprocessEvent: Event => Event | null (hmm do we actually need null here already 🤔) hook.

This makes the expected behaviour very clear. And it also makes writing custom integrations easier. Sure, setup could still be used to do the same thing with client hooks but it's more of a wild west behaviour IMO.

So basically:

export interface Integration {
  // no changes here for the moment
  name: string;
  setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void;

  preprocessEvent?: Event => Event | null;
  
  setup?: (client: Client): void;

  // We can still add for example this hook (or others) later on
  // processEvent?: Event => Event | null
}

In _prepareEvent we could call

integrations.foreach(integration => typeof integration.preprocess === "function" && integration.preprocessEvent(event))

which means we wouldn't need to add new client lifecycle hooks.

If others think this isn't necessary I'm happy to be overruled on this so feel free to disregard. IMO though, a clearly defined integrations API makes things easier in the long-run.

Lms24 · 2023-09-07T08:52:11Z

packages/core/src/integration.ts

  }
+
+  integration.setup && integration.setup(client);


l: should we check for typeof integration.setup === "function"? Maybe some (custom) integration currently exposes a setup property that doesn't match our interface...

good point, should def. do that! 👍

Lms24 · 2023-09-07T09:23:34Z

Damnit, thinking more about it, the hooks-based API leaves two open questions (at least for this case):

What goes into setup then?
We probably can't get rid of APIs like scope.addEventProcessor. So we'd have to live with event processing dualities around the (pre)processEvent hooks in integrations and event processors in the SDK or in user code.

mydea · 2023-09-07T09:35:49Z

I generally like the intent of the change and I think we should do it.

Don't want to bike shed on this too long but since we're talking about new APIs, I'd like to propose a slightly different approach, based on our conversations around a hook-based integration API.

....

I like the proposal! IMHO this makes sense, I would adjust it as follows, WDYT:

interface Integration {
  // ...
  preprocessEvent?(event: Event, hint?: EventHint): void
}

This way we can under the hood still simply call client.on('preprocessEvent'), which IMHO is cleaner than to call integrations etc. from the event processing pipeline?

Then in a separate step we can also add:

interface Integration {
  // ...
  processEvent?(event: Event, hint?: EventHint): Event | null
}

Which under the hood can call client.addEventProcessor? 🤔

setup can eventually remain as an escape hatch, but we don't need to figure that out in detail right now then :D

mydea · 2023-09-07T09:56:50Z

Opened a PR to this branch here with how this could work: #8969

Lms24

Thanks for applying my feedback! As I said, it doesn't have to happen but I think its the better DX.

…8969) A change to #8956 based on feedback. I figured it makes sense to also pass the client (as in the one concrete example we have we actually need it 😅 ) - not sure if this should be the first or last argument, but IMHO you probably don't need this always and then it is nicer to have (event, hint) as the API...?

mydea · 2023-09-11T08:59:55Z

Updated this with the change to have a preprocessEvent hook on the integration interface!

If we're good with this, I can do a follow up PR introducing a processEvent hook in a similar way. IMHO with this we are already 70% of the way to a new improved integrations API! (missing are "only" the monkeypatching things then, TBD).

…8969) A change to #8956 based on feedback. I figured it makes sense to also pass the client (as in the one concrete example we have we actually need it 😅 ) - not sure if this should be the first or last argument, but IMHO you probably don't need this always and then it is nicer to have (event, hint) as the API...?

mydea requested review from lforst, Lms24 and AbhiPrasad September 6, 2023 09:55

mydea self-assigned this Sep 6, 2023

lforst approved these changes Sep 6, 2023

View reviewed changes

mydea force-pushed the fn/integration-order branch from a28ae01 to 72275c8 Compare September 6, 2023 13:43

Lms24 reviewed Sep 7, 2023

View reviewed changes

mydea mentioned this pull request Sep 7, 2023

ref: Replace setup with preprocessEvent on Integration interface #8969

Merged

Lms24 approved these changes Sep 7, 2023

View reviewed changes

AbhiPrasad approved these changes Sep 8, 2023

View reviewed changes

mydea force-pushed the fn/integration-order branch from d11db32 to ff7f3d1 Compare September 11, 2023 08:58

mydea added 4 commits September 11, 2023 16:52

fix(core): ensure LinkedErrors integration runs before eventProcessors

6435437

Add integration.setup hook that always runs

bd90dc9

split hook signatures

976143b

mydea force-pushed the fn/integration-order branch from ff7f3d1 to 40cd69e Compare September 11, 2023 14:52

mydea merged commit ce84fb3 into develop Sep 11, 2023
78 checks passed

mydea deleted the fn/integration-order branch September 11, 2023 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(integration): Ensure `LinkedErrors` integration runs before all event processors #8956

feat(integration): Ensure `LinkedErrors` integration runs before all event processors #8956

mydea commented Sep 6, 2023

github-actions bot commented Sep 6, 2023 •

edited

lforst Sep 6, 2023

mydea Sep 6, 2023

Lms24 left a comment •

edited

Lms24 Sep 7, 2023

mydea Sep 7, 2023

Lms24 commented Sep 7, 2023 •

edited

mydea commented Sep 7, 2023

mydea commented Sep 7, 2023

Lms24 left a comment

mydea commented Sep 11, 2023

		*/
		emit?(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void;

feat(integration): Ensure LinkedErrors integration runs before all event processors #8956

feat(integration): Ensure LinkedErrors integration runs before all event processors #8956

Conversation

mydea commented Sep 6, 2023

github-actions bot commented Sep 6, 2023 • edited

size-limit report 📦

lforst Sep 6, 2023

Choose a reason for hiding this comment

mydea Sep 6, 2023

Choose a reason for hiding this comment

Lms24 left a comment • edited

Choose a reason for hiding this comment

Lms24 Sep 7, 2023

Choose a reason for hiding this comment

mydea Sep 7, 2023

Choose a reason for hiding this comment

Lms24 commented Sep 7, 2023 • edited

mydea commented Sep 7, 2023

mydea commented Sep 7, 2023

Lms24 left a comment

Choose a reason for hiding this comment

mydea commented Sep 11, 2023

feat(integration): Ensure `LinkedErrors` integration runs before all event processors #8956

feat(integration): Ensure `LinkedErrors` integration runs before all event processors #8956

github-actions bot commented Sep 6, 2023 •

edited

Lms24 left a comment •

edited

Lms24 commented Sep 7, 2023 •

edited