Skip to content

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

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

Merged
merged 4 commits into from
Nov 19, 2024

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

Copy link
Contributor

github-actions bot commented Nov 13, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 392.58 ms 456.35 ms 63.77 ms
Size 7.15 MiB 8.35 MiB 1.20 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
457e29f+dirty 591.49 ms 612.96 ms 21.47 ms
63ed251+dirty 485.02 ms 531.16 ms 46.14 ms
ac41368+dirty 395.91 ms 451.17 ms 55.26 ms
1d86dd6+dirty 335.76 ms 371.22 ms 35.46 ms
575f9da+dirty 337.15 ms 370.47 ms 33.32 ms
e540498+dirty 408.56 ms 480.00 ms 71.44 ms
4a6664f+dirty 357.02 ms 394.91 ms 37.89 ms
9cd0e9f+dirty 383.65 ms 418.65 ms 35.00 ms
e1ea4a8+dirty 451.98 ms 497.58 ms 45.60 ms
b1e8712+dirty 322.55 ms 331.84 ms 9.29 ms

App size

Revision Plain With Sentry Diff
457e29f+dirty 7.15 MiB 8.10 MiB 981.29 KiB
63ed251+dirty 7.15 MiB 8.35 MiB 1.20 MiB
ac41368+dirty 7.15 MiB 8.39 MiB 1.24 MiB
1d86dd6+dirty 7.15 MiB 8.13 MiB 1002.18 KiB
575f9da+dirty 7.15 MiB 8.10 MiB 979.68 KiB
e540498+dirty 7.15 MiB 8.38 MiB 1.23 MiB
4a6664f+dirty 7.15 MiB 8.22 MiB 1.07 MiB
9cd0e9f+dirty 7.15 MiB 8.35 MiB 1.20 MiB
e1ea4a8+dirty 7.15 MiB 8.35 MiB 1.20 MiB
b1e8712+dirty 7.15 MiB 8.04 MiB 912.27 KiB

Copy link
Contributor

github-actions bot commented Nov 13, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1236.94 ms 1241.85 ms 4.91 ms
Size 2.36 MiB 3.10 MiB 753.33 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
fe13591+dirty 1208.25 ms 1219.53 ms 11.28 ms
d2c32bb+dirty 1223.69 ms 1229.49 ms 5.80 ms
c2a4e9b+dirty 1240.10 ms 1239.22 ms -0.88 ms
ad6c299+dirty 1244.76 ms 1260.10 ms 15.34 ms
eb1e19f+dirty 1209.56 ms 1214.94 ms 5.38 ms
b1e8712+dirty 1256.02 ms 1265.14 ms 9.12 ms
1faf8e3+dirty 1214.87 ms 1222.83 ms 7.97 ms
6e8584e+dirty 1274.50 ms 1296.82 ms 22.32 ms
52a8031+dirty 1280.88 ms 1289.78 ms 8.90 ms
4cc5c27+dirty 1211.45 ms 1214.60 ms 3.16 ms

App size

Revision Plain With Sentry Diff
fe13591+dirty 2.36 MiB 3.10 MiB 752.40 KiB
d2c32bb+dirty 2.36 MiB 3.08 MiB 737.22 KiB
c2a4e9b+dirty 2.36 MiB 3.08 MiB 734.00 KiB
ad6c299+dirty 2.36 MiB 2.84 MiB 488.85 KiB
eb1e19f+dirty 2.36 MiB 3.08 MiB 737.21 KiB
b1e8712+dirty 2.36 MiB 2.84 MiB 488.84 KiB
1faf8e3+dirty 2.36 MiB 3.08 MiB 736.75 KiB
6e8584e+dirty 2.36 MiB 2.88 MiB 533.17 KiB
52a8031+dirty 2.36 MiB 2.82 MiB 469.44 KiB
4cc5c27+dirty 2.36 MiB 3.04 MiB 698.52 KiB

Copy link
Contributor

github-actions bot commented Nov 13, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.65 ms 1231.50 ms -0.15 ms
Size 2.92 MiB 3.66 MiB 758.54 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
fe13591+dirty 1250.69 ms 1246.27 ms -4.43 ms
d2c32bb+dirty 1244.00 ms 1245.77 ms 1.77 ms
c2a4e9b+dirty 1247.39 ms 1243.04 ms -4.35 ms
ad6c299+dirty 1248.50 ms 1248.88 ms 0.38 ms
eb1e19f+dirty 1229.91 ms 1231.63 ms 1.71 ms
b1e8712+dirty 1284.11 ms 1297.82 ms 13.71 ms
1faf8e3+dirty 1223.38 ms 1220.56 ms -2.82 ms
6e8584e+dirty 1271.71 ms 1281.26 ms 9.55 ms
52a8031+dirty 1255.96 ms 1273.00 ms 17.04 ms
4cc5c27+dirty 1220.43 ms 1215.13 ms -5.30 ms

App size

Revision Plain With Sentry Diff
fe13591+dirty 2.92 MiB 3.66 MiB 757.71 KiB
d2c32bb+dirty 2.92 MiB 3.64 MiB 742.84 KiB
c2a4e9b+dirty 2.92 MiB 3.64 MiB 739.91 KiB
ad6c299+dirty 2.92 MiB 3.40 MiB 494.12 KiB
eb1e19f+dirty 2.92 MiB 3.64 MiB 742.82 KiB
b1e8712+dirty 2.92 MiB 3.40 MiB 494.15 KiB
1faf8e3+dirty 2.92 MiB 3.64 MiB 742.61 KiB
6e8584e+dirty 2.92 MiB 3.44 MiB 536.52 KiB
52a8031+dirty 2.92 MiB 3.38 MiB 475.71 KiB
4cc5c27+dirty 2.92 MiB 3.61 MiB 705.47 KiB

(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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's unpredictable behavior we should avoid, if they added it we should respect it.

Sidenote: The integration check if it can run, it not it will gracefully disables itself.

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!

@krystofwoldrich krystofwoldrich merged commit 37ab937 into main Nov 19, 2024
55 of 56 checks passed
@krystofwoldrich krystofwoldrich deleted the kw/fix/replay-web branch November 19, 2024 11:51
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.

2 participants