feat(replay): combined envelope items #2170

JoshFerge · 2023-05-31T22:52:14Z

Implements the Combined Replay Envelope item as specified here: getsentry/rfcs#82

New ItemType
New processing function that takes the two items, combines them in a MsgPack, and re-inserts the item into the envelope
- This just uses msgpack keys and bytes as the values. Attempting to serialize the nested key/values of the messages proved problematic as Annotated cannot be easily serialized to messagepack.
New logic in store to handle this new ItemType. It will emit to a topic already in use.

Other Changes:

ReplayEvent can no longer "create an event" as this was putting it in a separate envelope for processing. The Envelope containing a replay should always contain an event_id, so this shouldn't be a problem.

Open Questions:

I'm not quite sure how to think about processing vs non-processing relays and this feature. This is something I could use ingests help on, and I have a commented out test for confirming this behavior once we know what we want. e.g.

Should this only run in processing mode? If not, is there anything else we need to add here?

cmanallen

I know you explicitly asked for no review which is why this isn't a review! It just looks like one :P

These are just general comments from looking at the code. No questions. No calls for action. Do your thing and let me know if you want me to help at all.

relay-server/src/actors/store.rs

relay-server/src/actors/processor.rs

bruno-garcia · 2023-06-06T19:22:44Z

Taking note that we'll rely on this change to have the data we need in order to create Dead Click events:

[Epic] Capture events for Dead Clicks sentry#48259

relay-server/src/actors/processor.rs

iker-barriocanal · 2023-06-07T11:19:24Z

Hi @JoshFerge! I see getsentry/rfcs#82 is not merged yet so I assume there's still work pending, such as this question. Could we first agree on the RFC before jumping into the implementation?

bruno-garcia · 2023-06-07T13:22:08Z

Hi @JoshFerge! I see getsentry/rfcs#82 is not merged yet so I assume there's still work pending, such as this question. Could we first agree on the RFC before jumping into the implementation?

Iker the comment is about the sdk change and won't affect this PR. Can we get a hand here to help clarify Josh's questions?
We'll merge the rfc in parallel.

Specifically:

Open Questions:

I'm not quite sure how to think about processing vs non-processing relays and this feature. This is something I could use ingests help on, and I have a commented out test for confirming this behavior once we know what we want. e.g.

Should this only run in processing mode? If not, is there anything else we need to add here?

…etsentry/relay into jferg/combine-replay-env-items

cmanallen · 2023-06-07T15:57:11Z

@JoshFerge Updated the pull. We're now splitting the headers and the body. I've also added error handling around serialization failure. There are comments which should explain the intent of the changes. My thought is if the envelope combining process fails we should default to the old style (e.g. two, un-merged envelope items).

JoshFerge · 2023-06-07T16:18:20Z

Hi @JoshFerge! I see getsentry/rfcs#82 is not merged yet so I assume there's still work pending, such as this question. Could we first agree on the RFC before jumping into the implementation?

Sorry, I wasnt sure how to update RFCs based off of a discussion, but see this comment from me last week, guiding this implementation: getsentry/rfcs#82 (comment). i'll go ahead and update the main Doc.

iker-barriocanal

About processing vs non-processing relays (docs), the general approach should be to not target processing actions to processing relays unless there's a reason not to. Non-processing relays include our internal PoP Relays, but also external relays run by customers (including sidecars).

I have only skimmed through the PR, leaving a few questions.

iker-barriocanal · 2023-06-09T11:31:15Z

relay-server/src/utils/sizes.rs

+ // The Combined Replay Envelope isn't generated on the client so its size does not need
+ // to be checked.
+ ItemType::CombinedReplayEventAndRecording => (),


Even if envelopes are generated in Relays, size requirements may vary and we don't have control over external relays, which could be running an older version and sending bigger envelopes.

gotcha, will add a size here.

iker-barriocanal · 2023-06-09T11:32:57Z

relay-server/src/envelope.rs

@@ -743,13 +752,14 @@ impl Item {
 ItemType::RawSecurity => true,
 ItemType::UnrealReport => true,
 ItemType::UserReport => true,
- ItemType::ReplayEvent => true,
+ ItemType::ReplayEvent => false,


ReplayEvent can no longer "create an event" as this was putting it in a separate envelope for processing. The Envelope containing a replay should always contain an event_id, so this shouldn't be a problem.

Should we expect older SDKs to continue sending ReplayEvents? This change seems like a breaking change for users not upgrading their SDKs.

I don't believe this is a breaking change, as ReplayEvents and ReplayRecordings are always in the same envelope, which has an event_id envelope header.

relay-dynamic-config/src/feature.rs

jjbayer

I'm unclear on whether or not Relay should be able to handle combined items coming from SDKs:

We'd then modify the sentry-sdk to send combined item types going forward

(rfc)

Envelopes will always arrive in an uncombined state.

Primary concern is: in this PR's implementation combined items will bypass data scrubbing, because they are not considered by process_replays. So if we expect SDKs to send combined items, we have to split and re-join these combined items in relay to do the scrubbing.

Should this only run in processing mode? If not, is there anything else we need to add here?

I think it makes sense to run this only in processing mode for now, for two reasons:

In general, it is easier to iterate on experimental changes if they are not performed by external relays, which might run outdated versions.
More specifically, if we change the data scrubbing logic for replays, items combined by outdated external relays would bypass this updated logic (external relays play a role similar to SDKs in this regard).

To do so, wrap the call to process_replays_combined_item in a if_processing!({ condition.

relay-server/src/envelope.rs

jjbayer · 2023-06-12T11:58:37Z

relay-server/src/actors/processor.rs

+ fn process_replays_combine_items(
+ &self,
+ state: &mut ProcessEnvelopeState,
+ ) -> Result<(), ProcessingError> {


If this function does not fail, no need to return a Result.

Suggested change

) -> Result<(), ProcessingError> {

)

relay-server/src/actors/processor.rs

relay-dynamic-config/src/feature.rs

jjbayer · 2023-06-12T12:51:40Z

relay-server/src/actors/processor.rs

+ let (headers, body) =
+ split_headers_from_body(replay_recording_item.payload().into())
+ .expect("This is pre-validated and should not fail.");


Instead of performing this split twice (once in process_replays and once here), I would unite the two functions and serialize either back into a recording item or into a message pack item:

flowchart TD A[event, recording] -->|split| B[event, header, body] B -->|scrub| C[event, header, body] C --> D{combine?} D -->|yes| E[MsgPack: event, header, body] D -->|no| F[event, recording]

Loading

@jjbayer Whats the best way to emit outcomes for dropped items? The only way I've found to do it is with retain_items but it seems like that method won't work for this use case.

You could manually call the function that retain_items calls:

relay/relay-server/src/utils/managed_envelope.rs

Lines 231 to 243 in c057e37

fn track_outcome(&self, outcome: Outcome, category: DataCategory, quantity: usize) {

self.outcome_aggregator.send(TrackOutcome {

timestamp: self.received_at(),

scoping: self.context.scoping,

outcome,

event_id: self.envelope.event_id(),

remote_addr: self.meta().remote_addr(),

category,

// Quantities are usually `usize` which lets us go all the way to 64-bit on our

// machines, but the protocol and data store can only do 32-bit.

quantity: quantity as u32,

});

}

@jjbayer So having done this change (e2782b8) I feel it was not worth it. I'd rather eat the cost of reading <20 bytes. In the original code, there may have been some memory concerns where we drop the merged value in exchange for the split values. That could have had implications (since we're doing it twice) but I leave it to you to decide if its worth it.

@jjbayer I am out for a little while. Josh Ferge will be re-taking over the PR so please direct feedback items to him. Thank you.

cmanallen · 2023-06-12T13:50:14Z

So if we expect SDKs to send combined items, we have to split and re-join these combined items in relay to do the scrubbing.

Is this true? We call process_replays prior to process_replays_combine_items. process_replays should mutate the envelope items prior to combination?

@cmanallen Prior to combination, yes. But if the SDK sends a CombinedReplayEventAndRecording, process_replays will ignore it because it is neither ReplayEvent nor ReplayRecording, right?

cmanallen · 2023-06-12T16:20:49Z

Prior to combination, yes. But if the SDK sends a CombinedReplayEventAndRecording, process_replays will ignore it because it is neither ReplayEvent nor ReplayRecording, right?

@jjbayer The SDK won't send that type so no worries there.

jjbayer

So having done this change (e2782b8) I feel it was not worth it. I'd rather eat the cost of reading <20 bytes.

@cmanallen I see your point that it does not improve readability, even though the data flow looks cleaner to me. Feel free to revert that change, as long as we don't .unwrap() the second split result.

Still pressing "Request changes" because it looks like process_replays and process_replays_combine_items contain duplicate code right now (e.g. process_recording).

jjbayer · 2023-06-19T10:24:59Z

relay-server/src/actors/processor.rs

+
+ // If combined envelope processing is not enabled default to the original workflow.
+ if !project_state.has_feature(Feature::SessionReplayCombinedEnvelopeItems) {
+ return self.process_replays(state);


Looks like we're calling process_replays twice in this case? Should we remove the call to process_replays here:

relay/relay-server/src/actors/processor.rs

Lines 2572 to 2573 in e2782b8

self.process_replays(state)?;

self.process_replays_combine_items(state)?;

TBS1996 · 2023-08-16T08:45:25Z

@JoshFerge Do you still want to work on this? If not we will close it.

cmanallen · 2023-08-16T13:41:16Z

@TBS1996 Sorry we've let this sit for a little bit. Josh and I will sync and see if we need to pursue this still. We'll close it out if so otherwise we'll get it back up to date.

jan-auer · 2023-09-15T11:00:07Z

Please feel free to reopen this, once you plan to pursue it. Thanks!

Reopening #2170 - We originally closed it because there wasn't a clear use case. Now there is. getsentry/sentry#60826 I've resolved merge conflicts, tests pass, and I believe comments from the previous PR have been addressed. --------- Co-authored-by: Joris Bayer <[email protected]> Co-authored-by: Colton Allen <[email protected]>

feat(replay): combined envelope items

e775838

JoshFerge force-pushed the jferg/combine-replay-env-items branch from 98494cf to e775838 Compare May 31, 2023 22:53

only run in processing mode

dc26a9d

JoshFerge mentioned this pull request Jun 1, 2023

feat(replays): Add dead click issue detector getsentry/sentry#50149

Merged

cmanallen reviewed Jun 6, 2023

View reviewed changes

relay-server/src/actors/store.rs Show resolved Hide resolved

relay-server/src/actors/processor.rs Outdated Show resolved Hide resolved

JoshFerge added 5 commits June 6, 2023 17:18

add integration test. fix processing mode stuff, add store logic

7033ae0

remove processing mode business, use vec instead of Bytes

2c9d294

fix args in combined store func

aa0c0ce

fix tests

9d21e80

remove unused test

d7d18bb

JoshFerge commented Jun 7, 2023

View reviewed changes

relay-server/src/actors/processor.rs Outdated Show resolved Hide resolved

JoshFerge commented Jun 7, 2023

View reviewed changes

relay-server/src/actors/processor.rs Outdated Show resolved Hide resolved

Merge branch 'master' into jferg/combine-replay-env-items

a2be327

JoshFerge marked this pull request as ready for review June 7, 2023 01:22

JoshFerge requested a review from a team June 7, 2023 01:22

iker-barriocanal assigned JoshFerge Jun 7, 2023

cmanallen added 5 commits June 7, 2023 10:02

Move headers and body splitting behavior into a separate function

5d8f9a7

Drop reference to bytes after the headers have been split from the body

85c7dc7

Encode the headers separately from the body

a25c220

Merge branch 'jferg/combine-replay-env-items' of https://github.com/g…

2045f70

…etsentry/relay into jferg/combine-replay-env-items

Serialize from structure and improve error handling in cases of failure

e5b05fd

Clean up language

0fd31f2

iker-barriocanal reviewed Jun 9, 2023

View reviewed changes

jjbayer requested changes Jun 12, 2023

View reviewed changes

Remove redundant split operation

e2782b8

jjbayer requested changes Jun 19, 2023

View reviewed changes

jan-auer closed this Sep 15, 2023

jan-auer deleted the jferg/combine-replay-env-items branch September 15, 2023 11:00

JoshFerge mentioned this pull request Jan 31, 2024

feat(replays): Combined Envelope Items (reopen) #3035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replay): combined envelope items #2170

feat(replay): combined envelope items #2170

JoshFerge commented May 31, 2023 •

edited

Loading

cmanallen left a comment •

edited

Loading

bruno-garcia commented Jun 6, 2023

iker-barriocanal commented Jun 7, 2023

bruno-garcia commented Jun 7, 2023 •

edited

Loading

cmanallen commented Jun 7, 2023

JoshFerge commented Jun 7, 2023

iker-barriocanal left a comment

iker-barriocanal Jun 9, 2023

JoshFerge Jun 9, 2023

iker-barriocanal Jun 9, 2023

JoshFerge Jun 9, 2023

jjbayer left a comment

jjbayer Jun 12, 2023

jjbayer Jun 12, 2023

cmanallen Jun 13, 2023

jjbayer Jun 14, 2023

cmanallen Jun 14, 2023 •

edited

Loading

cmanallen Jun 14, 2023

cmanallen commented Jun 12, 2023 •

edited by jjbayer

Loading

cmanallen commented Jun 12, 2023

jjbayer left a comment

jjbayer Jun 19, 2023

TBS1996 commented Aug 16, 2023

cmanallen commented Aug 16, 2023

jan-auer commented Sep 15, 2023

	fn track_outcome(&self, outcome: Outcome, category: DataCategory, quantity: usize) {
	self.outcome_aggregator.send(TrackOutcome {
	timestamp: self.received_at(),
	scoping: self.context.scoping,
	outcome,
	event_id: self.envelope.event_id(),
	remote_addr: self.meta().remote_addr(),
	category,
	// Quantities are usually `usize` which lets us go all the way to 64-bit on our
	// machines, but the protocol and data store can only do 32-bit.
	quantity: quantity as u32,
	});
	}

	self.process_replays(state)?;
	self.process_replays_combine_items(state)?;

feat(replay): combined envelope items #2170

feat(replay): combined envelope items #2170

Conversation

JoshFerge commented May 31, 2023 • edited Loading

cmanallen left a comment • edited Loading

Choose a reason for hiding this comment

bruno-garcia commented Jun 6, 2023

iker-barriocanal commented Jun 7, 2023

bruno-garcia commented Jun 7, 2023 • edited Loading

cmanallen commented Jun 7, 2023

JoshFerge commented Jun 7, 2023

iker-barriocanal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmanallen Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmanallen commented Jun 12, 2023 • edited by jjbayer Loading

cmanallen commented Jun 12, 2023

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBS1996 commented Aug 16, 2023

cmanallen commented Aug 16, 2023

jan-auer commented Sep 15, 2023

JoshFerge commented May 31, 2023 •

edited

Loading

cmanallen left a comment •

edited

Loading

bruno-garcia commented Jun 7, 2023 •

edited

Loading

cmanallen Jun 14, 2023 •

edited

Loading

cmanallen commented Jun 12, 2023 •

edited by jjbayer

Loading