fix(replay): Use correct replay category in client reports #6889
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lms24
commented
Jan 20, 2023
Comment on lines
-13
to
-14
| // Replay type event | ||
| | 'replay_event' |
There was a problem hiding this comment.
This was just a duplicated entry
Lms24
commented
Jan 20, 2023
| @@ -337,13 +338,15 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> { | |||
| // Note: we use `event` in replay, where we overwrite this hook. | |||
|
|
|||
| if (this._options.sendClientReports) { | |||
| // We want to track each category (error, transaction, session, replay_event) separately | |||
| const clientReportCategory = dataCategoryToClientReportCategory(category); | |||
There was a problem hiding this comment.
Arguably, it'd be cleaner to change the method's signature type to ClientReportCategory but this is breaking. Also, we'd need to call the conversion function more often which increases bundle size
size-limit report 📦
|
|
Actually, I think there is a better/easier fix for this:
I think that should work...? |
|
Made a draft to show what I mean, I am honestly not 100% sure if I am missing something here, but I think that should have the outcome we want: #6891 ?? WDYT? |
|
Closing in favour of #6891 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After adjusting the _admin UI to display outcomes of replay events (https://github.com/getsentry/getsentry/pull/9301), we noticed that only server-side outcomes are shown. After some investigation, I found out that the client reports our SDKs send to Sentry contain the wrong category for replay events:
Instead of
replay_event(orreplay_recording) the valid category for client reports should simply bereplay.To fix this, this PR first adds a conversion function to the
recordDroppedEventfunction (which collects the client outcomes) and secondly, it adds two newDataCategorytypes for rate limits and client reports. This is necessary because replay event types and rate-limit/client-report categories aren't identical anymore. While it'd arguably be better that they are in fact identical, I think this is the quickest fix and the types should help us distinguishing the applicable categories going forward.ref #6502