feat(processor): Migrate process_replay
#5580
Conversation
| /// There should be either 0 of both or 1 of both. | ||
| #[error("invalid item count")] | ||
| InvalidItemCount, |
There was a problem hiding this comment.
Not sure if we want a more granular error here for the (0,n) and (n,0) case since this error becomes DiscardReason::DuplicateItem which is misleading if the envelope has (0,1) of the (recording,event)
| /// The PII config for scrubbing the recording could not be loaded. | ||
| #[error("invalid pii config")] | ||
| PiiConfigError(PiiConfigError), | ||
|
|
||
| /// An error occurred during PII scrubbing of the Replay. | ||
| /// | ||
| /// This error is usually returned when the PII configuration fails to parse. | ||
| #[error("failed to scrub PII: {0}")] | ||
| CouldNotScrub(String), |
There was a problem hiding this comment.
These are very similar but one becomes DiscardReason::InvalidReplayEventPii and the other DiscardReason::ProjectStatePii I think just having the later and getting rid of InvalidReplayEventPii should be fine?
|
|
||
| // FIXME: Might not need to be a vec in the future. | ||
| /// Expanded replays | ||
| replays: Vec<ExpandedReplay>, |
There was a problem hiding this comment.
I think technically this can be just one instead of a vec. See the comment in the expand function.
| let envelope = self.serialize_envelope(ctx)?; | ||
| let envelope = ManagedEnvelope::from(envelope).into_processed(); | ||
|
|
||
| s.store(crate::services::store::StoreEnvelope { envelope }); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
I think we can avoid the StoreEnvelope here.
tobias-wilfert
changed the title
process_replayprocess_replay
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| let mut item = Item::new(ItemType::ReplayVideo); | ||
| item.set_payload(ContentType::MsgPack, payload); | ||
| item | ||
| } |
There was a problem hiding this comment.
ReplayVideo content type changed from OctetStream to MsgPack
Medium Severity
The create_replay_video_item function sets ContentType::MsgPack for ReplayVideo items, but the old deleted code in relay-server/src/services/processor/replay.rs used ContentType::OctetStream. This is a behavior change that could affect downstream consumers that check the content type header, even though the actual payload format (MessagePack) hasn't changed. For a migration PR intending to preserve behavior, this is an unintended change.
| ); | ||
| records.reject_err(Error::FailedToSerializeReplay, replay); | ||
| } | ||
| } |
There was a problem hiding this comment.
Serialization failure now rejects instead of forwarding original
Medium Severity
The old code had a fallback when replay event serialization (to_json()) failed: it would log an error and return the original unprocessed payload, allowing the item to continue being forwarded. The new code rejects the replay with Error::FailedToSerializeReplay which maps to DiscardReason::Internal. This means serialization failures that previously resulted in forwarding (potentially unprocessed) data now result in the item being completely dropped with an outcome recorded.
Migrates the
process_replaylogic to our new processor architecture.Follow ups:
relay-event-normalization/src/replay.rslogic makes senseFixes: INGEST-515