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

feat(core): Add multiplexed transport #7926

Merged
merged 10 commits into from
Apr 25, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Apr 20, 2023

Closes #5185

Adds makeMultiplexedTransport which can be used to create a Transport that sends envelopes to different, even multiple DSNs depending on the content of the envelope.

It takes two parameters:

  • A transport creation function that is used to create transports when required
  • A match callback with the following type:
interface MatchParam {
  // The envelope to be sent
  envelope: Envelope;
  // A function that returns an event from the envelope if one exists
  getEvent(): Event | undefined;
}

type Matcher = (param: MatchParam) => string[];

Currently, the DSN passed to init is used as a fallback if the match callback returns zero DSNs. I'm not 100% happy with this as in the example below, the DSN passed to init will never be used but is required for the client to send events.

Send everything to two DSNs

import { makeMultiplexedTransport } from '@sentry/core';
import { init, makeFetchTransport } from '@sentry/browser';

const DSN1 = '__DSN1__';
const DSN2 = '__DSN2__';

init({
  dsn: DSN1, // <- init dsn never gets used but need to be valid
  transport: makeMultiplexedTransport(makeFetchTransport, () => [DSN1, DSN2])
});

Override DSN from capture point

import { makeMultiplexedTransport } from '@sentry/core';
import { init, captureException, makeFetchTransport } from '@sentry/browser';

function dsnFromTag({ getEvent }) {
  const event = getEvent();
  return event?.tags?.DSN_OVERRIDE ? [event.tags.DSN_OVERRIDE] : [];
}

init({
  dsn: '__FALLBACK_DSN__',
  transport: makeMultiplexedTransport(makeFetchTransport, dsnFromTag)
});

captureException(new Error('oh no!', scope => {
  scope.addTag('DSN_OVERRIDE', '__ANOTHER_DSN__');
  return scope;
});

Different DSNs for different features

import { makeMultiplexedTransport } from '@sentry/core';
import { init, captureException, makeFetchTransport } from '@sentry/browser';

function dsnFromFeature({ getEvent }) {
  const event = getEvent();
  switch(event?.tags?.feature) {
    case 'cart':
      return ['__CART_DSN__'];
    case 'gallery':
      return ['__GALLERY_DSN__'];
  }
  return []
}

init({
  dsn: '__FALLBACK_DSN__',
  transport: makeMultiplexedTransport(makeFetchTransport, dsnFromFeature)
});

captureException(new Error('oh no!', scope => {
  scope.addTag('feature', 'cart');
  return scope;
});

@timfish timfish marked this pull request as ready for review April 24, 2023 15:46
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach makes a lot of sense to me

packages/core/src/transports/multiplexed.ts Show resolved Hide resolved
@AbhiPrasad
Copy link
Member

Tim another thing we need to make sure works is the use case in #7622 (comment), where matches can be added on demand.

Is this easiest way to do this to add to just make them enforce a global they mutate? Or should we think about exposing a separate API?

@mydea
Copy link
Member

mydea commented Apr 25, 2023

Tim another thing we need to make sure works is the use case in #7622 (comment), where matches can be added on demand.

Is this easiest way to do this to add to just make them enforce a global they mutate? Or should we think about exposing a separate API?

IMHO it should be good enough to do this via some closure state that can be mutated, like:

const myDsns = [
  { dsn: 'https://...', tag: 'my-tag' },
  { dsn: 'https://...', tag: 'other-tag' },
];

function dsnFromTag({ getEvent }) {
  const event = getEvent();
  const opt = myDsns.find(({tag} => event?.tags?.includes(tag));
  return opt ? [opt.dsn] : [];
}

init({
  dsn: '__FALLBACK_DSN__',
  transport: makeMultiplexedTransport(makeFetchTransport, dsnFromTag)
});

// later, just modify myDsns

?

@AbhiPrasad
Copy link
Member

IMHO it should be good enough to do this via some closure state that can be mutated, like:

Yeah I think thats fair, we just have to make sure we have good docs then!

@timfish
Copy link
Collaborator Author

timfish commented Apr 25, 2023

Is this easiest way to do this to add to just make them enforce a global they mutate?

Yes I think so. When I started coding this it was much more complex with an array of matcher functions but then I started thinking about match order and decided to simplify the API and leave more complex/niche scenarios up to users.

@amccloud
Copy link

amccloud commented May 7, 2023

@mydea question about this implementation vs. the flex-micro approach in https://github.com/sentry-demos/sentry-micro-frontend.

With this multiplex approach, can each component bring its own Sentry.init and/or call init independently? If other micro-frontends call init after, will events proxy and multiplex through the first Sentry client to init?

@AbhiPrasad, my comment in #7622 (comment) was for supporting micro-frontends in a federated environment. This seems to be one of the more popular, if not "the" requirement for micro-frontends #5217.

The tag delegation/multiplex approach does not look like it will work with those looking for federated micro-frontends. Each of my micro-frontends is developed by their teams with their own call to Sentry.init. The host(s)/document should mostly be unaware of Sentry for micro-frontends displayed.

The only thing I can think of is creating an internal @org/sentry-multiplexer-singleton package configured with this multiplexer while making sure all our micro-frontend teams use that instead of @sentry/browser. That should work for us until one of our micro-frontend hosts adopts Sentry. We won't have any influence on how they Sentry.init, and because of that I don't think we'll be able to configure this multiplexing approach for our micro-frontends.

@AbhiPrasad
Copy link
Member

@amccloud There should only be a single Sentry.init call. Using a single Sentry.init call improves performance overhead (no need to instrument multiple times), removes the burden of setting up options/dsn (a team can't override a sample rate and cause quota issues) and makes it easier to setup up source maps.

The only thing I can think of is creating an internal @org/sentry-multiplexer-singleton package configured with this multiplexer while making sure all our micro-frontend teams use that instead of @sentry/browser

This seems like a good strategy to me. The way this would work then is that the host app would consume a function that calls Sentry.init, and the federated micro-frontends would consume a function that updates multiplexing strategy as appropriate.

We are putting the onus on the implementors to define how global state should be stored, as it gets complicated if the SDK does this. I can add this as a note to the dcs.

Another things to think about is to use the stacktrace to determine what module you are in. See @timfish's example in https://github.com/timfish/sentry-module-federation

That should work for us until one of our micro-frontend hosts adopts Sentry

Unfortunately the way the browser works is that we can't isolate breadcrumbs/scope to a specific part of the web app. So if that micro-frontend throws an error, it bubbles up to the global error handler. In addition, we can't differentiate between breadcrumbs created (for ex. by a fetch request) by that micro-frontend vs. another one.

@amccloud
Copy link

amccloud commented May 8, 2023

@AbhiPrasad thanks for the reply.

Unfortunately the way the browser works is that we can't isolate breadcrumbs/scope to a specific part of the web app. So if that micro-frontend throws an error, it bubbles up to the global error handler. In addition, we can't differentiate between breadcrumbs created (for ex. by a fetch request) by that micro-frontend vs. another one.

I have a fork of flex-micro with most plugins working in isolation across 2 components and a host. All of which bring their own Sentry module through module federation when necessary. I think breadcrumb isolation could be accomplished with an approach like zone.js. This was next and last on my list to explore.

To your point, this can all be done in another lib and doesn't need to be supported by this lib.

Instead of electing a leader like with flex-micro, I see the more explicit route forward here where if a host adopts Sentry we can pass their instance of Sentry to micro-frontends to modify and register their multiplex preference. Maybe just as a global or through an interface of their choice. Ideally, hosts and micro-frontends consume the same hypothetical @org/sentry-multiplexer-singleton package so this should be easy to align on.

What is not clear is who owns plugins, performance data, and event quotas? I wouldn't want to multiplex navigate/pageload events unless the duplication is deduped/free or isolated for each micro-frontend.

@AbhiPrasad
Copy link
Member

I think breadcrumb isolation could be accomplished with an approach like zone.js

We are going to explore this ourselves, but to most users it's not feasible due to the bundle size impact and runtime performance costs. Angular themselves are switching away from Zone and change detection to use signals.

What is not clear is who owns plugins, performance data, and event quotas? I wouldn't want to multiplex navigate/pageload events unless the duplication is deduped/free or isolated for each micro-frontend.

Yeah we're struggling with this as well. Another issue here is that re: breadcrumb isolation, many folks actually told us they don't want it to be isolated, and seeing dom event/http request breadcrumbs from other micro frontends help with debugging.

@tanem
Copy link

tanem commented May 11, 2023

Thanks for this change 🙂 Had a q around this message from @AbhiPrasad earlier in the thread, happy to post this elsewhere if need be:

Using a single Sentry.init call ... makes it easier to setup up source maps.

Our MFEs each have their own Sentry project and upload their own sourcemaps to those projects. We set a release prop via the custom BrowserClient each MFE creates.

If we use this multiplex approach we can drop the custom client/hub for each MFE, but does that mean we'll have to explicitly set release on each event object, since there'll only be one init in the host app?

@timfish
Copy link
Collaborator Author

timfish commented May 11, 2023

does that mean we'll have to explicitly set release on each event object

Yes, the release will need to be overridden, but not necessarily manually at the capture point, it could occur for example in an integration.

There's no official code to support this yet but as Abhi mentions above, I've been looking into routing to specific dsn/release via inspecting the filenames on the stack trace.

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

Successfully merging this pull request may close these issues.

Introduce Multiplexing Transport
5 participants