feat(replay): Remove replayType from tags and into replay_event
#6658
Conversation
Moves `replayType` from tags to part of `replay_event`. Added in backend in getsentry/replay-backend#210
Similar to #6658 but waiting on getsentry/replay-backend#226
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Nice. Just had some thoughts while going through this PR but they're more food for thought than change requests :D LGTM!
| replayEventPayload: expect.objectContaining({ | ||
| replay_type: 'error', |
There was a problem hiding this comment.
Just as a side note: I'm not too happy about assertions like expect.objectContaining because they don't catch certain kinds of changes to the object under test. In this example, when we add a field, we need to specifically look up these tests and add them to cover the new field. Also, if we "accidentally" add a field, this assertion wouldn't catch it. If we just use "hard" comparisons, I think we could increase the robustness of these tests, as they would fail automatically. It's not something we have to change right now but I think, we should keep it in mind going forward.
| sdk: { integrations: ['BrowserTracing', 'Replay'], name: 'sentry.javascript.browser', version: '7.25.0' }, | ||
| segment_id: 3, | ||
| tags: { errorSampleRate: 0, replayType: 'error', sessionSampleRate: 1 }, | ||
| tags: { errorSampleRate: 0, sessionSampleRate: 1 }, |
There was a problem hiding this comment.
Also side note: I think eventually, we'll want to remove these tags as well, right? IMO they don't really add value for our users and SDKs shouldn't just add additional tags to events (unless there's a justified reason to do so ofc). That being said, I know that it's convenient for us to have this data so we might want to think about a similar approach for the sample rate tags. WDYT?
|
I think this PR broke CI on master: https://github.com/getsentry/sentry-javascript/actions/runs/3856298130/jobs/6572358551#step:6:408 |
Fixes type error from #6658 (semantic merge issue).
Fixes type error from #6658 (semantic merge issue).
Fixes type error from #6658 (semantic merge issue).
Similar to #6658 but waiting on getsentry/replay-backend#226
Similar to #6658 but waiting on getsentry/replay-backend#226
Moves
replayTypefrom tags to part ofreplay_event.Added in backend in https://github.com/getsentry/replay-backend/issues/210