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

ref(tracing): Split @sentry/tracing into runtime specific packages #5815

Closed
7 tasks done
timfish opened this issue Sep 26, 2022 · 14 comments
Closed
7 tasks done

ref(tracing): Split @sentry/tracing into runtime specific packages #5815

timfish opened this issue Sep 26, 2022 · 14 comments

Comments

@timfish
Copy link
Collaborator

timfish commented Sep 26, 2022

Problem Statement

After some discussions around supporting newer JavaScript runtimes (#5611) @lforst pointed out the first thing that needs tackling is @sentry/tracing!

This package currently contains:

  • Runtime agnostic code used to add transaction support to the hub
  • Browser specific instrumentation code
  • Node.js specific instrumentation code

Goal: Remove the tracing package, move stuff into core/browser/node

Solution Brainstorm

It looks like the runtime agnostic code can safely be moved to @sentry/core without affecting bundle sizes since if it goes unused it gets tree-shaken.

The runtime specific code could be moved into package exports under the specific runtime, ie. @sentry/browser/tracing and @sentry/node/tracing. We've been using this in the Electron SDK for a while to keep bundlers happy with the two different contexts we run in. We can't export the tracing code at the root of the packages since importing tracing has side-effects!

I haven't marked this issue as Breaking yet because I think it would be possible to move this code while keeping @sentry/tracing as a stub that would simply re-export the moved code. @sentry/tracing would have to depend on both @sentry/browser and @sentry/node so I'm not sure how viable this is yet or if it's worth that extra step 🤔

Tasks

Preview Give feedback
  1. Package: core Package: tracing Status: Backlog Type: Improvement
    AbhiPrasad timfish
  2. AbhiPrasad
  3. 1 of 1
    Package: browser Status: Backlog
    AbhiPrasad timfish
  4. 1 of 1
    Package: node Status: Backlog
    timfish
  5. 7 of 7
    Package: tracing Status: Backlog
    timfish

Cleanup

Preview Give feedback
  1. 3 of 3
    AbhiPrasad
  2. Package: tracing Status: Backlog
    timfish
@lforst
Copy link
Member

lforst commented Sep 26, 2022

Continuing the thread from #5611 (comment):

Would we want to add all the hub extensions by default too?

I think so. Ideally, we wouldn't need hub extensions anymore since it's all already there.

We wouldn't want to run _autoloadDatabaseIntegrations in the Electron SDK because it hits app startup time significantly!

We had an internal discussion about this some time ago and we would like to turn off auto-discovery by default for the next major because of the startup time.

@timfish
Copy link
Collaborator Author

timfish commented Sep 26, 2022

If we move the existing runtime agnostic code over to @sentry/core without any significant changes and simply export the existing code there is no change to browser bundle size since it's just a load of isolated functions.

I still need to do some experimentation to see how much it adds if the code is fully integrated to the hub without extensions. I would guess this could add 5-10kB since the code ends up in the Hub class where tree shaking can't works its magic.

@timfish
Copy link
Collaborator Author

timfish commented Sep 27, 2022

I did the experimenting and moved tracing into @sentry/core and moved the code into Hub (removing hub extensions) and it adds roughly 7kB to the minified, uncompressed browser bundle (bundle.min.js).

@timfish
Copy link
Collaborator Author

timfish commented Oct 5, 2022

This PR on my fork shows how the runtime agnostic code from @sentry/tracing can be moved to @sentry/core without impacting bundle size. This is possible because it retains the use of hub extensions and the tracing code is not referenced by @sentry/core so it can be tree-shaken.

@AbhiPrasad
Copy link
Member

After doing some experimenting, we cannot use the approach in #7345 because this would break older bundlers.

@AbhiPrasad
Copy link
Member

As per #7372, we must do this change in v8

@AbhiPrasad
Copy link
Member

A possible option we can take on here is to create a new tracing package - and then make @sentry/tracing, @sentry/node and @sentry/browser all import from that new package.

This new package can export everything without side effects so it can be tree-shaken/not slow down the node package.

@mydea
Copy link
Member

mydea commented Mar 9, 2023

IMHO that can be fine, we can make the package purely private/internal, so it would really just be an implementation detail!

@timfish
Copy link
Collaborator Author

timfish commented Mar 10, 2023

It's been a while, but I think we worked around the older bundler issues while still having named package exports with the Electron SDK by outputting the cjs build output into correctly named directories in the root of the package.

  "exports": {
    "./main": {
      "require": "./main/index.js",
      "import": "./esm/main/index.js"
    },
    "./renderer": {
      "require": "./renderer/index.js",
      "import": "./esm/renderer/index.js"
    }
  },

We don't actually have any webpack 4 e2e tests, but I just did some quick testing and with the above, both const { init } = require('@sentry/electron/renderer'); and import { init } from '@sentry/electron/renderer'; work with webpack 4.

The major downside here is that cjs gets used and therefore no tree shaking occurs. In the case of the Electron SDK, this means that all the session replay code gets bundled. cjs is no doubt why we've seen issues like this even when Replay isn't used.

@AbhiPrasad
Copy link
Member

The major downside here is that cjs gets used and therefore no tree shaking occurs

This is what I ran into, and unfortunately means we can't move forward with this for the stock browser package.

@timfish
Copy link
Collaborator Author

timfish commented Mar 13, 2023

Once the above two linked PRs are merged, I'm guessing the next steps go something like:

  • Export BrowserTracing and related consts/types from @sentry/browser
  • Configure browser tracing bundler config from @sentry/browser
  • Export Node.js tracing integrations from @sentry/node
  • Update docs to cater for all the above changes
  • Deprecate all exports in @sentry/tracing

For v8

  • Remove @sentry/tracing package

@AbhiPrasad
Copy link
Member

@timfish one thing we'll have to consider is hub extensions and auto discovery of database integrations.

We'll want to make sure the flow looks like so:

Browser

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

Sentry.init({
  dsn: "__DSN__",
  tracesSampleRate: 1.0,
  integrations: [new Sentry.BrowserTracing()]
});

If you don't want to use BrowserTracing, you'll have to add the tracing extensions yourself:

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

Sentry.addTracingExtensions();

Sentry.init({
  dsn: "__DSN__",
  tracesSampleRate: 1.0,
});

Node

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

Sentry.addTracingExtensions();
Sentry.discoverTracingIntegrations();

Sentry.init({
  dsn: "__DSN__",
  tracesSampleRate: 1.0,
});

@timfish
Copy link
Collaborator Author

timfish commented Mar 17, 2023

We need to think about what types need re-exporting from node/browser from @sentry/core. getActiveTransaction is the obvious one but I'm sure there are more.

Once the migration is complete, but before we add any deprecation jsdocs, we'll try to migrate all the Sentry packages to use node/browser rather than tracing. This should give us a good idea of what exports we need!

@AbhiPrasad
Copy link
Member

Since we've removed @sentry/tracing from all the sdks that use it, and migrated the imports to node/core/browser accordingly, I think we can consider this done.

Next up is docs work in getsentry/sentry-docs#6518 and final deprecation in #7586, but we can use those issues to track this.

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

No branches or pull requests

5 participants