Uh oh!
There was an error while loading. Please reload this page.
feat(core): Introduce processEvent hook on Integration - #9017
Conversation
size-limit report 📦
|
| * An optional hook that allows to process an event. | ||
| * Return `null` to drop the event, or mutate the event & return it. | ||
| */ | ||
| processEvent?(event: Event, hint: EventHint | undefined, client: Client): Event | null | PromiseLike<Event | null>; |
There was a problem hiding this comment.
giga nit: Can we add JS doc explaining that the client arg here is the client that recorded the event?
There was a problem hiding this comment.
ah, true, didn't actually update the comment here yet, will do!
c2d603d to
c354494Comparemydea
commented
Sep 14, 2023
I added tests for the new integration API, and actually also for |
| return result | ||
| .then(evt => { | ||
| // Process client-scoped event processors | ||
| return client && client.getEventProcessors ? notifyEventProcessors(client.getEventProcessors(), evt, hint) : evt; |
There was a problem hiding this comment.
I actually noticed a problem with the implementation in one of the other PRs (for browser). Which is that we can't just call this after prepareEvent is completed in the baseclient, because then the stuff that happens in here below will not be applied correctly.
So I updated it to instead run this here. And I am wondering, what do you think is nicer:
- Expose
getEventProcessors()on the client (like I did here) - Instead expose e.g.
applyToEvent()like we have on the scope and call that directly (which under the hood applies the event processors)?
cc @AbhiPrasad / @Lms24 / @lforst
There was a problem hiding this comment.
Let's do getEventProcessors on the client - I'd rather not show more internals than what is needed.
There was a problem hiding this comment.
sounds good. we maybe will adjust this for v8 anyhow if we adjust the event processing pipeline, but should be good for now I guess.
This adds a new (optional) `processEvent` hook on the `Integration` interface, which allows to register an event processor **for the current client only**. This has actually correct semantics in that the processor will only be registered for the client that the integration is added for. This is done by adding a new `addEventProcessor` method on the client, which for now are called after all global & scope event processors. Previously, all integrations always registered a _global_ event processor, which is not really necessary. With this, we can be much more focused & also skip checking for existence of the integration on the client etc.
The All the other event processor functions don't allow undefined here and pass an empty object so you can add attachments to events like this: hint.attachments=hint.attachments||[];hint.attachments.push({...}); |
mydea
commented
Oct 2, 2023
Hmm, good catch! We can update this in a backwards compatible way (just update the method signature), I think I just copied this from |
mydea
commented
Oct 2, 2023
See: #9151 |
This adds a new (optional)
processEventhook on theIntegrationinterface, which allows to register an event processor for the current client only.This has actually correct semantics in that the processor will only be registered for the client that the integration is added for. This is done by adding a new
addEventProcessormethod on the client, which for now are called after all global & scope event processors.Previously, all integrations always registered a global event processor, which is not really necessary. With this, we can be much more focused & also skip checking for existence of the integration on the client etc.
Supersedes #9015