feat(replays): Reject replay envelopes if any item contained within is invalid

[cmanallen]

Closes: #3180

Drop the envelope in its entirety if any component fails validation. This prevents confusing states where is shown a replay which was never ingested.

[jjbayer]

See comment. We definitely need a few tests for this at well.

[jjbayer]

This leads to different behavior depending on the order of items:

1. If the first item is dropped with an outcome, all subsequent items are dropped silently (one outcome in total).
2. If the second item is dropped with an outcome, the first outcome will still be in the envelope after retain_items. In that case, the first item will be rejected with ReplayRejected outcome (two outcomes in total).

Maybe we can replace retain_items with a simple for loop that breaks on the first failure, and then rejects the envelope with the outcome of the first failure.

[cmanallen]

@jjbayer I thought reject logged and emitted no outcomes? This is what I was referencing when writing the PR.

relay/relay-server/src/utils/managed_envelope.rs

Line 409 in c1f1aff

Handling::Success => relay_log::debug!("dropped envelope: {outcome}"),

[jjbayer]

reject does log outcomes (the Handling is only used to decide whether or not to log an error). But it turns out we don't log outcomes for replays in reject, which is probably just oversight:

relay/relay-server/src/utils/managed_envelope.rs

Lines 433 to 468 in 8eff863

	if let Some(category) = self.event_category() {
	self.track_outcome(outcome.clone(), category, 1);
	}
	if self.context.summary.attachment_quantity > 0 {
	self.track_outcome(
	outcome.clone(),
	DataCategory::Attachment,
	self.context.summary.attachment_quantity,
	);
	}
	if self.context.summary.profile_quantity > 0 {
	self.track_outcome(
	outcome.clone(),
	if self.use_index_category() {
	DataCategory::ProfileIndexed
	} else {
	DataCategory::Profile
	},
	self.context.summary.profile_quantity,
	);
	}
	// Track outcomes for attached secondary transactions, e.g. extracted from metrics.
	//
	// Primary transaction count is already tracked through the event category
	// (see: `Self::event_category()`).
	if self.context.summary.secondary_transaction_quantity > 0 {
	self.track_outcome(
	outcome,
	// Secondary transaction counts are never indexed transactions
	DataCategory::Transaction,
	self.context.summary.secondary_transaction_quantity,
	);
	}

Not sure what the best way forward is here, maybe the best would be to return a ProcessingError, which is then automatically reported as an outcome via ProcessingError::to_outcome.

[cmanallen]

I've added retain_or_reject_all methods to ManagedEnvelope and Envelope. It guarantees one outcome is emitted, breaks early on processing error, ensures the envelope is empty after error, and uses all of the familiar APIs replay is accustomed to.

[jjbayer]

I looked into the code some more, I'm convinced now that the cleanest way to reject an entire envelope is to raise a ProcessingError and let it bubble up, because that's API that already exists for this purpose. And to make sure that ManagedEnvelope::reject emits outcomes for replays.

For the test, you can add the outcomes_consumer fixture in existing replay integration tests for that, or add new tests in test_outcome.py.

[cmanallen]

@jjbayer This commit implements the ProcessingError flow 27fcc83

[iker-barriocanal]

Is the goal to have a single outcome per replay? IIRC, in the past replays were split into two envelopes; how would outcome generation behave in that situation?

[cmanallen]

@iker-barriocanal This change doesn't handle that case but its not a common path. It might not be used any more.

[jjbayer]

Nice, with some tests this should be good to go!

[cmanallen]

@iker-barriocanal @jjbayer Adding test coverage for this. I'm seeing in the logs relay_server::utils::managed_envelope: dropped envelope: invalid data (invalid_replay) but no outcomes emitted. The reason is ManagedEnvelope.context.summary.event_category is None. How can I populate this?

[iker-barriocanal]

@cmanallen Relay doesn't infer event categories for replays, see

relay/relay-server/src/utils/rate_limits.rs

Lines 110 to 112 in 912a697

	ItemType::ReplayEvent => None,
	ItemType::ReplayRecording => None,
	ItemType::ReplayVideo => None,

I believe updating the applicable cases should populate the envelope context.
Edit: actually, this could have bad side-effects.

[cmanallen]

@iker-barriocanal I updated those locally and it didn't change the test failure :\

[jjbayer]

@cmanallen I added the missing replay rejection to ManagedEnvelope::reject and pushed a commit!

The quantity of the outcome will still be the number of items, because that is consistent with how we count replays for rate limiting purposes.

[cmanallen]

@jjbayer Updated test coverage and changed quantity to only emit one outcome. This should be ready.