feat(replays): Combined Envelope Items (reopen)

[JoshFerge]

Reopening #2170

• We originally closed it because there wasn't a clear use case. Now there is. [Epic] Rage Click Issue and Alerting sentry#60826

I've resolved merge conflicts, tests pass, and I believe comments from the previous PR have been addressed.

[jjbayer]

Is the goal of this PR purely to unite the two items in the kafka message? If so, I would not introduce a new item type, and implement the entire logic in the kafka producer (store.rs) instead.

If the goal is to send combined items from relay to relay (in a chain of relays), we need to add support for parsing a combined item and processing it correctly. Right now, I believe such an item would hit this branch and cause an error:

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

Lines 233 to 235 in 8411b1d

	} else {
	// Cannot group this item type.
	ProcessingGroup::Ungrouped

[JoshFerge]

@jjbayer @cmanallen I've gone ahead and moved the logic to store -- now the change simply will look for the two replay items, and combine them if if finds them. A couple of questions to ponder:

1. How should we handle this roll-out? If we are able to use project features in store here, I would love to try this on the Sentry project first. If we cannot do that, it seems like we'd have to do some kind of hard cutover.

My general idea here would be to:

• Double produce, and simply log and ignore the messages with a replay_event on them in our consumer, and ensure that everything there is working correctly.

• Remove the production of Combined Messages temporarily, let any combined messages drain out

• Then do a hard cutover, where we change relay to produce combined messages instead of stand-alone replay recording messages.

• (Note, we've decided we're still going to produce replay_events from relay, so only recording messages will be impacted)

If we are able to use project features in store here, that would be more ideal, and also open to suggestions, perhaps we could use S4S if its set up with replays? Any other ideas?

[jjbayer]

@JoshFerge in replay::process, you have access to project features. So in that function you could set an internal item header similar to this one, and check that item header in store_envelope.

[jjbayer]

nit: We could declare these variables in store_envelope, so we don't have to transform to and from a vector.

[JoshFerge]

not sure what to do about the too many args here

[jjbayer]

Instead of passing replay_event, replay_recording, and a boolean flag, the most idiomatic would probably be to pass an enum like

enum Data {
    Event(&Item),
    Recording(&Item),
    Combined { event: &Item, recording: &Item }
}

That would also make states like `(None, None, true)` impossible.

[cmanallen]

Whoops forgot to publish this review on Friday...

[JoshFerge]

@cmanallen @jjbayer should be good for final review.

[jjbayer]

Instead of passing replay_event, replay_recording, and a boolean flag, the most idiomatic would probably be to pass an enum like

enum Data {
    Event(&Item),
    Recording(&Item),
    Combined { event: &Item, recording: &Item }
}

That would also make states like `(None, None, true)` impossible.

[cmanallen]

@jjbayer @TBS1996 Josh is out for the rest of the week. I've updated the PR. Please provide final approval.

Note: replay_event is only populated if the combined envelope feature is enabled. This means we don't need to consider the combined envelope boolean in any other step. Null is always sent if the feature is disabled or if replay_event was not present in the envelope.

Note: the kafka payload size calculation was reconfigured. I'm using addition rather than subtraction. This is to avoid situations where the replay_event is larger than the maximum size of a recording. This would yield a negative number but since its of type usize the value wraps around and could give incorrect results. Addition suffers from similar wrap around problems however because we our server's do not have 18 exabits of RAM I feel safe in using this method.

Note: oversized recording messages will now produce an outcome with discard reason TooLarge.