-
-
Notifications
You must be signed in to change notification settings - Fork 445
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
[SR] Add replay integration #3272
[SR] Add replay integration #3272
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
deb27b5 | 411.48 ms | 474.42 ms | 62.94 ms |
2bc4ec3 | 373.40 ms | 450.78 ms | 77.38 ms |
f2b8416 | 396.36 ms | 477.59 ms | 81.23 ms |
14060c4 | 401.53 ms | 465.96 ms | 64.43 ms |
795d513 | 380.51 ms | 445.06 ms | 64.55 ms |
2dc138c | 386.16 ms | 467.74 ms | 81.58 ms |
223546c | 326.56 ms | 379.68 ms | 53.12 ms |
d7ec9af | 353.02 ms | 402.46 ms | 49.44 ms |
fac4ae4 | 432.94 ms | 516.76 ms | 83.82 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
deb27b5 | 1.70 MiB | 2.30 MiB | 614.54 KiB |
2bc4ec3 | 1.70 MiB | 2.30 MiB | 614.84 KiB |
f2b8416 | 1.70 MiB | 2.30 MiB | 614.88 KiB |
14060c4 | 1.70 MiB | 2.30 MiB | 613.83 KiB |
795d513 | 1.70 MiB | 2.30 MiB | 614.40 KiB |
2dc138c | 1.70 MiB | 2.30 MiB | 614.27 KiB |
223546c | 1.70 MiB | 2.30 MiB | 614.27 KiB |
d7ec9af | 1.70 MiB | 2.30 MiB | 614.85 KiB |
fac4ae4 | 1.70 MiB | 2.30 MiB | 614.77 KiB |
marking as ready for review - I'll add remaining tests before merging into main as a separate PR |
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
Outdated
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
Show resolved
Hide resolved
integration tests are failing probably due to a running replay recorder, I guess I'll disable it in the following PR where we introduced sample rates |
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.
About halfway through, looking good, no blockers so far. Left a few comments which can be addressed as future improvements PRs as well.
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java
Show resolved
Hide resolved
sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
Outdated
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayCache.kt
Outdated
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
Outdated
Show resolved
Hide resolved
sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
Show resolved
Hide resolved
} | ||
|
||
fun pause() { | ||
val now = dateProvider.currentTimeMillis |
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.
I guess we need to align the date provider usage to have the same source of truth, e.g. DateUtils.getCurrentDateTime()
vs . DateUtils.getCurrentDateTime()
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.
DateUtils.getCurrentDateTime() vs . DateUtils.getCurrentDateTime()
you mean dateProvider.currentTimeMillis vs DateUtils.getCurrentDateTime()
right?
I'd love to align them, but the thing is that rrweb expects timestamp as long
in milliseconds, and relay expects Event timestamp in ISO format, hence the divergence. I'll leave a TODO here though, to explore if we can align those 👍
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.
yeah for sure I'll change that to be able to test it, will do that later when adding tests 👍
#skip-changelog
📜 Description
ReplayIntegration
which takes care of persisting screenshots, making a video segment out of them and sending them as envelope💡 Motivation and Context
Part of getsentry/sentry#63255
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps