Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants