Skip to content
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.

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

[v8] Replace addGlobalEventProcessor with addEventProcessor #9082

Closed
mydea opened this issue Sep 21, 2023 · 0 comments · Fixed by #9554
Closed

[v8] Replace addGlobalEventProcessor with addEventProcessor #9082

mydea opened this issue Sep 21, 2023 · 0 comments · Fixed by #9554
Milestone

Comments

@mydea
Copy link
Member

mydea commented Sep 21, 2023

I wonder if we should completely drop global event processors in v8.

Currently, when you do this:

import * as Sentry from '@sentry/browser';

Sentry.addGlobalEventProcessor(eventProcessor);

This will be added to a global and will apply to all clients.

Now we also have event processors on the client, which makes more sense for isolation of integrations etc.
However, this has the "problem" that if all our own code uses client integrations (to ensure isolation), but users use global event processors, they will potentially not run in a reasonable order.

To "fix" this, currently we run event processors in the order Client > Global > Scope. This is done to ensure current usage of global event processors works as expected (=they get the already processed event from the client processors). However, looking at it abstractly, this doesn't really make sense - why would client processors run before global ones? Normally processors should probably run in order of specificity, so Global > Client > Scope.

While thinking this through, I wondered - do we actually need global event processors? Is it not enough to have processors on the client + scope? Thus I would propose to:

  1. Add a new top level export addEventProcessor that puts the given processor on the current client.
  2. Deprecate addGlobalEventProcessor in favor of using addEventProcessor
  3. Remove addGlobalEventProcessor in v8

This will allow us to streamline this model a bit, and allow to remove the code where we have to keep the event processors on a global variable. I don't know if there is a reasonable scenario where you want an event processor that literally runs on all clients - IMHO the whole point of having multiple clients is that they should be isolated. And you can still just add the event processor to all clients like clients.forEach((client) => client.addEventProcessor(eventProcessor)) if you really want this.

@mydea mydea added this to the 8.0.0 milestone Sep 21, 2023
mydea added a commit that referenced this issue Dec 4, 2023
And deprecate `addGlobalEventProcessor()` and
`getGlobalEventProcessors()`.

In v8, all event processors will be on the client only, streamlining
this a bit and preventing global "pollution".

Closes #9082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant