feat(integration): Ensure LinkedErrors integration runs before all event processors
#8956
Conversation
size-limit report 📦
|
| */ | ||
| emit?(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void; |
There was a problem hiding this comment.
Can we make this two separate hooks? If necessary by ignoring the lint rules. Upside: Separate JSDoc.
There was a problem hiding this comment.
I split them up and ignored this eslint rule there!
a28ae01 to
72275c8
Compare
There was a problem hiding this comment.
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
setupto 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.
packages/core/src/integration.ts
Outdated
| } | ||
|
|
||
| integration.setup && integration.setup(client); |
There was a problem hiding this comment.
l: should we check for typeof integration.setup === "function"? Maybe some (custom) integration currently exposes a setup property that doesn't match our interface...
There was a problem hiding this comment.
good point, should def. do that! 👍
|
Damnit, thinking more about it, the hooks-based API leaves two open questions (at least for this case):
|
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 Then in a separate step we can also add: interface Integration {
// ...
processEvent?(event: Event, hint?: EventHint): Event | null
}Which under the hood can call
|
|
Opened a PR to this branch here with how this could work: #8969 |
…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...?
…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...?
d11db32 to
ff7f3d1
Compare
|
Updated this with the change to have a If we're good with this, I can do a follow up PR introducing a |
…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...?
ff7f3d1 to
40cd69e
Compare
While looking through our existing integrations, I noticed that the
LinkedErrorsintegration 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
setupOncewill 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
setuphook 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 oneLinkedErrorsintegration.Happy for pointers/ideas if we should have a different API for
setup(). I figured it would make sense/be easy to pass theclientin as an option, but am also happy to not do that and rely on doinggetCurrentHub().getClient()inside of thesetuphook, which should have the same effect as we can assume that at this point (whensetup()is called) the current hub is for the client we just setup. 🤔