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

feat(replay): Add beforeAddRecordingEvent Replay option #8124

Merged
merged 2 commits into from May 17, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented May 15, 2023

Allows you to modify/filter custom recording events for replays. Note this is only a recording event, not the replay event. Custom means that the events do not relate or affect the DOM recording, but rather they are additional events that the Replay integration adds for additional features.

This adds an option for the Replay integration beforeAddRecordingEvent to process a recording (rrweb) event before it is added to the event buffer. Example:

new Sentry.Replay({
  beforeAddRecordingEvent: (event) => {
    // Filter out specific events
    if (event.data.tag === 'foo') {
      return null;
    }

    // Remember to return an event if you want to keep it!
    return event;
  }
});

Closes #8127

Allows you to modify/filter recording events for replays. Note this is only a recording event, not the replay event.
@billyvg billyvg force-pushed the feat-replay-add-before-add-recording-event branch from f6ab9a2 to f8cfe73 Compare May 15, 2023 22:53
@github-actions
Copy link
Contributor

github-actions bot commented May 15, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 21.03 KB (+0.03% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 65.63 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 19.57 KB (+0.04% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 58.09 KB (+0.02% 🔺)
@sentry/browser - Webpack (gzipped + minified) 21.17 KB (0%)
@sentry/browser - Webpack (minified) 69.04 KB (0%)
@sentry/react - Webpack (gzipped + minified) 21.19 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 49.12 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 28.65 KB (+0.03% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 26.89 KB (+0.04% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 48.18 KB (+1.2% 🔺)
@sentry/replay - Webpack (gzipped + minified) 42.05 KB (+1.41% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 67.14 KB (+0.95% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.04 KB (+1.11% 🔺)

({ replay, integration } = await mockSdk({
replayOptions: {
beforeAddRecordingEvent: event => {
const eventData = event.data as Record<string, any>;
Copy link
Member Author

Choose a reason for hiding this comment

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

Types are not ideal here :/

I'll work on a follow-up to see how we can improve them.

@billyvg billyvg requested a review from Lms24 May 16, 2023 00:23
@billyvg billyvg marked this pull request as ready for review May 16, 2023 00:31
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -2,6 +2,8 @@

## Unreleased

- feat(replay): Add `beforeAddRecordingEvent` Replay option
Copy link
Member

Choose a reason for hiding this comment

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

No action required, just as a side note: You don't have to add this entry to the unreleased section. We always create a changelog PR when we release which is when we add all entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah thought it would make it easier for whoever is publishing

Comment on lines 274 to 276
/**
* Callback before adding a recording event
*/
Copy link
Member

Choose a reason for hiding this comment

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

l: Let's add a sentence about what to use this function for and what to return(i.e. returning null will drop the event). Just to make it a little clearer to users

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should probably point out that dropping one event here won't result in the entire replay being dropped, wdyt?

Copy link
Member Author

@billyvg billyvg May 16, 2023

Choose a reason for hiding this comment

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

I've also added the condition that we only call this callback for custom events, e.g. only when event.type == 5 (otherwise we risk user error breaking the replay), thoughts on that @Lms24?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that sounds good to me 👍 less chances for folks to screw things up with the actual replay! Let's just make sure we point this out in the JSDoc

@billyvg
Copy link
Member Author

billyvg commented May 16, 2023

@Lms24 One question I forgot to ask... does this option belong in the Replay integration instanciation, or root Sentry SDK init()?

@Lms24
Copy link
Member

Lms24 commented May 17, 2023

Does this option belong in the Replay integration instanciation, or root Sentry SDK init()?

I thought about this a little and I'd prefer it to be a Replay integration option, especially given that it's now only called for custom events. I know that his option is in a way similar to e.g. beforeSend hooks but it still seems very fine-grained and Replay-specific to me. More like we're giving users the option to drop/modify a part of rather than being able to control the entire replay.

@billyvg
Copy link
Member Author

billyvg commented May 17, 2023

Does this option belong in the Replay integration instanciation, or root Sentry SDK init()?

I thought about this a little and I'd prefer it to be a Replay integration option, especially given that it's now only called for custom events. I know that his option is in a way similar to e.g. beforeSend hooks but it still seems very fine-grained and Replay-specific to me. More like we're giving users the option to drop/modify a part of rather than being able to control the entire replay.

Agreed 👍

@billyvg billyvg merged commit 5543808 into develop May 17, 2023
62 checks passed
@billyvg billyvg deleted the feat-replay-add-before-add-recording-event branch May 17, 2023 17:33
@jcfrancisco
Copy link

@billyvg @Lms24 Thank you so much for adding this, we'll try it out soon. ✌️

Regarding whether options are better placed in Sentry.init or in the Replay initialization, I noticed that the two sample rates were in Sentry.init, and that they used to be in the Replay initialization, but that was deprecated. On our side we'd prefer it to be in the Replay initialization -- we currently use a workaround (pictured below) your Solutions Engineer recommended for now. This workaround is okay for us for now but modifying an object in place feels a little weird, and it's also not documented. Wonder if this is what you recommend going forward?

image

@billyvg
Copy link
Member Author

billyvg commented May 23, 2023

@jcfrancisco We recently added some documentation for this.

FWIW this is also how we do it at Sentry.

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

Successfully merging this pull request may close these issues.

Option to redact part of the URL within a Session Replay
3 participants