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.

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

Merged
merged 19 commits into from
Apr 3, 2024

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Mar 19, 2024

#skip-changelog

📜 Description

  • Adds ReplayIntegration which takes care of persisting screenshots, making a video segment out of them and sending them as envelope
  • Wires up sentry-android-replay module with sentry-android-core through compileOnly dependency and runtime checks
    • also hijacks the current session lifecycle logic to start/stop session replay
  • Adds replayId to Scope, which later we should send in the envelope header to connect with errors and performance

💡 Motivation and Context

Part of getsentry/sentry#63255

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

  • add tests
  • add replay options
  • add buffer mode (to capture 30s before an error happened)

Copy link
Contributor

github-actions bot commented Mar 19, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 373.83 ms 473.80 ms 99.97 ms
Size 1.70 MiB 2.30 MiB 614.86 KiB

Previous results on branch: rz/feat/session-replay-integration

Startup times

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

@romtsn romtsn marked this pull request as ready for review March 22, 2024 08:57
@romtsn
Copy link
Member Author

romtsn commented Mar 22, 2024

marking as ready for review - I'll add remaining tests before merging into main as a separate PR

@romtsn
Copy link
Member Author

romtsn commented Mar 28, 2024

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

Copy link
Member

@markushi markushi left a 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.

}

fun pause() {
val now = dateProvider.currentTimeMillis
Copy link
Member

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

Copy link
Member Author

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 👍

Copy link
Member Author

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 👍

@romtsn romtsn merged commit b04aaf2 into rz/feat/session-replay Apr 3, 2024
20 of 21 checks passed
@romtsn romtsn deleted the rz/feat/session-replay-integration branch April 3, 2024 14:53
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.

3 participants