Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit f84b57e

Browse files
authoredSep 3, 2024
fixreplays): Remove segment limit (#3984)
There is a "bug" in the SDK where a replay will emit more than one segment every 5 seconds. This leads to certain customers exceeding the segment limit earlier than others. By removing the limit here we allow the SDK to determine the cap based on the duration of the replay. As a service protection measure this limit fails to catch bad actors and only triggers when a malformed SDK exceeds its limit. Removing this limit and logging will allow us to fix (or not) SDK issues without impacting customers. #skip-changelog
1 parent 594561f commit f84b57e

File tree

4 files changed

+21
-44
lines changed

4 files changed

+21
-44
lines changed
 

‎relay-event-normalization/src/replay.rs

Lines changed: 1 addition & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,6 @@ pub enum ReplayError {
2929
#[error("invalid payload {0}")]
3030
InvalidPayload(String),
3131

32-
/// The Replay has consumed its segment limit.
33-
///
34-
/// This is returned from [`validate`].
35-
#[error("invalid replay length")]
36-
TooLong,
37-
3832
/// An error occurred during PII scrubbing of the Replay.
3933
///
4034
/// This erorr is usually returned when the PII configuration fails to parse.
@@ -52,19 +46,11 @@ pub fn validate(replay: &Replay) -> Result<(), ReplayError> {
5246
.value()
5347
.ok_or_else(|| ReplayError::InvalidPayload("missing replay_id".to_string()))?;
5448

55-
let segment_id = *replay
49+
replay
5650
.segment_id
5751
.value()
5852
.ok_or_else(|| ReplayError::InvalidPayload("missing segment_id".to_string()))?;
5953

60-
// Each segment is expected to be 5 seconds in length. A cap of 1080 segments means we
61-
// allow a replay to be up to 1.5 hours in length.
62-
const MAX_SEGMENT_ID: u64 = 1080;
63-
64-
if segment_id > MAX_SEGMENT_ID {
65-
return Err(ReplayError::TooLong);
66-
}
67-
6854
if replay
6955
.error_ids
7056
.value()
@@ -372,29 +358,6 @@ mod tests {
372358
assert!(replay_value.urls.value().unwrap().len() == 100);
373359
}
374360

375-
#[test]
376-
fn test_validate_segment_id() {
377-
let replay_id =
378-
Annotated::new(EventId("52df9022835246eeb317dbd739ccd059".parse().unwrap()));
379-
let segment_id: Annotated<u64> = Annotated::new(1081);
380-
let mut replay = Annotated::new(Replay {
381-
replay_id,
382-
segment_id,
383-
..Default::default()
384-
});
385-
assert!(validate(replay.value_mut().as_mut().unwrap()).is_err());
386-
387-
let replay_id =
388-
Annotated::new(EventId("52df9022835246eeb317dbd739ccd059".parse().unwrap()));
389-
let segment_id: Annotated<u64> = Annotated::new(1080);
390-
let mut replay = Annotated::new(Replay {
391-
replay_id,
392-
segment_id,
393-
..Default::default()
394-
});
395-
assert!(validate(replay.value_mut().as_mut().unwrap()).is_ok());
396-
}
397-
398361
#[test]
399362
fn test_truncated_list_less_than_limit() {
400363
let mut replay = Annotated::new(Replay {

‎relay-server/src/services/outcome.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,6 @@ pub enum DiscardReason {
449449
InvalidReplayEventPii,
450450
InvalidReplayRecordingEvent,
451451
InvalidReplayVideoEvent,
452-
ReplayExceededSegmentLimit,
453452

454453
/// (Relay) Profiling related discard reasons
455454
Profiling(&'static str),
@@ -501,7 +500,6 @@ impl DiscardReason {
501500
DiscardReason::InvalidReplayEventPii => "invalid_replay_pii_scrubber_failed",
502501
DiscardReason::InvalidReplayRecordingEvent => "invalid_replay_recording",
503502
DiscardReason::InvalidReplayVideoEvent => "invalid_replay_video",
504-
DiscardReason::ReplayExceededSegmentLimit => "replay_segment_limit_exceeded",
505503
DiscardReason::Profiling(reason) => reason,
506504
DiscardReason::InvalidSpan => "invalid_span",
507505
DiscardReason::FeatureDisabled(_) => "feature_disabled",

‎relay-server/src/services/processor/replay.rs

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use serde::{Deserialize, Serialize};
1818
use crate::envelope::{ContentType, ItemType};
1919
use crate::services::outcome::DiscardReason;
2020
use crate::services::processor::{ProcessEnvelopeState, ProcessingError, ReplayGroup};
21-
use crate::statsd::RelayTimers;
21+
use crate::statsd::{RelayCounters, RelayTimers};
2222

2323
/// Removes replays if the feature flag is not enabled.
2424
pub fn process(
@@ -143,6 +143,22 @@ fn handle_replay_event_item(
143143
global_config.filters(),
144144
)
145145
.map_err(ProcessingError::ReplayFiltered)?;
146+
147+
// Log segments that exceed the hour limit so we can diagnose errant SDKs
148+
// or exotic customer implementations.
149+
if let Some(segment_id) = replay_type.segment_id.value() {
150+
if *segment_id > 720 {
151+
metric!(counter(RelayCounters::ReplayExceededSegmentLimit) += 1);
152+
153+
relay_log::warn!(
154+
?event_id,
155+
project_id = project_id.map(|v| v.value()),
156+
organization_id = organization_id,
157+
segment_id = segment_id,
158+
"replay segment-exceeded-limit"
159+
);
160+
}
161+
}
146162
}
147163

148164
match replay.to_json() {
@@ -178,9 +194,6 @@ fn handle_replay_event_item(
178194
ReplayError::InvalidPayload(_) => {
179195
ProcessingError::InvalidReplay(DiscardReason::InvalidReplayEvent)
180196
}
181-
ReplayError::TooLong => {
182-
ProcessingError::InvalidReplay(DiscardReason::ReplayExceededSegmentLimit)
183-
}
184197
})
185198
}
186199
}

‎relay-server/src/statsd.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -822,6 +822,8 @@ pub enum RelayCounters {
822822
/// This metric is tagged with:
823823
/// - `aggregator`: The name of the metrics aggregator (usually `"default"`).
824824
BucketsDropped,
825+
/// Incremented every time a segment exceeds the expected limit.
826+
ReplayExceededSegmentLimit,
825827
}
826828

827829
impl CounterMetric for RelayCounters {
@@ -866,6 +868,7 @@ impl CounterMetric for RelayCounters {
866868
RelayCounters::CogsUsage => "cogs.usage",
867869
RelayCounters::ProjectStateFlushMetricsNoProject => "project_state.metrics.no_project",
868870
RelayCounters::BucketsDropped => "metrics.buckets.dropped",
871+
RelayCounters::ReplayExceededSegmentLimit => "replay.segment_limit_exceeded",
869872
}
870873
}
871874
}

0 commit comments

Comments
 (0)
Failed to load comments.