Improve user facing integrations configuration

[timfish]

Problem Statement

I've briefly chatted to @AbhiPrasad about this but we wanted to open the discussion up and get some feedback and ideas from others...

The current integration options are as follows:

  /**
   * If this is set to false, default integrations will not be added, otherwise this will internally be set to the
   * recommended default integrations.
   * TODO: We should consider changing this to `boolean | Integration[]`
   */
  defaultIntegrations?: false | Integration[];

  /**
   * List of integrations that should be installed after SDK was initialized.
   * Accepts either a list of integrations or a function that receives
   * default integrations and returns a new, updated list.
   */
  integrations?: Integration[] | ((integrations: Integration[]) => Integration[]);

There are a few issues I see with this:

• defaultIntegrations: false is not tree shakeable.
• It looks like defaultIntegrations allowing an array is an internal implementation detail that has leaked into the public api
  • it's not clear why users would ever supply an array here
• It appears that the integrations function option solely exists to allow users to remove default integrations easily
  • Is there a better/cleaner way to offer this functionality?

Ultimately, we're stuck between a rock and hard place trying to provide a fully featured standard configuration, simple customisability and the ability to fully customise for those who want to reduce bundle size to the bare minimum.

Solution Brainstorm

We could remove defaultIntegrations and supply a way to init the SDKs without any defaults and preferably one that allows tree shaking.

The obvious way to do this would be to have two different init functions, one which includes the defaults and one which doesn't. For now I'm going to call these init and initRaw but I'm open to better suggestions!

It's also worth noting that initRaw could also include no default Transport to allow for greatest "tree shakeability".

import { init, initRaw, defaultIntegrations } from '@sentry/xxx';

// init the sdk with defaults
init({
   dsn: '__DSN__',
});

// this does the same as above!
initRaw({
   dsn: '__DSN__',
   integrations: defaultIntegrations,
})

Adding a custom integration:

import { init, initRaw, defaultIntegrations } from '@sentry/xxx';
import { MyWonderfulIntegration } from './integrations';

init({
   dsn: '__DSN__',
   integrations: [new MyWonderfulIntegration()]
});

initRaw({
   dsn: '__DSN__',
   integrations: [...defaultIntegrations, new MyWonderfulIntegration()],
})

Removing an integration:

import { init, initRaw, defaultIntegrations } from '@sentry/xxx';

init({
   dsn: '__DSN__',
   // this wont tree shake 🤷‍♂️
   integrations: (itgs) => itgs.filter(i => i.name !== "Breadcrumbs")
});

initRaw({
   dsn: '__DSN__',
   // this wont tree shake either 🤷‍♂️
   integrations: defaultIntegrations.filter(i => i.name !== "Breadcrumbs"),
})

initRaw({
   dsn: '__DSN__',
   integrations: [ /* manually include all the integrations you need for greatest tree shakability! */ ]
})

The integrations function option + filtering by name string to remove an integration feels a bit nasty but currently it appears to be the simplest solution to exclude an integration 🤔

[lforst]

I do like the idea of initRaw!

[AbhiPrasad]

In a chat with @yordis we discussed having the multiple inits, but we would have to take care to make sure that this is communicated accordingly.

We could make it that using a client + hub directly is the only way to treeshake this - but this does require the user to learn what the hub/client is.

// client has 0 defaults
const client = new BrowserClient(options);
const hub = new Hub(client);
makeMain(hub)

As an aside there is a general thinking of improving the integration API and interface. I'm going to open up a separate issue for this.

[timfish]

One of the downsides I found with the client + hub approach is that all the SDKs currently have custom code in their init functions which would be completely missed.

So for example with the browser SDK, getting the release from window.SENTRY_RELEASE and session tracking wouldn't start. We'd either need to document all the differences for each SDK or move the code elsewhere.

[AbhiPrasad]

Opened #4790 to chat about integrations API specifically

[yordis]

I would lean on being explicit about how Sentry is being set up instead of going for strategies like init and initRaw.

As I mentioned to @AbhiPrasad, you barely touch the setup of Sentry after you are done. So the cost of wiring isn't that bad.

Proper documentation, and educating the users will be much better. in the long-term (while causing some pain in the midtime).

I would lean on one function only init, and provide the default list of integrations, among examples of how to import them independently rather than to even show users about filter (let power users figure the pros-and-cons) since you even if they are 20 integrations, the code is quite simple and it is done once.

import { init, defaultIntegrations, IntegrationA, IntegrationB } from '@sentry/xxx';

// only defaults
init({
   dsn: '__DSN__',
   integrations: defaultIntegrations,
});

// or 

init({
   dsn: '__DSN__',
   integrations: [
     ...defaultIntegrations,
     makeMyIntegration({ /* ... */})
  ),
});

// some sentry integrations and custome one
init({
   dsn: '__DSN__',
   integrations: [
     makeIntegrationA(),
     makeIntegrationB(),
     makeMyIntegration({ /* ... */})
  ),
});

[yordis]

One of the downsides I found with the client + hub approach is that all the SDKs currently have custom code in their init functions which would be completely missed.

Now I remember why @AbhiPrasad I told you to focus last on this because, in order to improve some areas, you must break change at the edge.

---

For example,

So for example with the browser SDK, getting the release from window.SENTRY_RELEASE

Don't.

Be explicit

import { init, defaultIntegrations, IntegrationA, IntegrationB } from '@sentry/xxx';

init({ release: '' })

Or

import { init, getRelease } from '@sentry/xxx';

// you can always expose more functions
init({ release: getRelease() })

---

session tracking wouldn't start.

Help me to understand what is the use-case here. I think I remember but I dont.

[timfish]

While I agree and also personally prefer a fully explicit API, it is at odds with the Sentry SDK guidelines.

Configuration comes at a Cost

We optimize towards a great out of the box experience. Customization should just be that: customization. Defaults matter and those defaults should be sensible. Integrations should auto activate if they can as every integration not enabled just causes a worse customer experience.

My interpretation of these guidelines is that Sentry would like users to have access to as many common features as possible with the most basic initialisation code (ie. init({ dsn: '__DSN__' })).

This applies to sessions too:

session tracking wouldn't start.

Help me to understand what is the use-case here. I think I remember but I dont.

Browser session tracking is currently started automatically here in init:

sentry-javascript/packages/browser/src/sdk.ts

Lines 98 to 100 in 689265a

	if (options.autoSessionTracking) {
	startSessionTracking();
	}

This is no doubt because the SDK guidelines say:

SDKs should create and report sessions by default, choosing to report them individually or as aggregates depending on the type of application.

This philosophy runs throughout the SDKs and is why I have suggested two different init functions, one which follows the API guidelines and gives a great out of the box experience and one which is more explicit for those that prefer that or want ultimate control.

Enable Customers

While we generally should try to keep the API surfaces of SDKs reasonable small. At the same time we also need to make sure that we enable customers to achieve their goals.

So we should offer an explicit API because there is a requirement for tree shaking and there is customer demand. However, it probably shouldn't be the default or only API.

Forcing ALL users to migrate to an explicit API also almost certainly falls foul of:

Customer Value Matters

Every change comes at a cost and change for the change's sake should be avoided. If the change benefits a customer it's good to go, if change only fulfills its own purpose it just adds more chances for us to break existing code or to cause user frustration.

The Client + Hub API is already minimal and explicit but it's far from simple to migrate to from init. For example, if you're using @sentry/nextjs and migrate to Client + Hub you've got to copy code from here and here just to maintain the same functionality. If we supply an initRaw and an init that calls initRaw with defaults, users can migrate between the two by simply copying and modifying the init code.

[yordis]

My interpretation of these guidelines is that Sentry would like users to have access to as many common features as possible with the most basic initialisation code (ie. init({ dsn: 'DSN' })).

If that is the long-term goal, then two functions that allow full tree-shaking is what I would lean on.

Being said, you can focus on what the "non-zero-install" wiring will look like, and then focus on tackling such wiring problems. In the worst case, "all problems can be solved with another layer of Indirection function".

Even things like init({ dsn: '__DSN__' }) is making assumption that I will only use one Sentry client ;) I am not saying that doesn't make sense but solving an architectural design to then focus at the edge will allow you to come up with multiple user experiences optimized for different audiences without trading too much.

// just to make a point, I made up the clients idea.
init({
  clients: [
    composeMiddleware(
      makeRateLimiting({ /*...*/ }),
      makeHttpClient({ dsn: 'https://sentry.io/' })
    ),
    composeMiddleware(
      makeLogger({ /*...*/ }),
      makeHttpBackend({ dsn: 'https://self-hosted-legacy.io/' })
    )
  ]
});

---

About philosophy

I would question such a goal at an all-of-nothing. On the web, every byte counts, and such philosophy doesn't take into consideration the negative consequences.

I fully understand the intention, but as a customer first and formal, the user experience is being punished when you don't take into consideration the different personas.

Being said, such a goal could be achieved in a way that takes into consideration such personal, just respect power-users and their experiences as well.

So please take my push back with the intention to focus on the architecture first and then think about such things. That is why I even told @AbhiPrasad to ignore breaking changes at the edge so you can delay such conversations and whatnot. Most of the other ideas can be accomplished without breaking changes at the edge.

Browser session tracking is currently started automatically here in init:

I wouldn't put such configuration in the same bucket as other ones, since that is actually something that will make sure the SDK works. So having it by default makes total sense to me.

Being said all that, ake everything I said with a grain of salt, I am wrong more often than not 😅

[timfish]

I would question such a goal at an all-of-nothing. On the web, every byte counts, and such philosophy doesn't take into consideration the negative consequences.

I checked out the Rust SDK since it has to solve a similar issue where defaults could contribute to binary bloat.
This is the basic init code in the Rust docs:

let _guard = sentry::init(("https://examplePublicKey@o0.ingest.sentry.io/0", sentry::ClientOptions {
    release: sentry::release_name!(),
    ..Default::default()
}));

So if that's ok for Rust, maybe this is perfectly ok for the JavaScript SDK:

import { init, defaults } from '@sentry/xxx';

init({
  ...defaults,
  dsn: '__DSN__'
})

If you don't care about bundle size (for node.js) you could remove an integration from the defaults like:

import { init, defaults } from '@sentry/xxx';

init({
  ...defaults,
  dsn: '__DSN__',
  integrations: defaults.integrations.filter(i => i.name !== 'Console')
})

[yordis]

I think that is reasonable 🥳

[AbhiPrasad]

I'm going to remove this from the v8 tracking issue given we've partly addressed some of this, but not all the items.

[mydea]

I will close this issue for now, as we are not planning anything concrete here right now. We can always revisit this if we decide to do work around this in the future!