Skip to content

Comments

[SR] Add replay integration#3272

Merged
romtsn merged 19 commits intorz/feat/session-replayfrom
rz/feat/session-replay-integration
Apr 3, 2024
Merged

[SR] Add replay integration#3272
romtsn merged 19 commits intorz/feat/session-replayfrom
rz/feat/session-replay-integration

Conversation

@romtsn
Copy link
Member

@romtsn romtsn commented Mar 19, 2024 •
edited
Loading

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 19, 2024 •
edited
Loading

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 mentioned this pull request Mar 19, 2024
Closed
@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

@krystofwoldrich krystofwoldrich mentioned this pull request Mar 25, 2024
Merged
29 tasks
vaind
vaind reviewed Mar 26, 2024
View reviewed changes
@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

markushi
markushi reviewed Mar 28, 2024
View reviewed changes
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.

sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
}

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

@markushi markushi Mar 28, 2024

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

@romtsn romtsn Apr 2, 2024

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

@romtsn romtsn Apr 2, 2024

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 👍

markushi
markushi reviewed Apr 2, 2024
View reviewed changes
@romtsn romtsn merged commit b04aaf2 into rz/feat/session-replay Apr 3, 2024
@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

Reviewers

@vaind vaind vaind left review comments

@markushi markushi markushi approved these changes

@adinauer adinauer Awaiting requested review from adinauer

@stefanosiano stefanosiano Awaiting requested review from stefanosiano

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@romtsn @markushi @vaind