Open bruno-garcia opened 5 months ago
https://github.com/getsentry/sentry/pull/68950 now merged, this work is no longer blocked. should be replay.id
everywhere
what minimum self-hosted version does this require? We can sneak the change into v8!
@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.
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 🤔
@mydea we could keep a set of error ids if we are in buffer mode, and check against that in handleAfterSend
?
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 🤔
Would we feel badly about closing this issue? IMHO it does not appear too important to me 🤔
Let's clarify the ask for this ticket next sync @mydea, but from what I understand:
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 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.
Relay automatically adds
replay_id
tocontexts
: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.