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

@sentry/browser size #1552

Closed
4 of 8 tasks
vkrol opened this issue Sep 19, 2018 · 69 comments
Closed
4 of 8 tasks

@sentry/browser size #1552

vkrol opened this issue Sep 19, 2018 · 69 comments
Assignees

Comments

@vkrol
Copy link
Contributor

vkrol commented Sep 19, 2018

Package + Version

  • @sentry/browser
  • @sentry/node
  • raven-js
  • raven-node (raven for node)
  • other:

Version:

4.0.2

Description

The size of the @sentry/browser is more than twice as large as the size of raven-js: 86 kB vs 39 kB (minified). In my opinion, this is definitely a regression and the serious reason not to update to the new version.

@HazAT
Copy link
Member

HazAT commented Sep 20, 2018

Hey, thanks for bringing this up.
While we understand and generally agree with your concerns about bundle size I think it's fair to first compare the gzip bundle sizes instead of the uncompressed minified file size:

@sentry/browser is 21.3799 KB
raven-js 13.44 KB

Also, while this might not be applicable to everyone, we provide and usually guide people to use our CDN Loader which will set up the SDK for you on your website.

see : https://docs.sentry.io/quickstart/?platform=browser

The footprint and impact on your pageload time of this script it <1KB gzipped while keeping the same functionality.

so tl;dr:
We are aware of it, we know there is room for improvement but it's not top priority right now.

@klaemo
Copy link

klaemo commented Sep 25, 2018

I think it's fair to first compare the gzip bundle sizes instead of the uncompressed minified file size:

I would argue it is also fair to compare minified sizes as perfomance issues not only arise from downloading javascript, but also from parsing and execution. ~92kb is quite hefty and could add up to 1s of parse time on low-end devices (just for this one library!).

I'm not sure where you take the number of < 1KB for the CDN'ed script from. Could you elaborate? When I open https://browser.sentry-cdn.com/4.0.4/bundle.min.js I see a gzipped size of 22KB.

You should be aware that sentry's sdk is just one of many libraries developers include.

PS: I love sentry, it's been super helpful for us. Web performance is just something I'm passionate about. ;)

@HazAT
Copy link
Member

HazAT commented Sep 25, 2018

@klaemo
Hehe, no worries 😅

Like I said, we are aware and it's not like we don't want it to be smaller.
Highest prio for us was to ship the new SDK, we will work to improve the bundle size over time.
There are many more steps we can take, e.g.: use tslib, combine strings ...
So there is a lot of space for improvements.

Sorry, the link I posted before is already outdated 🙃
I was referring to the Loader: https://docs.sentry.io/platforms/javascript/loader/
While the Loader also has it's limitations due it's async nature and in the end, it still fetches and injects the SDK (even if it's async) it's an alternative we offer because people asked for it.

@rayzru
Copy link

rayzru commented Sep 25, 2018

@HazAT Sorry, guys, but can you provide next version release date?
I mean, there are some changes already at the #master branch but not released yet. That one is deciding is 4x version is usable for Angular5+ projects though.

@HazAT
Copy link
Member

HazAT commented Sep 25, 2018

@rayzru Just released 4.0.5, but please keep posts related to the topic.

karlguillotte added a commit to avalanche-canada/ac-web that referenced this issue Sep 27, 2018
- extract from the vendors bundle as it is BIG: getsentry/sentry-javascript#1552
- Queue all events and send them when package loads
@oliviertassinari
Copy link
Contributor

oliviertassinari commented Oct 3, 2018

In my opinion, this is definitely a regression and the serious reason not to update to the new version.

💯 I was just about to upgrade up until I noticed that:

capture d ecran 2018-10-03 a 15 07 27

At least the package size is smaller, but I think that ⚠️ +10 KB of gzipped JavaScript in a bundle is significant. We will wait, keep up the good work :)

@hlehmann
Copy link

@HazAT Could it be possible to generate esm files. It would allow webpack to have better result with concatenation and tree shaking.

@jacobrask
Copy link

You might want to add some CI tool to track the bundle size of the package for each merge request. Since this issue it has actually grown to 100 kB, see https://bundlephobia.com/result?p=@sentry/browser@4.3.0

Try for instance https://github.com/siddharthkp/bundlesize

The default performance budget for an entry point in Webpack is 250 kb to make sure you get decent performance on any device and network. 100 kb of Sentry takes up quite a lot of that budget.

@jacobrask
Copy link

Please keep us updated if fixing this regression is on the roadmap at all, or if the library will just keep growing without a size budget.

We are paying customers and heavily invested in Sentry on both backend, native and web, but an upgrade to over 3x the size of the library (raven-js@3.23.1 is 31 kB) is not something we can justify.

@oliviertassinari
Copy link
Contributor

@jacobrask You can stick to the older version of the library, it's what we do at https://www.onepixel.com/, it's working fine 👌.
dont-confuse-motion-with-progress

@HazAT
Copy link
Member

HazAT commented Nov 13, 2018

@jacobrask It's definitely on our list and we also think there are some low hanging fruits where we can easily reduce our bundle size.
More and more people asking for this so we will probably tackle this sooner than expected.

@gaterking
Copy link
Contributor

gaterking commented Nov 14, 2018

@HazAT
Sentry browser SDK rollup bundle can be optimized. In bundle min js file, there are many tslib code repeatd. Such as __generator, __assign. In Browser env, it's best to share one code. Butthe browser project use another packages dist file. Maybe reduce the gzip size from 23K to 16K.

@vkrol
Copy link
Contributor Author

vkrol commented Nov 18, 2018

The bundle size is same in 4.3.2
https://bundlephobia.com/result?p=@sentry/browser@4.3.2 regardless of the changes from
#1738 :(

@HazAT
Copy link
Member

HazAT commented Nov 19, 2018

@vkrol We had to revert the changes that @gaterking made, at least for the npm package.
The bundle on CDN on the other hand should be smaller.

We will for sure re-add the changes but we need to talk about it since we need a dep on tslib for example.
Also how typings were generated was broken.

@vkrol
Copy link
Contributor Author

vkrol commented Nov 19, 2018

@HazAT OK, thanks.

@gaterking
Copy link
Contributor

@vkrol We had to revert the changes that @gaterking made, at least for the npm package.
The bundle on CDN on the other hand should be smaller.

We will for sure re-add the changes but we need to talk about it since we need a dep on tslib for example.
Also how typings were generated was broken.

Hope as soon as possible.

@HazAT
Copy link
Member

HazAT commented Nov 19, 2018

@gaterking @kamilogorek Fixed it already #1751
We just had to do an "urgent" release that's why we reverted it.

@mindplay-dk
Copy link

On the client-side, I basically want this to capture errors and submit them to the API.

What else is this library doing that's so complex?

It's really hard to understand why a simpler error-reporter needs to have such a substantial footprint 😐

@HazAT
Copy link
Member

HazAT commented Nov 27, 2018

@mindplay-dk It's mostly because JavaScript and Browsers are a mess ^^
We need to do a lot of fixing up broken/wrong stack traces.
The simple task of "just catching errors" is also way more complex than it seems.

@kamilogorek
Copy link
Contributor

It's really hard to understand why a simpler error-reporter needs to have such a substantial footprint 😐

@mindplay-dk because it's not simple.

Here's the code for just capturing errors in all the browsers and unifying them into a usable data structure: https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/tracekit.ts

@jacobrask
Copy link

Well done on the recent size reduction, greatly appreciated. 👍

A quick glance at the linked file shows code to handle errors for Opera 10, is that really still required? TraceKit also does not account for the (currently) 2x size increase from Raven, which was already large.

Some suggestions:

@gaterking
Copy link
Contributor

gaterking commented Nov 28, 2018

Is there some shared code(app:///${base} in rewriteframes.ts) in another packages such as package/core? They are not used by client, but used by node.

@mindplay-dk
Copy link

because it's not simple.

@kamilogorek yikes, you're obviously right... I realized JavaScript was a mess - I guess I've never dug into this area before, I didn't realize how bad this was. I guess there's really no simple way to handle stack-traces in JS. Just. Yikes. 😐

@kamilogorek
Copy link
Contributor

@cromefire yes, because it's meant to be used as a "snippet". However you can find it's code here https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/loader.js

@cromefire
Copy link
Contributor

Seems like I have to open a new PR, because with esm this would also be usable from your own code

@kamilogorek
Copy link
Contributor

We have a solution coming that will let you use loader alongside minimal and will effectively add only a few kB to your final bundle.

@cromefire
Copy link
Contributor

It shouldn't be hard to write a loader that loads that in under 1kb, so why not, I will try writing a quick one

@LarsDenBakker
Copy link

LarsDenBakker commented Jan 15, 2019

One thing that could help out a lot here is if some of the functionalities are optional.

For instance a really nice bare minimum could be:

  • native error stack trace (don't care that it's not optimal on some browsers)
  • user agent
  • timestamp
  • URL
  • match with source maps on the server

Any additional functionalities can just be an add on. We only support modern browsers, so we don't need to work around all the quirky behavior of old browsers.

@danielhusar
Copy link

We solved this by using webpack code splitting and load sentry client only on error.

sentry.js

import * as Sentry from '@sentry/browser';
Sentry.init({
  ...
  integrations: integrations => {
    return integrations.filter(integration => integration.name !== 'GlobalHandlers');
  },
});
export const logError = error => Sentry.captureException(error);

errors.js

const captureError = async error => {
 const { logError } await import(/* webpackChunkName: "sentry" */ './sentry');
 logError(error);
}
window.onerror = (message, url, line, column, error) => captureError(error);
window.onunhandledrejection = event => captureError(event.reason);

We do some more in there like populate breadcrumbs, add extra, add tags, etc.. but it's possible to use sentry client and not make your bundle bigger.

@cromefire
Copy link
Contributor

This is somewhat what I implemented in #1846

@Limess
Copy link

Limess commented Feb 25, 2019

Might be helpful to others:
I used the ESM build with webpack (4.29.5) by:

  • adding a webpack alias to use the ESM build rather than the standard build as there's no module declaration in package.json
resolve: {
    alias: {
		// use sentry ESM build which is not declared in the @sentry/browser package.json
        '@sentry/browser': path.resolve(
		    __dirname,
			'node_modules/@sentry/browser/esm',
		),
    }
  • add an exclusion to sentry/.+/esm to our babel-loader config, as it seems that the ESM build includes features newer than ES2015.
{
    test: /\.m?jsx?$/,
    loader: 'babel-loader',
    // compile sentry as the ESM build is new and ships modern features which break our supported browsers
    exclude: /(node_modules\/(?!(@sentry\/[^/]+\/esm))|bower_components)\//,
}

Notes:

  • We used aliases so we don't have to worry about bundling when using it in code (we do similar for lodash-es among other things)

@cromefire
Copy link
Contributor

@Limess

You can just redirect to @sentry/browser/esm:

resolve: {
    alias: {
        // use sentry ESM build which is not declared in the @sentry/browser package.json
        '@sentry/browser': '@sentry/browser/esm'
    }

But you don't need to add an alias, just import @sentry/browser/esm

For the loader it's easier to write it like this:

  • If you have other things in babel:
{
    test: /other_things/,
    include: [/@sentry\/.+\/esm/],
    exclude: /node_modules/,
    // config
}
  • If you don't:
{
    test: /@sentry\/.+\/esm/,
    exclude: /node_modules/,
    // config
}

Also remember to customize the babel config for your needs: babel docs, else it's not worth to use the esm version

@HazAT
Copy link
Member

HazAT commented Mar 21, 2019

Update: We will soon release new major version of the SDK which reduces the bundle size significantly.

26.1 kB - https://bundlephobia.com/result?p=@sentry/browser@4.6.4
vs.
17.2 kB - https://bundlephobia.com/result?p=@sentry/browser@5.0.0-rc.1

Our prebuilt CDN versions even show better results (not sure how bundlephobia measures stuff)

ES6:  14.3535 kB
ES5:  15.6846 kB

Anyway, I wanted to let you know that were are still working on reducing the bundle size further but this should already be a huge step in the right direction.

Note on upgrading: It's a major bump since there are many internal changes in the SDK. There shouldn't be any code changes necessary on your side. We are currently testing the new version ourselves on sentry.io to see how it behaves. ref: getsentry/sentry#12492

Disclaimer: Don't use 5.0.0-rc.x in production yet as we do 🙈

@mindplay-dk
Copy link

@HazAT thank you for taking this seriously! this is a big step forward - I am already much less concerned about deploying this now :-)

@kamilogorek
Copy link
Contributor

image

@Sija
Copy link
Contributor

Sija commented Apr 1, 2019

@kamilogorek Just out of curiosity, could you add to the comparison the last version from 3.x branch?

@HazAT
Copy link
Member

HazAT commented Apr 2, 2019

I am closing this issue, as of now, we think, the reduction we introduced moving from v4 to v5 is a trend in the right direction. We will still try to reduce it further and we will be very conscious about increasing it ever again.

As a quick remark, we really only like to compare our "bundle" size, since depending on what bundler you are using your mileage may vary, so here are the CDN bundle number we ship(ped):

Package Version Size in Bytes Size in kB Link
raven-js 3.27.0 13741 Bytes ~13.4kB https://cdn.ravenjs.com/3.27.0/raven.min.js
@sentry/browser 4.6.6 22607 Bytes ~22.1kB https://browser.sentry-cdn.com/4.6.6/bundle.min.js
@sentry/browser 5.0.3 16059 Bytes ~15.7kB https://browser.sentry-cdn.com/5.0.3/bundle.min.js

With v5 we also ship and esm build, meaning if your are using a bundler it should be able to treeshake unused code paths.

Thank you all for your patience 🙇

@HazAT HazAT closed this as completed Apr 2, 2019
@vkrol
Copy link
Contributor Author

vkrol commented Apr 2, 2019

@HazAT @kamilogorek awesome!

@berkant
Copy link

berkant commented Oct 5, 2020

@Limess Is it still relevant to use this today: @sentry/browser/esm instead of @sentry/browser?

It's imported like import * as Sentry from "@sentry/browser/esm"; and bundled with webpack -p but I see no difference in bundle sizes. I also have a bare webpack.config.js with no plugins or loaders.

@piotr-cz
Copy link
Contributor

@0xbkt It doesn't make difference in bundle size, at least now when using rollup to bundle app.

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