feat(replay): Deprecate privacy options in favor of a new API, remove some recording options #6645

billyvg · 2023-01-04T00:31:28Z

Deprecate maskTextSelector, maskTextClass, blockClass, blockSelector, ignoreClass in favor of new API: mask, block, ignore which accepts an array of CSS selectors.
Adds new API for unmasking and unblocking elements: unmask, unblock. This can be used in conjunction with maskAllText, blockAllMedia to have privacy by default and selectively unmask elements.

Note: We'll need to upgrade sentry-docs as well.

Closes #6559, #6582

billyvg

Thoughts on this API?

patches/rrweb+1.1.3.patch

billyvg · 2023-01-04T00:36:30Z

I'm going to create a branch on https://github.com/getsentry/rrweb where I'll push up changes to the rrweb library.

github-actions · 2023-01-04T00:42:00Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.83 KB (-0.02% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	61.48 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.5 KB (-0.03% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	54.79 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.22 KB (0%)
@sentry/browser - Webpack (minified)	66.19 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.25 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.54 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.75 KB (-0.02% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.03 KB (-0.02% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	44 KB (+1% 🔺)
@sentry/replay - Webpack (gzipped + minified)	38.76 KB (+1.17% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	61.33 KB (+0.69% 🔺)

mydea · 2023-01-04T08:35:23Z

Two thoughts on this:

I would prefer not to add unmaskTextSelector, as you can also accomplish this with maskInputSelector, via not, or am I mistaken? If that is not good enough, imho this should rather be some kind of include/exclude naming. Also we get into a really broad API at a certain time, because the same could be added for every other kind of selector. suddenly, you have unblockSelector, unignoreSelector etc. Then you end up with IMHO a kind of hard to grasp list of options:

blockAllMedia
blockClass
blockSelector
unblockSelector (new)
maskAllInputs
maskTextClass
maskTextSelector
unmaskTextSelector (new)
maskInputSelector (new)
unmaskInputSelector (new)
maskInputOptions
ignoreClass
ignoreSelector (new)
unignoreSelector (new)

I have to admit for me this (even with the options we have now) can be a bit confusing, it is rather un-symmetrical.

Instead of adding two more options like this, IMHO I'd rather prefer it to provide a better, more unified API for this. I am not entirely sure how that would look like, but for example something like this:

maskTextInclude: string | string[] - provide a selector or array of selectors to mask
maskTextExclude: string | string[] - provide a selector or array of selectors to not mask, even if they were included
maskInputInclude: string | string[]
maskInputExclude: string | string[]
redactInclude: string | string[] - or something like this, IMHO block is not really explanatory of what it does
redactExclude: string | string[]
ignoreEventsInclude: string | string[] - or something like this, instead of ignore which is also not totally clear IMHO
ignoreEventsExclude: string | string[]

something along this line would be a more consistent & easy to understand API from my POV. But obviously would also need adjustments to rrweb, as this is not really possible to do with what exists there now. IMHO the best way to implement such a thing on rrweb side is to allow to pass regex to all options, and then build a regex on our side for the array options (otherwise, it can be hard to exclude multiple selectors, because :not(selector1, selector2) is not supported everywhere, sadly). But that's an implementation detail then.

So, I guess if you feel strongly that adding these options for now is important, I'm OK with that, but IMHO it is really more of a bandaid and might come back to bite us later if we have a loooong list of options that we can't get rid of anymore.

Maybe we should first update rrweb to 2.0.0-alpha, before we make such changes to it 🤔 I think we'll want to update anyhow, and these changes may not work well anymore after the update.

billyvg · 2023-01-04T19:16:33Z

Thanks, you bring up some good points, replies below:

I would prefer not to add unmaskTextSelector, as you can also accomplish this with maskInputSelector, via not, or am I mistaken?

It's possible (although we previously had some trouble getting it to work when we were using the more broad * selector to mask, maybe it's possible now with the new selector), but it's also not very user friendly. It's more straightforward to have the user give a basic class/id selector instead of a bit more unusual pseudo-class.

As an aside, I would also like to make maskAllText (and probably blockAllMedia) into an API upstream in rrweb so that we don't have to use the catch all selector at all.

If that is not good enough, imho this should rather be some kind of include/exclude naming. Also we get into a really broad API at a certain time, because the same could be added for every other kind of selector. suddenly, you have unblockSelector, unignoreSelector etc. Then you end up with IMHO a kind of hard to grasp list of options:

blockAllMedia

blockClass

blockSelector

unblockSelector (new)

maskAllInputs

maskTextClass

maskTextSelector

unmaskTextSelector (new)

maskInputSelector (new)

unmaskInputSelector (new)

maskInputOptions

ignoreClass

ignoreSelector (new)

unignoreSelector (new)

I agree with this. Also having the -Class options are a bit redundant as well. unignoreSelector (and unblockSelector probably too) is not needed because we don't have an "ignore all" option.

I have to admit for me this (even with the options we have now) can be a bit confusing, it is rather un-symmetrical.

Instead of adding two more options like this, IMHO I'd rather prefer it to provide a better, more unified API for this. I am not entirely sure how that would look like, but for example something like this:

maskTextInclude: string | string[] - provide a selector or array of selectors to mask

maskTextExclude: string | string[] - provide a selector or array of selectors to not mask, even if they were included

maskInputInclude: string | string[]

maskInputExclude: string | string[]

redactInclude: string | string[] - or something like this, IMHO block is not really explanatory of what it does

redactExclude: string | string[]

ignoreEventsInclude: string | string[] - or something like this, instead of ignore which is also not totally clear IMHO

ignoreEventsExclude: string | string[]

This is not unlike what we are doing with un[type] and [type] tbh, except they are suffixes in this case. I do agree we can get rid of the class name based options. I don't think the types need to be an array as we can pass a comma separated string of selectors. It uses element.matches(), which accepts multiple selectors.

I do agree that "block" and "ignore" are not very clear. Renaming block -> redact is not really an improvement because redact will still be confused with mask imo. (block and mask are also vague and can be confused for the same thing). I do like renaming to ignoreEvents though.

So, I guess if you feel strongly that adding these options for now is important, I'm OK with that, but IMHO it is really more of a bandaid and might come back to bite us later if we have a loooong list of options that we can't get rid of anymore.

It's not the most important, but I think it's important to give users good control over privacy configuration. Let me work on an API proposal with #6641 in mind as well.

Maybe we should first update rrweb to 2.0.0-alpha, before we make such changes to it 🤔 I think we'll want to update anyhow, and these changes may not work well anymore after the update.

I actually built this on v2 because I was working off their master branch 🤦. Agree about upgrading to v2, but don't think we should wait to make these changes. Although we should agree on an API before proceeding.

billyvg · 2023-01-11T01:32:23Z

@mydea thoughts on this?

I will need to patch rrweb a bit more to add unblockSelector and ignoreSelector.

billyvg · 2023-01-19T02:17:58Z

packages/replay/src/integration.ts

+    ];
+
+    // @deprecated
+    if (blockClass) {


TODO: support regex

billyvg · 2023-01-26T17:48:54Z

packages/replay/src/integration.ts

+      inlineImages: false,
+      // collect fonts, but be aware that `sentry.io` needs to be an allowed
+      // origin for playback
+      collectFonts: true,


We may get some CORS errors on playback, but it should be fine as it will fallback to system fonts.

mydea · 2023-01-27T14:25:54Z

packages/replay/src/integration.ts

+      }),
+
+      // Our defaults
+      slimDOMOptions: 'all',


Just to be clear (we should put this in the changelog), this means passing through "random" rrweb option is not possible anymore in this release!

mydea · 2023-01-27T14:26:24Z

packages/replay/src/types.ts

+  /**
+   * Ignore input events for elements that match the CSS selectors in the list.
+   */
+  ignore?: string[];


should we go with ignoreEvents here?

@mydea hmm my worry is that people will confuse it for DOM events e.g. clicks?

mydea · 2023-01-27T14:26:54Z

packages/replay/src/types.ts

+  /**
+   * A callback function to customize how your text is masked.
+   */
+  maskFn?: Pick<RecordingOptions, 'maskTextFn'>;


Just thinking, do we need to expose this? Or is it OK to just not allow to configure this?

I'm thinking we should keep it since we are being so aggressive about privacy first

mydea

Looking good to me! 🚀

This was unintentionally removed in #6645 - it is still deprecated and will be removed at a later date.

This was unintentionally removed in #6645 - it is still deprecated and will be removed at a later date. Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

billyvg force-pushed the feat-replays-add-mask-and-unmask-options branch from 368e29e to be11ca5 Compare January 4, 2023 00:32

billyvg commented Jan 4, 2023

View reviewed changes

patches/rrweb+1.1.3.patch Outdated Show resolved Hide resolved

billyvg linked an issue Jan 4, 2023 that may be closed by this pull request

Replay: Allow masking of specific input elements (e.g: by selectors or ids) #6582

Closed

billyvg requested review from mydea and Lms24 January 4, 2023 00:34

billyvg force-pushed the feat-replays-add-mask-and-unmask-options branch from 8c1eaa2 to 23df92e Compare January 12, 2023 16:37

billyvg commented Jan 19, 2023

View reviewed changes

packages/replay/src/integration.ts Outdated

];

// @deprecated

if (blockClass) {

Copy link

Member Author

billyvg Jan 19, 2023

Choose a reason for hiding this comment

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

TODO: support regex

billyvg added 2 commits January 26, 2023 10:31

update readme

de1e48f

wip

f5ac26d

billyvg force-pushed the feat-replays-add-mask-and-unmask-options branch from 23df92e to f5ac26d Compare January 26, 2023 17:42

billyvg commented Jan 26, 2023

View reviewed changes

billyvg changed the title ~~feat(replay): Patch rrweb to add maskInputSelector and unmaskTextSelector~~ feat(replay): Deprecate privacy options in favor of a new API, remove some recording options Jan 26, 2023

lint

a1d37ad

billyvg marked this pull request as ready for review January 26, 2023 18:46

mydea reviewed Jan 27, 2023

View reviewed changes

mydea approved these changes Jan 27, 2023

View reviewed changes

billyvg merged commit e4e7e2e into master Jan 30, 2023

billyvg deleted the feat-replays-add-mask-and-unmask-options branch January 30, 2023 15:03

billyvg added a commit that referenced this pull request Jan 30, 2023

feat(replay): Add back deprecated maskInputOptions

5d16b6b

This was unintentionally removed in #6645 - it is still deprecated and will be removed at a later date.

billyvg mentioned this pull request Jan 30, 2023

feat(replay): Add back deprecated maskInputOptions #6981

Merged

billyvg mentioned this pull request Feb 2, 2023

[Replay] Move rrweb-specific configuration into recordingOptions #6390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replay): Deprecate privacy options in favor of a new API, remove some recording options #6645

feat(replay): Deprecate privacy options in favor of a new API, remove some recording options #6645

billyvg commented Jan 4, 2023 •

edited

billyvg left a comment

billyvg commented Jan 4, 2023

github-actions bot commented Jan 4, 2023 •

edited

mydea commented Jan 4, 2023

billyvg commented Jan 4, 2023

billyvg commented Jan 11, 2023

billyvg Jan 19, 2023

billyvg Jan 26, 2023

mydea Jan 27, 2023

mydea Jan 27, 2023

billyvg Jan 27, 2023

mydea Jan 27, 2023

billyvg Jan 27, 2023

mydea left a comment

feat(replay): Deprecate privacy options in favor of a new API, remove some recording options #6645

feat(replay): Deprecate privacy options in favor of a new API, remove some recording options #6645

Conversation

billyvg commented Jan 4, 2023 • edited

billyvg left a comment

Choose a reason for hiding this comment

billyvg commented Jan 4, 2023

github-actions bot commented Jan 4, 2023 • edited

size-limit report 📦

mydea commented Jan 4, 2023

billyvg commented Jan 4, 2023

billyvg commented Jan 11, 2023

billyvg Jan 19, 2023

Choose a reason for hiding this comment

billyvg Jan 26, 2023

Choose a reason for hiding this comment

mydea Jan 27, 2023

Choose a reason for hiding this comment

mydea Jan 27, 2023

Choose a reason for hiding this comment

billyvg Jan 27, 2023

Choose a reason for hiding this comment

mydea Jan 27, 2023

Choose a reason for hiding this comment

billyvg Jan 27, 2023

Choose a reason for hiding this comment

mydea left a comment

Choose a reason for hiding this comment

billyvg commented Jan 4, 2023 •

edited

github-actions bot commented Jan 4, 2023 •

edited