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

feat(replay): Add Mobile Replay Alpha #3714

Merged
merged 47 commits into from
May 21, 2024
Merged

feat(replay): Add Mobile Replay Alpha #3714

merged 47 commits into from
May 21, 2024

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Mar 25, 2024 •
edited

📢 Type of change

  • New feature

Todo before merge

  • Release sentry-android stable version with experimental implementation of Replay (or merge with the alpha release to a clean feature branch which will be later merged to main)
  • Add tests for MobileReplay options are passed to native init
  • Add tests for SDK options enabling the Replay intergation
  • Add tests for native options mapping (iOS and Android)
  • Enable buffered mode in E2E tests (if possible)
  • Review

Done

  • Android doesn't automatically pick up the Root View fixed by getsentry/sentry-java@bee240b
  • Test redacting RN Images on Android -> works
  • iOS Swift implementation doesn't compile with the RN SDK fixed by getsentry/sentry-cocoa@1afd0d9
  • Android uses sample rate options
  • Android add replay start for buffered mode
  • iOS Redact RN text elements and images
  • iOS add replay start for buffered mode
  • iOS expose replayId on the Scope (or anywhere else accessible by the Hybrid SDKs)
  • Add _experimental option
  • Add maskAll bool flag

Done for alpha.0 release

📝 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

#skip-changelog

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 25, 2024 •
edited

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.00 ms 1223.49 ms 1.49 ms
Size 2.36 MiB 2.92 MiB 571.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0db0c72+dirty 1275.02 ms 1285.84 ms 10.82 ms
9433f35+dirty 1246.94 ms 1271.45 ms 24.52 ms
76d1baf+dirty 1244.10 ms 1268.52 ms 24.42 ms
e2b64fe+dirty 1232.22 ms 1255.20 ms 22.98 ms
8900e1a+dirty 1210.27 ms 1218.66 ms 8.39 ms
e73f4ed+dirty 1243.27 ms 1244.52 ms 1.25 ms
3853f43+dirty 1221.82 ms 1242.64 ms 20.82 ms
27ef4ee+dirty 1293.52 ms 1296.08 ms 2.56 ms
d7401ac+dirty 1252.38 ms 1275.04 ms 22.66 ms
22e31b6+dirty 1253.62 ms 1265.96 ms 12.34 ms

App size

Revision Plain With Sentry Diff
0db0c72+dirty 2.36 MiB 2.84 MiB 487.01 KiB
9433f35+dirty 2.36 MiB 2.85 MiB 499.80 KiB
76d1baf+dirty 2.36 MiB 2.82 MiB 469.45 KiB
e2b64fe+dirty 2.36 MiB 2.85 MiB 495.80 KiB
8900e1a+dirty 2.36 MiB 2.83 MiB 479.25 KiB
e73f4ed+dirty 2.36 MiB 2.82 MiB 469.44 KiB
3853f43+dirty 2.36 MiB 2.85 MiB 499.81 KiB
27ef4ee+dirty 2.36 MiB 2.85 MiB 500.03 KiB
d7401ac+dirty 2.36 MiB 2.83 MiB 481.14 KiB
22e31b6+dirty 2.36 MiB 2.87 MiB 520.67 KiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 25, 2024 •
edited

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1218.39 ms 1219.85 ms 1.47 ms
Size 2.92 MiB 3.48 MiB 577.47 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0db0c72+dirty 1258.88 ms 1262.52 ms 3.64 ms
9433f35+dirty 1232.24 ms 1232.74 ms 0.50 ms
76d1baf+dirty 1245.00 ms 1257.76 ms 12.76 ms
e2b64fe+dirty 1285.78 ms 1297.56 ms 11.78 ms
8900e1a+dirty 1268.36 ms 1273.04 ms 4.68 ms
e73f4ed+dirty 1282.90 ms 1309.30 ms 26.40 ms
3853f43+dirty 1271.74 ms 1278.04 ms 6.30 ms
27ef4ee+dirty 1236.41 ms 1244.90 ms 8.49 ms
d7401ac+dirty 1288.10 ms 1289.54 ms 1.44 ms
22e31b6+dirty 1276.55 ms 1278.12 ms 1.57 ms

App size

Revision Plain With Sentry Diff
0db0c72+dirty 2.92 MiB 3.40 MiB 492.71 KiB
9433f35+dirty 2.92 MiB 3.41 MiB 503.55 KiB
76d1baf+dirty 2.92 MiB 3.38 MiB 475.74 KiB
e2b64fe+dirty 2.92 MiB 3.41 MiB 499.97 KiB
8900e1a+dirty 2.92 MiB 3.39 MiB 485.96 KiB
e73f4ed+dirty 2.92 MiB 3.38 MiB 475.71 KiB
3853f43+dirty 2.92 MiB 3.41 MiB 503.54 KiB
27ef4ee+dirty 2.92 MiB 3.41 MiB 503.72 KiB
d7401ac+dirty 2.92 MiB 3.40 MiB 488.06 KiB
22e31b6+dirty 2.92 MiB 3.43 MiB 524.74 KiB

@krystofwoldrich krystofwoldrich changed the title wip: Add Mobile Replay Add Mobile Replay Mar 25, 2024
@krystofwoldrich krystofwoldrich mentioned this pull request Apr 23, 2024
Merged
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 23, 2024 •
edited

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 426.92 ms 452.41 ms 25.49 ms
Size 17.73 MiB 20.02 MiB 2.29 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 23, 2024 •
edited

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 312.93 ms 354.21 ms 41.28 ms
Size 7.15 MiB 8.30 MiB 1.15 MiB

@krystofwoldrich krystofwoldrich mentioned this pull request Apr 23, 2024
Merged
This was referenced Apr 24, 2024
Merged
Merged
@krystofwoldrich krystofwoldrich marked this pull request as draft April 30, 2024 09:25
@krystofwoldrich krystofwoldrich changed the title feat(replay): Add Mobile Replay Alpha.0 feat(replay): Add Mobile Replay Alpha May 9, 2024
@getsentry getsentry deleted a comment from github-actions bot May 9, 2024
@lucas-zimerman
Copy link
Collaborator

Should we bump feat/replay with main? There seems to have changes on kw-add-replay-v0 that are from the main branch but are missing on feat/replay, that could help cleaning the PR file changes by only pointing to the changes that were actually done on the PR

@vaind
Copy link
Collaborator

vaind commented May 15, 2024 •
edited

Is it possible to review this and get merged to feat/replay soonish? I'd rather not create a third PR in chain to land #3818

TODOs from this PR could move to feat/replay PR (which should only have reviewed code AFAIU) or a tracking issue.

@getsentry getsentry deleted a comment from github-actions bot May 15, 2024
@krystofwoldrich
Copy link
Member Author

@lucas-zimerman @vaind @romtsn @brustolin Can, you take a look at this alpha PR? I would like to merge it as is to feat/replay where only review commits will go and then that will be eventually be merged without squash to main.

@billyvg @mydea Feel free to comment on the JS part, so RN doesn't unnecessarily deviate from browser replay.

@krystofwoldrich krystofwoldrich marked this pull request as ready for review May 15, 2024 16:26
vaind
vaind reviewed May 15, 2024
View reviewed changes
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

LGTM but leaving approval to someone who knows the SDK.

brustolin
brustolin reviewed May 17, 2024
View reviewed changes
RNSentry.podspec
@@ -33,7 +33,7 @@ Pod::Spec.new do |s|
s.preserve_paths = '*.js'

s.dependency 'React-Core'
s.dependency 'Sentry/HybridSDK', '8.25.2'
s.dependency 'Sentry/HybridSDK', '8.25.0-alpha.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

We can target 8.26.0 now

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll do the bump.

lucas-zimerman
lucas-zimerman approved these changes May 17, 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.

There are some comments that I made but nothing blocking so I am approving it for merge 🚀

@krystofwoldrich krystofwoldrich merged commit 60d07df into feat/replay May 21, 2024
48 of 52 checks passed
@krystofwoldrich krystofwoldrich deleted the kw-add-replay-v0 branch May 21, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaind vaind vaind left review comments

@brustolin brustolin brustolin left review comments

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

@mydea mydea Awaiting requested review from mydea

@romtsn romtsn Awaiting requested review from romtsn

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@krystofwoldrich @lucas-zimerman @vaind @brustolin @getsentry-bot