-
Notifications
You must be signed in to change notification settings - Fork 26
Replays Self-Serve Bulk Delete System #5
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: replays-delete-vulnerable
Are you sure you want to change the base?
Conversation
This validates both the [Working Draft](https://www.w3.org/TR/reporting-1/#concept-reports) and the [Editor's Draft](https://w3c.github.io/reporting/#concept-reports) formats. Fixes [ID-730 - Accept current and upcoming data model](https://linear.app/getsentry/issue/ID-730/accept-current-and-upcoming-data-model).
…o 'low' (#93927)" This reverts commit 8d04522. Co-authored-by: roaga <47861399+roaga@users.noreply.github.com>
Missed in the initial commit, leading to some relevant logs being unannotated.
We have had a few tasks get killed at 10% rollout.
Also add a test, so that this doesn't happen again
Fixes DE-129 and DE-156 --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
These transitions should be matching
…` (#93946) Use `project_id` on the replay record instead of the URL (where it does not always exist). --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Also fixed `replay.view_html` -> `replay.view-html` --------- Co-authored-by: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com>
…948) gets `npx @typescript/native-preview` passing again
This adds mode for all things tracing. This encompasses transactions/metrics/spans. Taken from https://github.com/getsentry/sentry/blob/feeaf393deeca8b97675bff23039c6320270aab5/src/sentry/runner/commands/devserver.py#L370
The conditions associated with a DCG can change over time, and it's good if we can be completely confident that they're consistent within a given task execution.
This is unused and most regex experiments have required broader changes to ensure that regexes are evaluated in a specific order (ex: traceparent). Removing this for now to simplify the code and very slightly improve runtime performance.
From some testing (on feedback lists of all different lengths), this prompt seems to work better. It doesn't write overly long sentences and also does a better job at "summarizing" versus just mentioning a few specific topics and leaving out others.
Just remove a couple custom Flex* classes in favor of the Flex primitive
This has been killed a few times. Refs SENTRY-42M7
…n table (#93892) <!-- Describe your PR here. --> [ticket](https://linear.app/getsentry/issue/ID-156/grouping-info-remove-type-field-from-ui) The Type field in the Grouping Info section of the issue details page was redundant. This removes the Type row from all variant types while keeping the underlying data structure intact. before  after 
### Changes Related to this PR: getsentry/sentry#93810. This is part 1 of the change, which is pulling out the new component and just adding it to the repo. Also includes some simplification of the logic in the base component. Part 2 will be replacing tables in widgets. ### Before/After There is no UI change as the table is not being used yet. There is a new story page for the component.
…93943) to prevent this issue from becoming too noisy, add a noise config
Unfortunately, 'event_data' went from being the variable for current event context to being the complete parsed data from Redis, and we continued logging it per group. That's more data than we should be logging even arguably once, let alone per group.
Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
Adds some simple analytics to our endpoint so we can begin building a dashboard in Amplitude.
Previously, explore supported multiple y axis per chart, so each visualize supported multiple y axis. That functionality has since been removed for simplicity so update the types here to match. Keep in mind that saved queries still store them as an array so when serializing/deserializing, we still need to treat it as an array.
We'll need the `useGetTraceItemAttributeKeys` hook in other places so refactoring it so that it can exported.
- getsentry/sentry#93894 removed usage - getsentry/sentry-options-automator#4243 removed the last override
When the max segment ID is null the process fails. We should exit early since if there aren't any segments to delete there's nothing to do.
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.
Greptile Summary
This PR implements a comprehensive set of changes across the Sentry codebase focused on system improvements and feature enhancements. The most significant changes include:
Autofix Configuration Changes: The default value for sentry:autofix_automation_tuning has been changed from "low" to "off" across multiple configuration files (src/sentry/constants.py, src/sentry/projectoptions/defaults.py) and corresponding test updates. This makes AI-powered autofix functionality opt-in rather than enabled by default.
Workflow Engine Enhancements: Multiple improvements to the workflow engine including the addition of owner field support for detectors (allowing user/team ownership assignment), performance optimizations in delayed workflow processing by pre-computing slow conditions, and enhanced context-aware logging throughout the system.
Integration Improvements: Updates to GitHub and GitLab PR comment workflows with cleaner formatting, environment information display, and standardized template handling. The changes move from hardcoded templates to method-based approaches for better flexibility.
Replay System Enhancements: Significant improvements to the replay system including bulk deletion bug fixes (handling null max_segment_id values), enhanced breadcrumb summarization with error context integration, and comprehensive test coverage additions.
Analytics and Monitoring: New analytics tracking for preprod artifact assembly operations and various task timeout configurations to improve system reliability.
Code Quality Improvements: Removal of experimental grouping parameterization code, migration cleanup, and extensive test coverage additions across multiple endpoints and workflows.
These changes collectively improve system stability, user experience, and maintainability while adding new functionality for replay management and workflow ownership.
Confidence score: 4/5
• This PR is generally safe to merge with mostly configuration changes, bug fixes, and feature enhancements that follow established patterns.
• The score reflects some potential concerns around the fail-fast validation in browser reporting, hardcoded timeout values, and the significant scope of changes across many system components.
• Files that need more attention: src/sentry/issues/endpoints/browser_reporting_collector.py (fail-fast validation behavior), src/sentry/hybridcloud/tasks/deliver_webhooks.py (timeout values), and src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py (performance impact of error fetching).
39 files reviewed, 5 comments
| @@ -60,6 +62,22 @@ def data_conditions(self) -> BaseDataConditionValidator: | |||
| def update(self, instance: Detector, validated_data: dict[str, Any]): | |||
| instance.name = validated_data.get("name", instance.name) | |||
| instance.type = validated_data.get("detector_type", instance.group_type).slug | |||
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.
logic: Bug: Using 'detector_type' key but field is named 'type'. Should be validated_data.get('type', instance.group_type).slug
| instance.type = validated_data.get("detector_type", instance.group_type).slug | |
| instance.type = validated_data.get("type", instance.group_type).slug |
|
|
||
| @staticmethod | ||
| def get_merged_pr_single_issue_template(title: str, url: str, environment: str) -> str: | ||
| truncated_title = PRCommentWorkflow._truncate_title(title) |
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.
logic: The method calls PRCommentWorkflow._truncate_title(title) but could use the passed max_length parameter from the method signature for consistency
| def validate_timestamp(self, value: int) -> int: | ||
| """Validate that age is absent, but timestamp is present.""" | ||
| if self.initial_data.get("age"): | ||
| raise serializers.ValidationError("If timestamp is present, age must be absent") | ||
| return value | ||
|
|
||
| def validate_age(self, value: int) -> int: | ||
| """Validate that age is present, but not timestamp.""" | ||
| if self.initial_data.get("timestamp"): | ||
| raise serializers.ValidationError("If age is present, timestamp must be absent") | ||
| return value |
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.
logic: The mutual exclusion validation between age and timestamp correctly handles the two W3C specification versions, but both fields are optional which means reports with neither field would pass validation. Consider if at least one should be required.
| for report in raw_data: | ||
| browser_report = BrowserReport(**report) | ||
| serializer = BrowserReportSerializer(data=report) | ||
| if not serializer.is_valid(): | ||
| logger.warning( | ||
| "browser_report_validation_failed", | ||
| extra={"validation_errors": serializer.errors, "raw_report": report}, | ||
| ) | ||
| return Response( | ||
| {"error": "Invalid report data", "details": serializer.errors}, | ||
| status=HTTP_422_UNPROCESSABLE_ENTITY, | ||
| ) |
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.
style: The fail-fast validation approach rejects entire batches if any single report is invalid. Consider if partial success (processing valid reports while logging invalid ones) would be more resilient for browser reporting scenarios.
| iterator: Iterator[tuple[int, memoryview]], error_events: list[ErrorEvent] | ||
| ) -> list[str]: | ||
| # Sort error events by timestamp | ||
| error_events.sort(key=lambda x: x["timestamp"]) |
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.
style: In-place sorting of error_events could cause issues if the same list is reused elsewhere. Consider using sorted() to create a new list.
Test 5