[Replay]: Remove `replayId` tag

[bruno-garcia]

Relay automatically adds replay_id to contexts:

• https://github.com/getsentry/relay/pull/1983

This should be enough for the product to link an error to a replay.

We should be able to remove this code now: https://github.com/getsentry/sentry-javascript/blob/1a715fc5891a59af6f9b3979d74e91e278bd90ad/packages/replay-internal/src/coreHandlers/handleGlobalEvent.ts#L62-L68

Blocked by https://github.com/getsentry/sentry/pull/68950

Worth noting that because this requires a fix that's being done in Sentry, this change will regress the experience of replays on Self Hosted Sentry that doesnt' include this version. So we might want to postpone adding this for a while.

[JoshFerge]

https://github.com/getsentry/sentry/pull/68950 now merged, this work is no longer blocked. should be replay.id everywhere

[AbhiPrasad]

what minimum self-hosted version does this require? We can sneak the change into v8!

[bruno-garcia]

@mydea said: the self hosted version required by v8 will contain this change (since it's from April). So it's fine for us to remove the tag now on any v8 release.

[mydea]

actually, looking at this, maybe this is not that easy after all 🤔

Just thinking about this: Today, when an error happens, in the code linked above (handleGlobalEvent) we make the sampling decision for replaysOnErrorSampleRate. If this passes, we add the replayId to the event that is being sent.

When the event was successfully sent (in handleAfterSend), we then, if previously we sampled this and the error was successfully sent, convert the replay buffer session and start sending this.

We could now make the sampling decision in handleAfterSend, but in this case the very first error that triggered sampling would not have the replayId tag yet, because only afterwards we started to send this along 🤔

[billyvg]

@mydea we could keep a set of error ids if we are in buffer mode, and check against that in handleAfterSend?

[mydea]

but the error would not have the replay attached on the server then, as at this point we would not attach the replay ID yet (I believe), as from the perspective of the SDK we are still buffering 🤔

[mydea]

Would we feel badly about closing this issue? IMHO it does not appear too important to me 🤔

[billyvg]

Let's clarify the ask for this ticket next sync @mydea, but from what I understand:

• We can remove the condition replay.recordingMode === "session" here because in this case, we already have the replay id in DSC: https://github.com/getsentry/sentry-javascript/blob/1a715fc5891a59af6f9b3979d74e91e278bd90ad/packages/replay-internal/src/coreHandlers/handleGlobalEvent.ts#L62-L68
• We cannot add replay id in DSC when recordingMode is buffering because it is still waiting for an error to occur before it can even sample
• If we get an error event and it is sampled for replay, we currently tag it with replayId. I think what we want to do here is remove the tag so that we are not polluting tags as that is generally reserved for users to use (and not for SDKs).

Based on the 3rd point above, I think what we actually want to do is move replay id from tags to context.

[billyvg]

@mydea now that we've kinda decided it's ok to mutate the DSC for replay_id, thoughts on adding replay id to DSC in the buffering case?

[mydea]

I guess the main question is, do we really want to/need to change anything about what we have right now? 🤔

[billyvg]

I'm good with closing this out, it doesn't seem like the benefits outweigh the added complexity and overhead.