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

🐝 collect session replays via Sentry #4101

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Conversation

bnjmacdonald
Copy link
Contributor

@bnjmacdonald bnjmacdonald commented Nov 1, 2024

Collect session replays via Sentry for 1% of cookie-consenting user sessions. Closes owid/analytics#155.

Implementation notes and privacy considerations.

Remaining todos before merging:

  • Update privacy policy to mention Sentry usage
  • apply for Sentry nonprofit discount
  • temporarily add SENTRY_DSN to staging env (then remove after merging – we don't need to waste precious replay budget on staging sites going forward). owid/ops#233
  • If Sentry nonprofit discount not approved yet, start Sentry free trial (so that we aren't capped at 50 session replays)
  • add SENTRY_DSN to prod env (ops:templates/owid-admin-prod/admin-env.secret)

@owidbot
Copy link
Contributor

owidbot commented Nov 1, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-add-session-replays

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-11-01 01:40:56 UTC
Execution time: 1.19 seconds

Copy link
Contributor

@rakyi rakyi left a comment

Choose a reason for hiding this comment

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

Sorry, if you already discussed this before, but just a quick fly by review:

  • We should be probably using @sentry/react instead of @sentry/browser to get better contextual data
  • If we keep using Sentry we should stop using Bugsnag, but this can wait until we confirm it works as expected, let's just not forget about it

}),
],
replaysSessionSampleRate: ENV === "development" ? 1 : 0.01,
replaysOnErrorSampleRate: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Capturing replays on error seems even more useful than non-errors. Is there a good reason to disable it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more so the opposite for me, as I don't want our budget of session replays to get inadvertently consumed by a few pages that raise a lot of the same error over and over (e.g. we end up with 1k monthly session replays on /search out of our 5k budget just b/c of errors).

Obviously, keeping replaysOnErrorSampleRate low would help avoid this problem, but it was easier for now to just set this to 0 to get a clear baseline of how well the 5k monthly replays are distributed across our pages (without having to worry about how much errors skew the distribution of replays).

@bnjmacdonald bnjmacdonald force-pushed the add-session-replays branch 2 times, most recently from 37fdef5 to 0702378 Compare November 1, 2024 16:28
@bnjmacdonald
Copy link
Contributor Author

  • We should be probably using @sentry/react instead of @sentry/browser to get better contextual data

thanks Martin – done ✅

Copy link
Contributor

@larsyencken larsyencken 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, no obvious issues with the integration from my end. We can obviously tweak the sampling rates later as needed.

@bnjmacdonald bnjmacdonald merged commit 7abc704 into master Nov 5, 2024
16 of 18 checks passed
@bnjmacdonald bnjmacdonald deleted the add-session-replays branch November 5, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants