-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Replay]: Remove replayId
tag
#11618
Comments
getsentry/sentry#68950 now merged, this work is no longer blocked. should be |
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 When the event was successfully sent (in We could now make the sampling decision in |
@mydea we could keep a set of error ids if we are in buffer mode, and check against that in |
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:
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:
sentry-javascript/packages/replay-internal/src/coreHandlers/handleGlobalEvent.ts
Lines 62 to 68 in 1a715fc
Blocked by getsentry/sentry#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.
The text was updated successfully, but these errors were encountered: