-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(replay): filter out events before replay start for log messages #102931
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
base: master
Are you sure you want to change the base?
Conversation
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.
Bug: Prematurely Emitting Pre-Start Error Events
Remaining error events are yielded without checking if they occurred before replay_start_ms. After processing all segment events, the code yields all remaining errors regardless of their timestamps, allowing error messages from before the replay started to appear in the logs.
src/sentry/replays/usecases/summarize.py#L310-L320
sentry/src/sentry/replays/usecases/summarize.py
Lines 310 to 320 in 9a5ea32
| # Yield any remaining error messages | |
| while error_idx < len(error_events): | |
| error = error_events[error_idx] | |
| if error["category"] == "error": | |
| yield generate_error_log_message(error) | |
| elif error["category"] == "feedback": | |
| yield generate_feedback_log_message(error) | |
| error_idx += 1 |
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.
Bug: Feedback breadcrumb timestamp in seconds causes filtering error
The feedback breadcrumb timestamp in the test is in seconds instead of milliseconds. The timestamp field should be float((now - timedelta(minutes=3)).timestamp() * 1000) to match the expected millisecond format for segment events, causing this feedback to be incorrectly filtered out as occurring before the replay start.
tests/sentry/replays/usecases/test_summarize.py#L1404-L1405
sentry/tests/sentry/replays/usecases/test_summarize.py
Lines 1404 to 1405 in 3488fa5
| self.store_replay(dt=now - timedelta(minutes=10), segment_id=0, trace_ids=[trace_id]) |
| return list(generate_summary_logs(segment_data, error_events, project_id, is_mobile_replay)) | ||
| return list( | ||
| generate_summary_logs( | ||
| segment_data, error_events, project_id, is_mobile_replay, replay_start |
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.
| segment_data, error_events, project_id, is_mobile_replay, replay_start | |
| segment_data, error_events, project_id, is_mobile_replay=is_mobile_replay, replay_start=replay_start |
and same for L598 - best to be explicit w kwargs
| """ | ||
| error_idx = 0 | ||
| seen_feedback_ids = {error["id"] for error in error_events if error["category"] == "feedback"} | ||
| replay_start_ms = _parse_iso_timestamp_to_ms(replay_start) if replay_start else 0.0 |
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.
| replay_start_ms = _parse_iso_timestamp_to_ms(replay_start) if replay_start else 0.0 | |
| replay_start_ms = _parse_iso_timestamp_to_ms(replay_start) if replay_start else 0.0 | |
| while error_events[error_idx]["timestamp"] < replay_start_ms: | |
| error_idx += 1 |
lets you avoid the ifs below when yielding errors
| # Create an error that occurred BEFORE replay start (should be filtered) | ||
| early_error_id = uuid.uuid4().hex | ||
| early_error_timestamp = (replay_start - timedelta(minutes=3)).timestamp() | ||
| self.store_event( | ||
| data={ | ||
| "event_id": early_error_id, | ||
| "timestamp": early_error_timestamp, | ||
| "exception": { | ||
| "values": [ | ||
| { | ||
| "type": "EarlyError", | ||
| "value": "This happened before replay started", | ||
| } | ||
| ] | ||
| }, | ||
| "contexts": { | ||
| "trace": { | ||
| "type": "trace", | ||
| "trace_id": trace_id, | ||
| "span_id": span_id, | ||
| } | ||
| }, | ||
| }, | ||
| project_id=self.project.id, | ||
| ) | ||
|
|
||
| # Create an error that occurred AFTER replay start (should be included) |
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.
would make these errors direct connected rather than trace connected, cuz the trace query already uses the replay range filter
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
related to: #102897
backend PR for filtering out events that occur before the replay start time, so we don't have to generate log messages for those. we filter out these events on the frontend as well -- they aren't shown as breadcrumbs.
also fix some timestamp inconsistencies in tests