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 screenshot recorder #3203

Merged
merged 15 commits into from
Mar 4, 2024

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Feb 13, 2024

#skip-changelog

This is just a dump of the PoC phase more-or-less, so a lot will be refactored and made pretty, but you're welcome to review nonetheless. I've tried to left TODOs where I see changes forthcoming

Relates to:

Copy link
Contributor

github-actions bot commented Feb 13, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 392.73 ms 459.08 ms 66.35 ms
Size 1.70 MiB 2.27 MiB 584.69 KiB

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

Startup times

Revision Plain With Sentry Diff
acb8705 340.75 ms 368.17 ms 27.42 ms
6d9e721 383.10 ms 458.98 ms 75.88 ms
268db42 415.38 ms 481.70 ms 66.31 ms
4ffa796 382.90 ms 445.32 ms 62.42 ms
007a5ae 383.88 ms 455.89 ms 72.02 ms
6c0fe29 381.20 ms 444.18 ms 62.98 ms

App size

Revision Plain With Sentry Diff
acb8705 1.70 MiB 2.27 MiB 584.69 KiB
6d9e721 1.70 MiB 2.27 MiB 584.69 KiB
268db42 1.70 MiB 2.27 MiB 584.69 KiB
4ffa796 1.70 MiB 2.27 MiB 584.69 KiB
007a5ae 1.70 MiB 2.27 MiB 584.69 KiB
6c0fe29 1.70 MiB 2.27 MiB 584.69 KiB

@romtsn romtsn requested a review from vaind February 14, 2024 10:30
Copy link

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Looks great.

Some comments to discuss.

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.

Not much to review for me here, because, as you're saying, "This is just a dump of the PoC".

I'd be interested to see the refactored version that would be usable from outside sentry-java

@romtsn
Copy link
Member Author

romtsn commented Feb 16, 2024

Not much to review for me here, because, as you're saying, "This is just a dump of the PoC".

I'd be interested to see the refactored version that would be usable from outside sentry-java

You got to tell me what do you want to have exposed then :)

@vaind
Copy link
Collaborator

vaind commented Feb 16, 2024

You got to tell me what do you want to have exposed then :)

I imagine the code that captures images is going to be separate from the code that produces the video, at least from your description how it's going to be done in two steps in order to avoid loses during crashes.
In order to use that from another SDK, we'd need an interface that allows constructing this envelope, i.e. creating the video (from bitmaps), adding replay events ans such.
I assume sending the envelope is no different than any other envelope so that should be already done.

@romtsn
Copy link
Member Author

romtsn commented Feb 16, 2024

i.e. creating the video (from bitmaps)

Yeah, this makes sense, I'll create a new method in SimpleVideoEncoder that creates a video out of a bitmap list. But this:

we'd need an interface that allows constructing this envelope

I'm not sure. Don't we usually do that on the hybrid SDK level and then just pass the envelope bytes to captureEnvelope? https://github.com/getsentry/sentry-dart/blob/732a7b44ad1624e75967be2227665c066ef0154e/dart/lib/src/sentry_envelope.dart

@vaind
Copy link
Collaborator

vaind commented Feb 16, 2024

i.e. creating the video (from bitmaps)

Yeah, this makes sense, I'll create a new method in SimpleVideoEncoder that creates a video out of a bitmap list. But this:

we'd need an interface that allows constructing this envelope

I'm not sure. Don't we usually do that on the hybrid SDK level and then just pass the envelope bytes to captureEnvelope? https://github.com/getsentry/sentry-dart/blob/732a7b44ad1624e75967be2227665c066ef0154e/dart/lib/src/sentry_envelope.dart

In this case we won't be creating the envelope in dart due to the cost of transferring the data. Instead, sentry-dart has a native kotlin binding that would call native functions to create and send the envelope.

@romtsn
Copy link
Member Author

romtsn commented Feb 16, 2024

i.e. creating the video (from bitmaps)

Yeah, this makes sense, I'll create a new method in SimpleVideoEncoder that creates a video out of a bitmap list. But this:

we'd need an interface that allows constructing this envelope

I'm not sure. Don't we usually do that on the hybrid SDK level and then just pass the envelope bytes to captureEnvelope? getsentry/sentry-dart@732a7b4/dart/lib/src/sentry_envelope.dart

In this case we won't be creating the envelope in dart due to the cost of transferring the data. Instead, sentry-dart has a native kotlin binding that would call native functions to create and send the envelope.

Alright, let me give it a stab for the Android SDK first and then we can discuss in the following PR what has to be changed and so on. I imagine this is all the glue code I'm gonna write to connect recording and envelopes that is the most valuable for you.

@romtsn romtsn changed the title Add screenshot recorder [SR] Add screenshot recorder Feb 20, 2024
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.

Looking good overall! Left a few comments. We probably can improve on caching / handling bitmap allocation as they seem to happen in multiple places.

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.

Nice one, looks great!

@romtsn romtsn merged commit 64f70c5 into rz/feat/session-replay Mar 4, 2024
21 checks passed
@romtsn romtsn deleted the rz/feat/session-replay-sources branch March 4, 2024 11:47
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.

6 participants