Skip to content

feat(preprod): Support absolute detectors from single-build path#111141

Merged
chromy merged 1 commit intomasterfrom
chromy/feat/absolute-size-analysis-single-build
Mar 24, 2026
Merged

feat(preprod): Support absolute detectors from single-build path#111141
chromy merged 1 commit intomasterfrom
chromy/feat/absolute-size-analysis-single-build

Conversation

@chromy
Copy link
Contributor

@chromy chromy commented Mar 19, 2026

Add maybe_emit_issues_from_size_results so that "absolute" threshold detectors fire as soon as a single build's size metrics are committed, without waiting for a comparison against a base artifact.

Previously, all issue emission went through the comparison path (maybe_emit_issues), which requires both head and base artifacts. This meant absolute detectors — which only need one build's size — couldn't fire until a comparison happened. Now:

  • The single-build path (called after metrics commit in _assemble_preprod_artifact_size_analysis) processes only threshold_type == "absolute" detectors
  • The comparison path (_maybe_emit_issues) processes only threshold_type in ("absolute_diff", "relative_diff") detectors
  • SizeAnalysisMetadata base fields (base_metric_id, base_artifact_id, base_artifact) are now NotRequired, and create_occurrence conditionally populates base-related evidence and tags

Agent transcript: https://claudescope.sentry.dev/share/UgFhB6vtmF1RiOVM-Ble2aoN68w7YgfkO_BzAXfdxnE

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 19, 2026
@chromy chromy marked this pull request as ready for review March 19, 2026 20:03
@chromy chromy requested a review from a team as a code owner March 19, 2026 20:03
@chromy chromy force-pushed the chromy/feat/absolute-size-analysis-single-build branch from f0021b4 to 7d89f9d Compare March 23, 2026 11:04

# Only process diff-based detectors from the comparison path
detectors = [
d for d in detectors if d.config.get("threshold_type") in ("absolute_diff", "relative_diff")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice if the values here were in some array const or something in the same file as all of the options so that it's easy to maintain if we ever add more options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks!

logger.exception("Error emitting issues from size results")


def _maybe_emit_issues_from_size_results(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is so specific to the "absolute" type of metrics, perhaps we should have the method title include that fact?

perhaps

_maybe_emit_issues_from_absolute_size_results or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks!

@chromy chromy force-pushed the chromy/feat/absolute-size-analysis-single-build branch from 7d89f9d to 3a64705 Compare March 23, 2026 19:59
Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Constants placed between import statements
    • Moved DIFF_THRESHOLD_TYPES and ABSOLUTE_THRESHOLD_TYPES constants to after the import block, following Python conventions.

Create PR

Or push these changes by commenting:

@cursor push 41940c7851
Preview (41940c7851)
diff --git a/src/sentry/preprod/size_analysis/tasks.py b/src/sentry/preprod/size_analysis/tasks.py
--- a/src/sentry/preprod/size_analysis/tasks.py
+++ b/src/sentry/preprod/size_analysis/tasks.py
@@ -28,14 +28,14 @@
 from sentry.tasks.base import instrumented_task
 from sentry.taskworker.namespaces import preprod_tasks
 from sentry.utils import metrics
+from sentry.utils.json import dumps_htmlsafe
+from sentry.workflow_engine.models import DataPacket, Detector
+from sentry.workflow_engine.processors.detector import process_detectors
 
 # Threshold type categories for filtering detectors by path.
 # These must stay in sync with the enum in PreprodSizeAnalysisGroupType.detector_settings.config_schema.
 DIFF_THRESHOLD_TYPES = frozenset({"absolute_diff", "relative_diff"})
 ABSOLUTE_THRESHOLD_TYPES = frozenset({"absolute"})
-from sentry.utils.json import dumps_htmlsafe
-from sentry.workflow_engine.models import DataPacket, Detector
-from sentry.workflow_engine.processors.detector import process_detectors
 
 logger = logging.getLogger(__name__)

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

# Threshold type categories for filtering detectors by path.
# These must stay in sync with the enum in PreprodSizeAnalysisGroupType.detector_settings.config_schema.
DIFF_THRESHOLD_TYPES = frozenset({"absolute_diff", "relative_diff"})
ABSOLUTE_THRESHOLD_TYPES = frozenset({"absolute"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constants placed between import statements

Low Severity

DIFF_THRESHOLD_TYPES and ABSOLUTE_THRESHOLD_TYPES are defined in the middle of the import block, with from sentry.utils.json import dumps_htmlsafe immediately following on line 36. This appears to be an accidental placement — the constants split the imports, reducing readability and violating standard Python conventions for keeping all imports together at the top of a file. The constants belong after the import block finishes.

Fix in Cursor Fix in Web

Add maybe_emit_issues_from_size_results to fire absolute threshold
detectors as soon as size metrics are available, without waiting for a
comparison. The existing comparison path now only processes diff-based
detectors (absolute_diff, relative_diff).

Make base-related fields in SizeAnalysisMetadata optional (NotRequired)
so single-build data packets are valid, and guard base field usage in
create_occurrence accordingly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/xbdNSzXgX_bRWElBRhFouJ15W3F6D85d-6haI6BK4E0
@chromy chromy force-pushed the chromy/feat/absolute-size-analysis-single-build branch from 3a64705 to 2b24838 Compare March 24, 2026 11:30
Comment on lines +656 to +658
project = head_metric.preprod_artifact.project
project_id = project.id
organization_id = project.organization.id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The function _maybe_emit_issues_from_absolute_size_results causes an N+1 query by accessing head_metric.preprod_artifact.project.organization in a loop without preloading these relations.
Severity: MEDIUM

Suggested Fix

Preload the necessary relations when fetching the metrics. Add .select_related('preprod_artifact__project__organization') to the query that populates the metrics_in_transaction list before it is iterated over.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/preprod/size_analysis/tasks.py#L656-L658

Potential issue: The function `_maybe_emit_issues_from_absolute_size_results` is called
within a loop for each metric. Inside the function, accessing
`head_metric.preprod_artifact.project.organization` triggers separate database queries
for `preprod_artifact`, `project`, and `organization` because these related objects are
not preloaded. This results in an N+1 query pattern, which can degrade the performance
of size analysis processing, particularly for builds that involve multiple components.

Did we get this right? 👍 / 👎 to inform future reviews.

@chromy chromy merged commit e85530a into master Mar 24, 2026
64 checks passed
@chromy chromy deleted the chromy/feat/absolute-size-analysis-single-build branch March 24, 2026 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants