feat(Session Replay): ReplayEvent, ReplayRecording and Envelope handling by brustolin · Pull Request #3638 · getsentry/sentry-cocoa

brustolin · 2024-02-14T12:30:57Z

Added SentryReplayEven, SentryReplayRecording and Envelope handling for this new types.

#skip-changelog

Relates to:

[Epic] Mobile Replay: Private Alpha Release sentry#63255

github-actions · 2024-02-14T12:31:22Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 53b3089

github-actions · 2024-02-14T12:40:58Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1203.16 ms	1216.26 ms	13.09 ms
Size	21.58 KiB	429.58 KiB	408.00 KiB

Previous results on branch: feat(SR)/ReplayEvent

Startup times

Revision	Plain	With Sentry	Diff
`6381b5a`	1239.04 ms	1243.80 ms	4.76 ms
`20a0fe7`	1204.59 ms	1211.98 ms	7.39 ms
`91877f9`	1196.71 ms	1212.69 ms	15.98 ms
`90ce64a`	1204.37 ms	1212.98 ms	8.61 ms
`39743bf`	1234.49 ms	1258.06 ms	23.57 ms

App size

Revision	Plain	With Sentry	Diff
`6381b5a`	21.58 KiB	423.65 KiB	402.06 KiB
`20a0fe7`	21.58 KiB	429.51 KiB	407.92 KiB
`91877f9`	21.58 KiB	422.76 KiB	401.17 KiB
`90ce64a`	21.58 KiB	424.51 KiB	402.93 KiB
`39743bf`	21.58 KiB	429.46 KiB	407.88 KiB

Sources/Sentry/SentryClient.m

Sources/Sentry/SentryDateUtil.m

Sources/Sentry/SentryReplayEvent.m

Sources/Sentry/SentryReplayRecording.m

Sources/Sentry/include/SentryReplayRecording.h

philipphofmann

That looks great. I'm excited to see session reply land on Cocoa soonish 😃. A few points to consider:

Some new classes miss NS_ASSUME_NONNULL_BEGIN and NS_ASSUME_NONNULL_END. Please add them. I think we should write a linter for that.
You could consider writing classes without dependencies in Swift.
I think you could set the base of this PR to main, so we include it sooner in the code base. Less potential for merge conflicts. I'm OK with the tradeoff that there is a bit of code that isn't used yet in there.

Sources/Sentry/include/HybridPublic/SentryEnvelopeItemType.h

Sources/Sentry/include/SentrySerialization.h

Sources/Sentry/SentryReplayEvent.m

Tests/SentryTests/Integrations/SessionReplay/SentryReplayEnvelopeItemHeaderTests.swift

Sources/Sentry/SentryEnvelope.m

Sources/Sentry/SentryClient.m

Sources/Sentry/SentryReplayRecording.m

Sources/Sentry/include/SentryReplayRecording.h

Sources/Sentry/SentryReplayEnvelopeItemHeader.m

vaind

LGTM so far

…coa into feat(SR)/ReplayEvent

philipphofmann

A few comments

Sources/Sentry/include/SentryDataCategory.h

Sources/Sentry/SentryDataCategoryMapper.m

Sources/Sentry/include/SentryMsgPackSerializer.h

philipphofmann · 2024-02-20T10:59:00Z

Tests/SentryTests/SentryMsgPackSerializerTests.m

+
+@end
+
+@implementation SentryMsgPackSerializerTests


m: I think it would make sense to add some sample files in the message pack format and use them for extra tests to ensure our serialization works properly.

We do have tests with files

No, I mean using some predefined dictionary with a couple of different values, serializing it with another message pack library, store the output to a file, add this file to our tests, and then comparing the results of the predefined dictionary with the stored file.

Although the reading works in another message pack library (I tested it). The writing is different since I didn't optimise as explained in the SentryMsgPackSerializer.m line 35

// We will always use the 4 bytes data length for simplicity. // Worst case we're losing 3 bytes.

As long we dont change the tests in this file that check for format, there is nothing to worry about.

Then maybe add a snapshot. Take some sample data, serialize it with SentryMsgPackSerializer, validate the output against another message pack library, store the contents to a file, and use the file in the tests.

Sources/Sentry/SentryMsgPackSerializer.m

Sources/Sentry/SentryEnvelope.m

Sources/Sentry/SentryClient.m

Sources/Sentry/SentryMsgPackSerializer.m

philipphofmann · 2024-02-20T16:18:47Z

Tests/SentryTests/SentryMsgPackSerializerTests.m

+
+@end
+
+@implementation SentryMsgPackSerializerTests


No, I mean using some predefined dictionary with a couple of different values, serializing it with another message pack library, store the output to a file, add this file to our tests, and then comparing the results of the predefined dictionary with the stored file.

Sources/Sentry/include/SentryDataCategory.h

Co-authored-by: Philipp Hofmann <[email protected]>

romtsn · 2024-02-22T12:27:34Z

Sources/Sentry/SentryClient.m

+    }
+
+    // breadcrumbs for replay will be send with ReplayRecording
+    replayEvent.breadcrumbs = nil;


we probably also got to nullify other stuff that replays do not support currently:

replayEvent.extra = nil; replayEvent.threads = nil; replayEvent.debugMeta = nil;

and also this part which will be executed for the replay event I believe, but it shouldn't:

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 557 to 562 in 7b5ebd9

// Transactions have their own sampleRate

if (eventIsNotATransaction && [self isSampled:self.options.sampleRate]) {

SENTRY_LOG_DEBUG(@"Event got sampled, will not send the event");

[self recordLostEvent:kSentryDataCategoryError reason:kSentryDiscardReasonSampleRate];

return nil;

}

Good catch. I will fix this.

I believe it's best to open a new PR to address prepareEvent rather than further inflating this PR

sure! just pointed it out so we don't miss that

Sources/Sentry/SentryEnvelope.m

romtsn

LGTM 🎉

philipphofmann

LGTM

Sources/Sentry/include/SentryMsgPackSerializer.h

brustolin added 2 commits February 12, 2024 11:25

Merge branch 'feat/session-replay' into feat(SR)/ReplayEvent

663199a

feat(Session Replay): ReplayEvent, ReplayRecording and Envelope handling

eae7f93

brustolin requested review from bruno-garcia and romtsn February 14, 2024 12:30

brustolin requested review from armcknight and philipphofmann as code owners February 14, 2024 12:30

brustolin changed the title ~~Feat(sr)/replay event~~ feat(Session Replay): ReplayEvent, ReplayRecording and Envelope handling Feb 14, 2024

Merge branch 'main' into feat(SR)/ReplayEvent

7f7be69

Merge branch 'feat/session-replay' into feat(SR)/ReplayEvent

37d92b3

bruno-garcia reviewed Feb 14, 2024

View reviewed changes

Sources/Sentry/SentryClient.m Show resolved Hide resolved

Sources/Sentry/SentryDateUtil.m Outdated Show resolved Hide resolved

Sources/Sentry/SentryReplayEvent.m Outdated Show resolved Hide resolved

Sources/Sentry/SentryReplayRecording.m Outdated Show resolved Hide resolved

brustolin requested a review from vaind February 15, 2024 08:30

refactoring

9e21c1e

romtsn reviewed Feb 15, 2024

View reviewed changes

Sources/Sentry/SentryReplayRecording.m Show resolved Hide resolved

romtsn reviewed Feb 15, 2024

View reviewed changes

Sources/Sentry/include/SentryReplayRecording.h Show resolved Hide resolved

philipphofmann reviewed Feb 15, 2024

View reviewed changes

romtsn reviewed Feb 16, 2024

View reviewed changes

Sources/Sentry/SentryReplayEnvelopeItemHeader.m Outdated Show resolved Hide resolved

vaind reviewed Feb 16, 2024

View reviewed changes

brustolin and others added 10 commits February 19, 2024 08:02

MsgPack

723b074

Capture with scope

d27ec15

MsgPack Tests

665bec4

Format code

f42c9e7

Replacing envelope item

398e3d0

Merge branch 'feat(SR)/ReplayEvent' of github.com:getsentry/sentry-co…

ab57929

…coa into feat(SR)/ReplayEvent

more test

33ead44

Format code

6cd821e

Hub test

216c42e

Update SentryHubTests.swift

21efb9a

brustolin requested a review from romtsn February 19, 2024 16:43

brustolin requested review from bruno-garcia and vaind February 19, 2024 16:43

philipphofmann reviewed Feb 20, 2024

View reviewed changes

brustolin and others added 5 commits February 20, 2024 16:08

More tests and log messages

522e972

Format code

522a5da

CategoryMapper tests

4463fed

Update SentryMsgPackSerializer.m

b14f253

Format code

16b192d

philipphofmann reviewed Feb 20, 2024

View reviewed changes

brustolin and others added 2 commits February 21, 2024 14:39

Merge branch 'feat/session-replay' into feat(SR)/ReplayEvent

2cab983

Apply suggestions from code review

e39d660

Co-authored-by: Philipp Hofmann <[email protected]>

romtsn reviewed Feb 22, 2024

View reviewed changes

Sources/Sentry/SentryEnvelope.m Show resolved Hide resolved

romtsn approved these changes Feb 22, 2024

View reviewed changes

brustolin and others added 2 commits February 22, 2024 14:10

Update SentryClient.m

53b3089

Format code

b780c1e

philipphofmann approved these changes Feb 22, 2024

View reviewed changes

Sources/Sentry/include/SentryMsgPackSerializer.h Show resolved Hide resolved

brustolin merged commit 88c7547 into feat/session-replay Feb 29, 2024

brustolin deleted the feat(SR)/ReplayEvent branch February 29, 2024 09:26

	// Transactions have their own sampleRate
	if (eventIsNotATransaction && [self isSampled:self.options.sampleRate]) {
	SENTRY_LOG_DEBUG(@"Event got sampled, will not send the event");
	[self recordLostEvent:kSentryDataCategoryError reason:kSentryDiscardReasonSampleRate];
	return nil;
	}


		@end

		@implementation SentryMsgPackSerializerTests


		@end

		@implementation SentryMsgPackSerializerTests

Uh oh!

Comments

Conversation

brustolin commented Feb 14, 2024 • edited by bruno-garcia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: feat(SR)/ReplayEvent

Startup times

App size

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaind left a comment

Choose a reason for hiding this comment

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipphofmann Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romtsn left a comment

Choose a reason for hiding this comment

Uh oh!

brustolin commented Feb 14, 2024 •

edited by bruno-garcia

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

philipphofmann Feb 22, 2024 •

edited

Loading