-
-
Notifications
You must be signed in to change notification settings - Fork 61
Modifying Replay Deserializer To Parse Tap Message #7439
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
Conversation
tap_message: String, | ||
tap_view_class: String, | ||
tap_view_id: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The new fields tap_message
, tap_view_class
, and tap_view_id
added to ReplayRow
are missing corresponding columns in the ClickHouse database schema, which will cause insertion failures.
-
Description: The
ReplayRow
struct has been updated with new fieldstap_message
,tap_view_class
, andtap_view_id
to support tap events. However, the corresponding columns have not been added to the ClickHouse database schema. The schema definition insnuba/datasets/configuration/replays/storages/replays.yaml
and the lack of a new database migration file confirm this omission. When the Rust processor serializes a tap event, the resulting JSON will contain these new fields. The subsequent attempt to insert this data into thereplays
table will fail with a "Missing columns" error from ClickHouse, preventing tap event data from being saved. -
Suggested fix: Create a new database migration to add the
tap_message
,tap_view_class
, andtap_view_id
columns to thereplays_local
andreplays_dist
tables in ClickHouse. Also, update the YAML schema configuration atsnuba/datasets/configuration/replays/storages/replays.yaml
to include these new columns.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Let's add a simple unit test in replay.rs
to verify the behavior explicitly and then we'll go over next steps.
rust_snuba/src/processors/replays.rs
Outdated
event_hash: tap.event_hash, | ||
offset, | ||
partition, | ||
platform: "".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this since empty string is the default value for String
and platform
is String
.
rust_snuba/src/processors/replays.rs
Outdated
tap_message: tap.message, | ||
tap_view_class: tap.view_class, | ||
tap_view_id: tap.view_id, | ||
environment: event.environment.clone().unwrap_or("".to_string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
environment: event.environment.clone().unwrap_or("".to_string()), | |
environment: event.environment.clone().unwrap_or_default(), |
This PR modifies ReplayRow Type for a new event, and creates a new type ReplayTapEvent. Then, a new case is added to the deserializer function to parse tap event and convert to ReplayRow type.
Relates to: https://linear.app/getsentry/issue/REPLAY-760/modify-replay-processor