feat(preprod): Use sequential (n-1) comparisons for size analysis diff monitors#111482
feat(preprod): Use sequential (n-1) comparisons for size analysis diff monitors#111482
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
mtopo27
force-pushed
the
mtopo27/sequential-diff-monitors
branch
from
2b05752 to
32989ef
Compare
mtopo27
changed the base branch from
master
to
mtopo27/refactor-commit-comparison-conditional
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![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 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| search_filters = parse_search_query( | ||
| query, config=search_config, get_field_type=get_field_type | ||
| ) | ||
| queryset = apply_filters(queryset, search_filters, organization) |
There was a problem hiding this comment.
Missing queryset annotations for annotation-dependent search fields
Medium Severity
get_sequential_base_artifact creates a bare queryset via PreprodArtifact.objects.get_queryset() without calling the annotation methods that several valid search filter fields depend on. The existing queryset_for_query applies annotate_download_count, annotate_installable, annotate_main_size_metrics, and annotate_platform_name before calling apply_filters. Without those annotations, any detector query using install_size, download_size, download_count, installable, or platform_name will raise a Django FieldError at query execution time. That exception isn't caught by the InvalidSearchQuery handler in the calling loop, so it propagates to the top-level catch-all, silently aborting evaluation for all monitors on that artifact.
Additional Locations (1)
|
|
||
| queryset = queryset.exclude(pk=artifact.pk) | ||
|
|
||
| return queryset.order_by("-date_added").first() |
There was a problem hiding this comment.
No date constraint allows returning newer artifacts as base
Low Severity
get_sequential_base_artifact excludes the current artifact by PK and picks the most recent remaining one, but never filters date_added__lt=artifact.date_added. If an older artifact is reprocessed or tasks run out of order, the function can return a newer artifact as the "base," inverting the comparison direction. This contradicts the docstring's guarantee of finding "the most recent prior artifact."
Flip the early-return guard `if not artifact.commit_comparison: return` into an `if/else` block. This is a pure structural change with no behavior difference, preparing for a follow-up that adds logic to the else branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mtopo27
force-pushed
the
mtopo27/refactor-commit-comparison-conditional
branch
from
f20d53c to
21a3f8c
Compare
…f monitors Change size analysis diff monitors to use sequential comparisons instead of git-based (merge-base) comparisons. When a new build arrives, each diff-threshold monitor now resolves its base by finding the most recent prior artifact matching the detector's query filters and structural identity (app_id, artifact_type, build_configuration). - Add get_sequential_base_artifact() to artifact_search.py for n-1 base lookup with batch optimization per unique query string - Refactor maybe_emit_issues into maybe_emit_issues_from_diff_size_results which owns all diff monitor evaluation via sequential comparison - Remove maybe_emit_issues call from git-based _run_size_analysis_comparison so the git path only persists comparison records for status checks - Restructure compare_preprod_artifact_size_analysis so the commit_comparison check guards only the git block; sequential monitor eval runs unconditionally Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…uations Wire _run_size_analysis_comparison into the sequential (n-1) monitor path so that PreprodArtifactSizeComparison records are created and file-level diffs are persisted, matching the git-based status check path. Base metrics are now fetched once per unique base artifact and cached, removing redundant per-detector DB queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use `detector_base_metric` to avoid shadowing the outer `base_metric` variable, which caused an incompatible assignment and unreachable statement error in mypy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mtopo27
force-pushed
the
mtopo27/sequential-diff-monitors
branch
from
681eeeb to
33ac18c
Compare
…diff-monitors # Conflicts: # src/sentry/preprod/size_analysis/tasks.py
2 participants
Change size analysis diff monitors to use sequential (n-1) comparisons instead of git-based (merge-base) comparisons. When a new build arrives, each diff-threshold monitor (
absolute_diff,relative_diff) now resolves its base by finding the most recent prior artifact matching the detector's query filters and structural identity (app_id,artifact_type,build_configuration), ordered bydate_added.Previously, monitors were evaluated inside the git-based comparison path (
_run_size_analysis_comparison→maybe_emit_issues), which meant:commit_comparison) never triggered monitorsNow the two paths are independent:
PreprodArtifactSizeComparisonrecords and triggers status checks. No longer evaluates monitors.maybe_emit_issues_from_diff_size_resultsruns unconditionally after the git block (even without git metadata). Each detector resolves its own base viaget_sequential_base_artifact, with batch optimization (one DB lookup per unique query string across detectors).absolutethreshold monitors are unaffected — they're already handled separately bymaybe_emit_issues_from_absolute_size_results.