-
-
Notifications
You must be signed in to change notification settings - Fork 355
Add caching to getReplayId
#5449
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
Conversation
|
| debug.log( | ||
| `[Sentry] ${MOBILE_REPLAY_INTEGRATION_NAME} Captured recording replay ${replayId} for event ${event.event_id}.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucas-zimerman I wonder if it's sufficient. In this case, updateCachedReplayId is being called only when processEvent is called, and I think it makes sense — we call NATIVE.captureReplay there, get the ID, and update it in our "cache".
BUT is it possible for replay ID to be updated outside of this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function getCachedReplayId(): string | null { | ||
| if (cachedReplayId !== null) { | ||
| return cachedReplayId; | ||
| } | ||
| const nativeReplayId = NATIVE.getCurrentReplayId(); | ||
| if (nativeReplayId) { | ||
| cachedReplayId = nativeReplayId; | ||
| } | ||
| return nativeReplayId; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 459a438+dirty | 359.50 ms | 390.53 ms | 31.03 ms |
| 95aaf8a+dirty | 342.82 ms | 393.75 ms | 50.93 ms |
| 55b77fc+dirty | 410.46 ms | 414.11 ms | 3.65 ms |
| ad27f6e+dirty | 484.67 ms | 532.79 ms | 48.12 ms |
| 7480abe+dirty | 363.80 ms | 431.34 ms | 67.54 ms |
| 2f9fb30+dirty | 383.06 ms | 404.56 ms | 21.50 ms |
| 6fee48d+dirty | 370.23 ms | 427.86 ms | 57.63 ms |
| 46e3d54+dirty | 436.24 ms | 474.18 ms | 37.94 ms |
| e76d0d3+dirty | 358.22 ms | 378.65 ms | 20.43 ms |
| 93137d1+dirty | 367.58 ms | 434.94 ms | 67.36 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 459a438+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 95aaf8a+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 55b77fc+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| ad27f6e+dirty | 43.94 MiB | 48.90 MiB | 4.96 MiB |
| 7480abe+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 2f9fb30+dirty | 43.94 MiB | 48.87 MiB | 4.93 MiB |
| 6fee48d+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 46e3d54+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| e76d0d3+dirty | 7.15 MiB | 8.44 MiB | 1.28 MiB |
| 93137d1+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 55b77fc+dirty | 411.87 ms | 417.16 ms | 5.29 ms |
| 3312430+dirty | 362.54 ms | 368.76 ms | 6.22 ms |
| 955f2eb+dirty | 422.74 ms | 410.19 ms | -12.55 ms |
| 1e7a472+dirty | 348.80 ms | 362.55 ms | 13.75 ms |
| ad27f6e+dirty | 471.44 ms | 516.23 ms | 44.79 ms |
| 2f9fb30+dirty | 416.78 ms | 444.94 ms | 28.16 ms |
| 3bd3f0d+dirty | 447.21 ms | 472.31 ms | 25.10 ms |
| 46e3d54+dirty | 467.76 ms | 487.53 ms | 19.78 ms |
| 170d5ea+dirty | 407.92 ms | 422.49 ms | 14.57 ms |
| 49ef936+dirty | 405.96 ms | 417.22 ms | 11.26 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 55b77fc+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| 3312430+dirty | 17.75 MiB | 19.75 MiB | 2.00 MiB |
| 955f2eb+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 1e7a472+dirty | 17.75 MiB | 19.70 MiB | 1.96 MiB |
| ad27f6e+dirty | 43.75 MiB | 48.07 MiB | 4.32 MiB |
| 2f9fb30+dirty | 43.75 MiB | 48.05 MiB | 4.29 MiB |
| 3bd3f0d+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 46e3d54+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| 170d5ea+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 49ef936+dirty | 17.75 MiB | 19.69 MiB | 1.94 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 1213.17 ms | 1216.33 ms | 3.17 ms |
| a31630c+dirty | 1241.32 ms | 1226.98 ms | -14.34 ms |
| ff5a06a+dirty | 1205.63 ms | 1204.73 ms | -0.90 ms |
| eb07ba3+dirty | 1214.49 ms | 1221.59 ms | 7.10 ms |
| 3401245+dirty | 1216.70 ms | 1241.44 ms | 24.74 ms |
| 3e0a5f9+dirty | 1233.65 ms | 1239.10 ms | 5.45 ms |
| 294387d+dirty | 1199.23 ms | 1204.16 ms | 4.93 ms |
| 69602ce+dirty | 1230.59 ms | 1230.84 ms | 0.24 ms |
| 5526494+dirty | 1217.06 ms | 1222.26 ms | 5.20 ms |
| 276d348+dirty | 1222.10 ms | 1229.02 ms | 6.92 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| a31630c+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| ff5a06a+dirty | 3.41 MiB | 4.59 MiB | 1.18 MiB |
| eb07ba3+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 3401245+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 3e0a5f9+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 294387d+dirty | 3.41 MiB | 4.59 MiB | 1.18 MiB |
| 69602ce+dirty | 3.19 MiB | 4.48 MiB | 1.29 MiB |
| 5526494+dirty | 3.19 MiB | 4.44 MiB | 1.25 MiB |
| 276d348+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 1216.40 ms | 1210.47 ms | -5.93 ms |
| a31630c+dirty | 1229.09 ms | 1230.94 ms | 1.85 ms |
| ff5a06a+dirty | 1208.85 ms | 1206.29 ms | -2.56 ms |
| eb07ba3+dirty | 1222.46 ms | 1220.37 ms | -2.08 ms |
| 3401245+dirty | 1222.60 ms | 1223.06 ms | 0.46 ms |
| 3e0a5f9+dirty | 1226.94 ms | 1230.02 ms | 3.08 ms |
| 294387d+dirty | 1197.73 ms | 1208.35 ms | 10.61 ms |
| 69602ce+dirty | 1235.65 ms | 1230.82 ms | -4.83 ms |
| 5526494+dirty | 1224.73 ms | 1229.08 ms | 4.36 ms |
| 276d348+dirty | 1224.22 ms | 1227.38 ms | 3.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46e3d54+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| a31630c+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| ff5a06a+dirty | 3.41 MiB | 4.59 MiB | 1.18 MiB |
| eb07ba3+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 3401245+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 3e0a5f9+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 294387d+dirty | 3.41 MiB | 4.59 MiB | 1.18 MiB |
| 69602ce+dirty | 2.63 MiB | 3.91 MiB | 1.28 MiB |
| 5526494+dirty | 2.63 MiB | 3.87 MiB | 1.24 MiB |
| 276d348+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
lucas-zimerman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good for me, Thank you for the PR!
antonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and worked as expected in my tests 🎸
Thank you for this improvement @alwx 🙇
We can iterate later with better caching if this feasible from the native side.
📢 Type of change
📜 Description
Improves performance by introducing caching to
getReplayIdinmobileReplayIntegration💡 Motivation and Context
Fixes #5376
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps