ref(core): Reduce inboundfilters bundle size #4625

AbhiPrasad · 2022-02-23T20:47:06Z

Had this branch lying around for a while, went back and cleaned it up

Move all private methods from InboundFilters into their own standalone functions
Pass in options explicitly so you can do allowUrls.length instead of options.allowUrls.length (can't minify allowUrls in the 2nd option)
Remove typeof self logic since now we explicitly pass everything in
Rewrite tests to stop relying on private methods but instead assert on event processor

github-actions · 2022-02-23T20:54:26Z

size-limit report

Path	Base Size (20b3e38)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.66 KB	19.58 KB	-0.4% 🔽
@sentry/browser - ES5 CDN Bundle (minified)	62.83 KB	62.46 KB	-0.59% 🔽
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.24 KB	18.19 KB	-0.28% 🔽
@sentry/browser - ES6 CDN Bundle (minified)	56.15 KB	55.96 KB	-0.34% 🔽
@sentry/browser - Webpack (gzipped + minified)	22.78 KB	22.72 KB	-0.24% 🔽
@sentry/browser - Webpack (minified)	80.16 KB	80.38 KB	+0.28% 🔺
@sentry/react - Webpack (gzipped + minified)	22.81 KB	22.76 KB	-0.24% 🔽
@sentry/nextjs Client - Webpack (gzipped + minified)	47.78 KB	47.69 KB	-0.18% 🔽
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.53 KB	25.46 KB	-0.3% 🔽
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.85 KB	23.81 KB	-0.19% 🔽

lobsterkatie · 2022-02-23T22:26:43Z

packages/core/src/integrations/inboundfilters.ts

-      return true;
+/** JSDoc */
+export function _mergeOptions(
+  intOptions: Partial<InboundFiltersOptions> = {},


What is int in intOptions? Could we give this a clearer name?

integrationOptions, we can make that clearer (internalIntegrationOptions?)

I tried not to touch the original variables names as much as possible to make it clear how we've migrated from before -> after.

Ah, got it. Yeah, I think spelling it out is clearer.

And yeah, if you want this to be a pure moving-stuff-around PR and then integrate some of the improvements separately, I think that's fine and I get the reasoning.

packages/core/src/integrations/inboundfilters.ts

packages/core/test/lib/integrations/inboundfilters.test.ts

lobsterkatie · 2022-02-23T23:23:59Z

packages/core/test/lib/integrations/inboundfilters.test.ts

+import { InboundFilters, InboundFiltersOptions } from '../../../src/integrations/inboundfilters';
+
+/** JSDoc */
+function createInboundFiltersEventProcessor(


It took me a second or third read to totally grok this function, specifically the fact that

a) all of the machinery is only there in aid of the side effect of creating the event processor, and
b) as a consequence, these aren't mocks I need to care about later on in the tests.

It's also not totally clear to me as a reader (yet) why what we want is the event processor rather than a dummy instance of the integration.

I think it would be helpful to add a docstring explaining those things.

It's also not totally clear to me as a reader (yet) why what we want is the event processor rather than a dummy instance of the integration.

This is also a function of the unit testing of integrations being kind of inconsistent.

Will add a doc string

packages/core/test/lib/integrations/inboundfilters.test.ts

lobsterkatie · 2022-02-24T00:45:06Z

packages/core/src/integrations/inboundfilters.ts

+  if (_isSentryError(event, options.ignoreInternal)) {
+    if (isDebugBuild()) {
+      logger.warn(`Event dropped due to being internal Sentry Error.\nEvent: ${getEventDescription(event)}`);
    }
-    if (this._isIgnoredError(event, options)) {
-      if (isDebugBuild()) {
-        logger.warn(
-          `Event dropped due to being matched by \`ignoreErrors\` option.\nEvent: ${getEventDescription(event)}`,
-        );
-      }
-      return true;
+    return true;
+  }


Rather than passing the option and checking it in _isSentryError (and it potentially turning effectively into a no-op), I think it would be better to check it here, and not even call the function if the question is made moot by the option's value.

(That said, do you know why we even have this option? It's not documented, and it's not used anywhere in any sentry repo. Could we just get rid of it?)

We use this to drop all SentryError errors, which is used pretty extensively throughout the codebase. I don't think we want people to turn this off.

For example, we throw a SentryError if we don't parse a DSN properly.

I think it would be better to check it here, and not even call the function if the question is made moot by the option's value

Great point, will change.

I don't think we want people to turn this off.

Exactly. And we don't anywhere. What I'm asking is why we need to have an off switch at all.

This is a good question. I'm going to open up a follow up issue - but leave it like this for now.

lobsterkatie · 2022-02-24T00:46:02Z

packages/core/src/integrations/inboundfilters.ts

-    if (!options.allowUrls || !options.allowUrls.length) {
-      return true;
+/** JSDoc */
+function _isSentryError(event: Event, ignoreInternal: Partial<InboundFiltersOptions>['ignoreInternal']): boolean {


Suggested change

function _isSentryError(event: Event, ignoreInternal: Partial<InboundFiltersOptions>['ignoreInternal']): boolean {

function _isSentryError(event: Event, ignoreInternal: boolean): boolean {

It's not going to change, so why not just say what it is?

It's not going to change, so why not just say what it is?

It's to communicate intent - the whole purpose of aliasing types.

packages/core/src/integrations/inboundfilters.ts

lobsterkatie

Three more, same idea as above.

lobsterkatie · 2022-02-24T00:56:47Z

packages/core/src/integrations/inboundfilters.ts


+/** JSDoc */
+function _isDeniedUrl(event: Event, denyUrls: Partial<InboundFiltersOptions>['denyUrls']): boolean {


Suggested change

function _isDeniedUrl(event: Event, denyUrls: Partial<InboundFiltersOptions>['denyUrls']): boolean {

function _isDeniedUrl(event: Event, denyUrls: Array<string | RegExp>;): boolean {

lobsterkatie · 2022-02-24T00:57:27Z

packages/core/src/integrations/inboundfilters.ts

-      (options.ignoreErrors as Array<RegExp | string>).some(pattern => isMatchingPattern(message, pattern)),
-    );
+/** JSDoc */
+function _isAllowedUrl(event: Event, allowUrls: Partial<InboundFiltersOptions>['allowUrls']): boolean {


Suggested change

function _isAllowedUrl(event: Event, allowUrls: Partial<InboundFiltersOptions>['allowUrls']): boolean {

function _isAllowedUrl(event: Event, allowUrls: Array<string | RegExp>;]): boolean {

lobsterkatie · 2022-02-24T00:59:18Z

packages/core/src/integrations/inboundfilters.ts

-      return false;
-    }
+/** JSDoc */
+function _isIgnoredError(event: Event, ignoreErrors: Partial<InboundFiltersOptions>['ignoreErrors']): boolean {


Suggested change

function _isIgnoredError(event: Event, ignoreErrors: Partial<InboundFiltersOptions>['ignoreErrors']): boolean {

function _isIgnoredError(event: Event, ignoreErrors: Array<string | RegExp>;): boolean {

Although this may make that specific line, I like using the alias here, it makes things more consistent. Readers can scroll to the InboundFiltersOptions above and see the type exactly.

The alias makes the intent of the function very clear, and also communicates how exactly this function is being used - which I think is worth the extra search step a reader has to do to find the exact type.

My rationale here applies for the suggestions below.

All right - agree to disagree on this one.

I'm definitely with you that communicating intent and correct usage is important - that's why I'm pretty religious about adding full docstrings everywhere. When it comes to types, though, what I'm generally going for is ease of reading and the ability to understand quickly what something is - so that scrolling you mentioned is precisely what I'm trying to avoid.

(To be fair, it's not just you - I take issue with derived types in general. To me they're a tool of absolute last resort, and I only use them when I've utterly failed to get TS to accept any other, more direct description, and finally give up and say, Okay, TS, if you're going to be so difficult about it, you figure it out.)

All of the above said - if we are going to keep it as derived types, I don't think we need the Partial, do we?

You're right, we can remove the Partial usage.

lobsterkatie · 2022-02-24T01:02:36Z

Rewrite tests to stop relying on private methods but instead assert on event processor

This makes the tests a lot cleaner. Nice!

As part of centralizing and unifying our CDN rollup config, this uses the config generation function introduced in #4650 to generate the config for all browser bundles. Unlike many of the other PRs in this series, this one does actually have an effect on certain bundles, to wit: - `_mergeOptions` is no longer a protected-from-minifcation property, as it's about to be extracted into a stand-alone function. (See #4625.) - Up until this point, one difference between the `@sentry/browser` rollup config and all of the other rollup configs was the former's use of the `__SENTRY_NO_DEBUG__` flag to suppress logs in the minified bundles. Rather than add a parameter to the function controlling whether or not each package should use the flag, I decided it was better to just apply the flag to all minified bundles.

AbhiPrasad · 2022-03-18T08:14:49Z

Changed to remove the type alias

AbhiPrasad · 2022-03-18T09:26:03Z

Hmm idk what's going on with size-bot. Any ideas?

AbhiPrasad · 2022-03-23T13:11:33Z

Size bot fixed itself 🙏

lforst

Big fan of this PR. Left some optional action items. The one on the ignoreInternal arg would be really nice to have.

lforst · 2022-03-23T15:57:15Z

packages/core/src/integrations/inboundfilters.ts

-    }
-    return false;
+/** JSDoc */
+export function _mergeOptions(


(optional) Wondering if we should drop the underscore in the extracted function names...

I'll leave it here for now, but I'll remove the JSDoc as per your comments below.

I left it with the underscore initially to minimize the diff, and make it clear as possible how the functions moved.

lforst · 2022-03-23T16:26:17Z

packages/core/src/integrations/inboundfilters.ts

-    if (!options.allowUrls || !options.allowUrls.length) {
-      return true;
+/** JSDoc */
+function _isSentryError(event: Event, ignoreInternal?: boolean): boolean {


Can we get rid of the ignoreInternal argument? It seems unrelated to what the function is saying it does.

The if (_isSentryError(event, options.ignoreInternal)) { above would become if (!options.ignoreInternal && _isSentryError(event)) {. Might even save some bytes 🤔

Good change, will do!

packages/core/src/integrations/inboundfilters.ts

lforst

Thanks for implementing my feedback!

AbhiPrasad requested review from a team, Lms24 and lobsterkatie and removed request for a team February 23, 2022 20:47

AbhiPrasad added this to the Treeshaking / Bundle Size milestone Feb 23, 2022

lobsterkatie reviewed Feb 24, 2022

View reviewed changes

AbhiPrasad added 7 commits March 1, 2022 09:23

ref(core): Reduce inboundfilters bundle size

5515dd8

add tests

a4e6425

move into fixtures

48134b2

yarn fix

174ffe2

address PR review

c1e171d

move fixtures around

7f01599

review comment

ecb6460

AbhiPrasad force-pushed the abhi-private-inboundfilter branch from adc13ba to ecb6460 Compare March 1, 2022 14:23

AbhiPrasad requested a review from lobsterkatie March 1, 2022 14:23

lobsterkatie mentioned this pull request Mar 2, 2022

ref(build): Use rollup config function for @sentry/browser #4668

Merged

AbhiPrasad added 2 commits March 18, 2022 09:13

Merge branch 'master' into abhi-private-inboundfilter

beffc9f

remove type alias

f2bfe03

AbhiPrasad requested a review from a team March 18, 2022 08:16

AbhiPrasad enabled auto-merge (squash) March 18, 2022 08:49

Merge branch 'master' into abhi-private-inboundfilter

c277576

AbhiPrasad requested a review from lforst March 23, 2022 13:11

lforst reviewed Mar 23, 2022

View reviewed changes

address PR review feedback

94eac23

AbhiPrasad requested a review from lforst March 23, 2022 17:28

lforst approved these changes Mar 23, 2022

View reviewed changes

AbhiPrasad merged commit 59a0cb6 into master Mar 23, 2022

AbhiPrasad deleted the abhi-private-inboundfilter branch March 23, 2022 17:57

Lms24 mentioned this pull request Feb 2, 2023

_mergeOptions is not a function at React SDK #7032

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(core): Reduce inboundfilters bundle size #4625

ref(core): Reduce inboundfilters bundle size #4625

AbhiPrasad commented Feb 23, 2022 •

edited by lobsterkatie

github-actions bot commented Feb 23, 2022 •

edited

lobsterkatie Feb 23, 2022

AbhiPrasad Feb 24, 2022

lobsterkatie Feb 25, 2022

lobsterkatie Feb 23, 2022

AbhiPrasad Feb 24, 2022

lobsterkatie Feb 24, 2022

AbhiPrasad Feb 24, 2022

lobsterkatie Feb 25, 2022 •

edited

AbhiPrasad Mar 1, 2022

lobsterkatie Feb 24, 2022

AbhiPrasad Feb 24, 2022

lobsterkatie left a comment

lobsterkatie Feb 24, 2022

lobsterkatie Feb 24, 2022

lobsterkatie Feb 24, 2022

AbhiPrasad Feb 24, 2022

lobsterkatie Feb 25, 2022

lobsterkatie Mar 1, 2022

AbhiPrasad Mar 1, 2022 •

edited

lobsterkatie commented Feb 24, 2022

AbhiPrasad commented Mar 18, 2022

AbhiPrasad commented Mar 18, 2022

AbhiPrasad commented Mar 23, 2022

lforst left a comment

lforst Mar 23, 2022

AbhiPrasad Mar 23, 2022

lforst Mar 23, 2022

AbhiPrasad Mar 23, 2022

lforst left a comment

	function _isSentryError(event: Event, ignoreInternal: Partial<InboundFiltersOptions>['ignoreInternal']): boolean {
	function _isSentryError(event: Event, ignoreInternal: boolean): boolean {


		/** JSDoc */
		function _isDeniedUrl(event: Event, denyUrls: Partial<InboundFiltersOptions>['denyUrls']): boolean {

	function _isAllowedUrl(event: Event, allowUrls: Partial<InboundFiltersOptions>['allowUrls']): boolean {
	function _isAllowedUrl(event: Event, allowUrls: Array<string \| RegExp>;]): boolean {

	function _isIgnoredError(event: Event, ignoreErrors: Partial<InboundFiltersOptions>['ignoreErrors']): boolean {
	function _isIgnoredError(event: Event, ignoreErrors: Array<string \| RegExp>;): boolean {

ref(core): Reduce inboundfilters bundle size #4625

ref(core): Reduce inboundfilters bundle size #4625

Conversation

AbhiPrasad commented Feb 23, 2022 • edited by lobsterkatie

github-actions bot commented Feb 23, 2022 • edited

size-limit report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobsterkatie Feb 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobsterkatie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad Mar 1, 2022 • edited

Choose a reason for hiding this comment

lobsterkatie commented Feb 24, 2022

AbhiPrasad commented Mar 18, 2022

AbhiPrasad commented Mar 18, 2022

AbhiPrasad commented Mar 23, 2022

lforst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lforst left a comment

Choose a reason for hiding this comment

AbhiPrasad commented Feb 23, 2022 •

edited by lobsterkatie

github-actions bot commented Feb 23, 2022 •

edited

lobsterkatie Feb 25, 2022 •

edited

AbhiPrasad Mar 1, 2022 •

edited