rfc(informational): Combined Replay Envelope Item

[JoshFerge]

Rendered RFC

[cmanallen]

I think there are some very old SDKs from alpha still out there that sent two separate requests. As far as I'm aware none of these SDKs are paying customers and they may have stopped ingesting totally.

I think its worthwhile documenting what happens in the event of a missing envelope item. I assume we should 400 or otherwise reject in some meaningful way. Something that tells the client to stop sending.

[cmanallen]

I don't think we need a new consumer. We can switch on the type field in the kafka payload. Right now the struct name is converted to snake-case and applied as the "type" value of the serialized object. We would update our consumer to accept the new type and then process accordingly.

[JoshFerge]

got it. 👍🏼

[cmanallen]

Caught this at the very end. I'm leaving my previous feedback in place :P

[cmanallen]

Tags could have a new-line character.

[cmanallen]

I'm not sure I understand. If rate-limits are applied before processing then this merged type doesn't exist yet.

[JoshFerge]

yeah, this would only be the case when people are on new SDKs that send the proposed new combined envelope format

[mydea]

Instead of making this a newline-separated string, could we also send this as a JSON like this:

{
  event: ReplayEventJSON,
  recording: CompressedReplayRecording
}

Not sure about implications for parsing this, but from an SDK perspective this seems a bit nicer/safer than string-concatting this. But not a hard feeling, just a thought.

[mydea]

a wait, I guess we can't put the compressed stuff into JSON 🤔 but now that I think about this, not sure if we can concat the UInt8Array that is the compressed payload onto a string as well 🤔

[cmanallen]

If this merged type is generated on the client then you will need to serialize with a binary format like msgpack.

[mydea]

hmm, I see. I guess we can do that, convert the payload into UInt8Array if it isn't compressed, then combine them together into a single UInt8Array.

One downside to note here is that it may make debugging harder - I currently tend to disable compression when debugging issues, in order to see what data is sent. This would not be possible anymore (as the payload would then always be UInt8Array). Not a blocker, just to be aware of this downside!

[JoshFerge]

update: in the RFC I proposed a combined format using a newline. I'd like to propose an alternative format of which we transmit it on the backend.

The Replays Recording Consumer expects MessagePack bytes in the payload field. I propose that in our processor, we combine the ReplayEvent and ReplayRecording into a single messagepack payload, and change the content type to ContentType::MessagePack.

The messagepack payload will be a key value object like:

{
  replay_event: {...}
  replay_recording: _binary_data_ or uncompressed data
  replay_recording_headers {}
}

In our recordings consumer, we'll parse the message pack which will then give us a dict with these values.

Depending on whether or not we can specify the entire kafka message be compressed or not, we can set the replay recording to the uncompressed value, or as a byte array.

We can also take this opportunity to put the parsed recording headers in the item in relay. This will remove another complication downstream in the consumer.

We could either emit these events onto a new topic with a new consumer, or have the existing consumer use a field to know whether or not the payload is a combined one or not. I think it would be easy to add a field onto the kafka message, like

"combined_event_and_recording": True

I will re-evaulate whether or not its worth it to make SDK changes to combine the envelope client side, but server side I feel good about this solution.

[cmanallen]

@JoshFerge

I think it would be easy to add a field onto the kafka message, like

Agree. Though I think a version field is better. Since we don't have one currently we can conditionally check if the version field exists and if it does deserialize with the new msgpack format. In the future, we can increment the version number to handle new payload types.

[JoshFerge]

@cmanallen great, i'll get started on this today.

[bruno-garcia]

Makes sense. Let's do this! Left a note on future considerations regarding SDK changes but doesn't affect anything agreed here

[bruno-garcia]

Suggested change

	1. Look at the version of the payload to determine if its the new event, and if so, handle additional work for the ReplayEvent, and load the SDK contextual data into the ReplayRecording for events emitted from it.
	1. Look at the version of the payload to determine if it's the new event, and if so, handle additional work for the ReplayEvent, and load the SDK contextual data into the ReplayRecording for events emitted from it.

[bruno-garcia]

We need to consider compatibility with Self Hosted (and Single Tenant) before we change the SDK