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.

Sign up for GitHub

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

Jump to bottom

fix(replay): browserReplayIntegration should not be included by default #4270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(replay): browserReplayIntegration should not be included by default #4270

wants to merge 1 commit into from

Conversation

krystofwoldrich
Copy link
Member

📢 Type of change

  • Bugfix

📜 Description

This PR fixes a web replay bug where the custom options would be ignored because they would be overwritten by the default web replay integration.

💚 How did you test it?

sample app, added unit tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

@github-actions GitHub Actions
Copy link
Contributor

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 379.08 ms 429.06 ms 49.98 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
9cab16b+dirty 370.82 ms 416.37 ms 45.55 ms
6e8584e+dirty 383.37 ms 400.84 ms 17.47 ms
31fcca2+dirty 366.64 ms 395.78 ms 29.14 ms
0d3e677+dirty 384.24 ms 431.45 ms 47.21 ms
e73d82f+dirty 377.67 ms 407.06 ms 29.39 ms
ad6c299+dirty 336.47 ms 362.89 ms 26.42 ms
2534337+dirty 597.14 ms 665.04 ms 67.90 ms
c398f67+dirty 315.08 ms 345.60 ms 30.52 ms
5446992+dirty 371.61 ms 390.00 ms 18.39 ms
80b2ce3+dirty 271.29 ms 316.47 ms 45.18 ms

App size

Revision Plain With Sentry Diff
9cab16b+dirty 7.15 MiB 8.35 MiB 1.20 MiB
6e8584e+dirty 7.15 MiB 8.13 MiB 1002.18 KiB
31fcca2+dirty 7.15 MiB 8.18 MiB 1.03 MiB
0d3e677+dirty 7.15 MiB 8.35 MiB 1.20 MiB
e73d82f+dirty 7.15 MiB 8.34 MiB 1.19 MiB
ad6c299+dirty 7.15 MiB 8.04 MiB 912.17 KiB
2534337+dirty 7.15 MiB 8.11 MiB 988.68 KiB
c398f67+dirty 7.15 MiB 8.21 MiB 1.07 MiB
5446992+dirty 7.15 MiB 8.12 MiB 999.45 KiB
80b2ce3+dirty 7.15 MiB 8.04 MiB 911.02 KiB

@github-actions GitHub Actions
Copy link
Contributor

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.14 ms 1234.80 ms 8.65 ms
Size 2.36 MiB 3.10 MiB 752.56 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e540498+dirty 1220.61 ms 1212.93 ms -7.68 ms
b95b8af+dirty 1221.39 ms 1228.52 ms 7.13 ms
5bb8d5f+dirty 1235.47 ms 1237.39 ms 1.92 ms
eb1e19f+dirty 1209.56 ms 1214.94 ms 5.38 ms
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
31fcca2+dirty 1209.17 ms 1216.21 ms 7.04 ms
1c65324+dirty 1235.17 ms 1235.08 ms -0.09 ms
8c88ac7+dirty 1205.13 ms 1218.87 ms 13.74 ms
63ed251+dirty 1232.55 ms 1238.77 ms 6.22 ms
d2c32bb+dirty 1223.69 ms 1229.49 ms 5.80 ms

App size

Revision Plain With Sentry Diff
e540498+dirty 2.36 MiB 3.14 MiB 793.34 KiB
b95b8af+dirty 2.36 MiB 3.14 MiB 793.32 KiB
5bb8d5f+dirty 2.36 MiB 2.92 MiB 570.22 KiB
eb1e19f+dirty 2.36 MiB 3.08 MiB 737.21 KiB
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
31fcca2+dirty 2.36 MiB 2.90 MiB 552.95 KiB
1c65324+dirty 2.36 MiB 3.04 MiB 698.64 KiB
8c88ac7+dirty 2.36 MiB 3.10 MiB 752.63 KiB
63ed251+dirty 2.36 MiB 3.10 MiB 752.55 KiB
d2c32bb+dirty 2.36 MiB 3.08 MiB 737.22 KiB

@github-actions GitHub Actions
Copy link
Contributor

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1234.41 ms 1222.78 ms -11.63 ms
Size 2.92 MiB 3.66 MiB 757.09 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e540498+dirty 1254.92 ms 1247.21 ms -7.71 ms
b95b8af+dirty 1235.60 ms 1242.06 ms 6.46 ms
5bb8d5f+dirty 1215.04 ms 1217.52 ms 2.48 ms
eb1e19f+dirty 1229.91 ms 1231.63 ms 1.71 ms
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
31fcca2+dirty 1222.04 ms 1226.51 ms 4.47 ms
1c65324+dirty 1239.71 ms 1239.86 ms 0.15 ms
8c88ac7+dirty 1240.66 ms 1247.42 ms 6.76 ms
63ed251+dirty 1223.27 ms 1222.94 ms -0.33 ms
d2c32bb+dirty 1244.00 ms 1245.77 ms 1.77 ms

App size

Revision Plain With Sentry Diff
e540498+dirty 2.92 MiB 3.69 MiB 794.14 KiB
b95b8af+dirty 2.92 MiB 3.69 MiB 794.16 KiB
5bb8d5f+dirty 2.92 MiB 3.48 MiB 575.85 KiB
eb1e19f+dirty 2.92 MiB 3.64 MiB 742.82 KiB
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
31fcca2+dirty 2.92 MiB 3.46 MiB 557.31 KiB
1c65324+dirty 2.92 MiB 3.61 MiB 705.56 KiB
8c88ac7+dirty 2.92 MiB 3.66 MiB 757.12 KiB
63ed251+dirty 2.92 MiB 3.66 MiB 757.10 KiB
d2c32bb+dirty 2.92 MiB 3.64 MiB 742.84 KiB

@krystofwoldrich krystofwoldrich mentioned this pull request Nov 13, 2024
Draft
7 tasks
lucas-zimerman
lucas-zimerman reviewed Nov 13, 2024
View reviewed changes
CHANGELOG.md
@@ -41,6 +41,7 @@
- Remove duplicate HTTP Client Errors on iOS ([#4250](https://github.com/getsentry/sentry-react-native/pull/4250))
- Replay `maskAll*` set to `false` on iOS kept all masked ([#4257](https://github.com/getsentry/sentry-react-native/pull/4257))
- Add missing `getRootSpan`, `withActiveSpan` and `suppressTracing` exports from `@sentry/core`, and `SeverityLevel` export from `@sentry/types` ([#4254](https://github.com/getsentry/sentry-react-native/pull/4254), [#4260](https://github.com/getsentry/sentry-react-native/pull/4260))
- `browserReplayIntegration` should not be included by default ([#4270](https://github.com/getsentry/sentry-react-native/pull/4270))
Copy link
Collaborator

Choose a reason for hiding this comment

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

i: Just a small detail to point that this change only reflects on web projects.

Suggested change
- `browserReplayIntegration` should not be included by default ([#4270](https://github.com/getsentry/sentry-react-native/pull/4270))
- `browserReplayIntegration` is no longer included by default on React Native Web ([#4270](https://github.com/getsentry/sentry-react-native/pull/4270))

lucas-zimerman
lucas-zimerman reviewed Nov 13, 2024
View reviewed changes
packages/core/src/js/integrations/default.ts
(options as BrowserOptions).replaysOnErrorSampleRate = options._experiments.replaysOnErrorSampleRate;
(options as BrowserOptions).replaysSessionSampleRate = options._experiments.replaysSessionSampleRate;
} else {
integrations.push(mobileReplayIntegration());
Copy link
Collaborator

Choose a reason for hiding this comment

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

offtopic to this PR: Should we to filter out browserReplayIntegration by default on mobile projects if users adds this integration?

lucas-zimerman
lucas-zimerman approved these changes Nov 13, 2024
View reviewed changes
Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

Minor request on changelog, after resolving it, LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucas-zimerman lucas-zimerman lucas-zimerman approved these changes

@antonis antonis Awaiting requested review from antonis antonis is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@krystofwoldrich @lucas-zimerman