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 session replay envelope and events #3214

Merged
merged 26 commits into from
Mar 4, 2024

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Feb 20, 2024

#skip-changelog

📜 Description

You can read up more about the new envelope spec here getsentry/rfcs#129

It has a good chunk of work that you've done during the hackweek, but with several enhancements:

  • RRWeb events are now typed
  • Properly capture replay events with recordings in SentryClient and record client reports if e.g. an event is dropped by an event processor
  • Also have a separate method for applying scope as replay events don't have everything as regular events (e.g. no breadcrumbs)
  • I had to introduce a new MapObjectReader and extract an ObjectReader interface from JsonObjectReader but I made a separate PR for this (this one will be failing until the other one is merged)

💡 Motivation and Context

Closes #3213

💚 How did you test it?

With tests

📝 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

Glue recording and envelopes together

@romtsn romtsn changed the title Add session replay envelope and events [SR] Add session replay envelope and events Feb 20, 2024
@romtsn
Copy link
Member Author

romtsn commented Feb 22, 2024

@markushi @stefanosiano I'll wait with merging #3215 otherwise it'll be hell to review this

@romtsn romtsn marked this pull request as ready for review February 22, 2024 14:06
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.

Left a few minor comments, and stopped at some point adding final and @NotNull annotations. But apart from that, LGTM!

@romtsn
Copy link
Member Author

romtsn commented Feb 27, 2024

and stopped at some point adding final and @NotNull annotations

thank you! I will revise that myself and add where necessary

…ject-reader

[SR] Introduce MapObjectReader
Copy link
Contributor

github-actions bot commented Feb 28, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 374.76 ms 457.26 ms 82.50 ms
Size 1.70 MiB 2.28 MiB 591.02 KiB

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

Startup times

Revision Plain With Sentry Diff
6650dc9 424.67 ms 506.40 ms 81.73 ms
8aeeb6b 364.00 ms 434.31 ms 70.31 ms

App size

Revision Plain With Sentry Diff
6650dc9 1.70 MiB 2.28 MiB 591.02 KiB
8aeeb6b 1.70 MiB 2.28 MiB 591.02 KiB

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.

I reviewed only the Session Replay related changes (all the serialization change could have been a different PR), and it looks good.
I wrote just a few comments for discussion.

sentry/src/main/java/io/sentry/Sentry.java Outdated Show resolved Hide resolved
@romtsn romtsn changed the base branch from rz/feat/session-replay-sources to main March 1, 2024 15:24
@romtsn romtsn changed the base branch from main to rz/feat/session-replay-sources March 1, 2024 15:24
Base automatically changed from rz/feat/session-replay-sources to rz/feat/session-replay March 4, 2024 11:47
@romtsn romtsn merged commit 79151e9 into rz/feat/session-replay Mar 4, 2024
19 of 21 checks passed
@romtsn romtsn deleted the rz/feat/session-replay-envelopes branch March 4, 2024 12:19
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