Skip to content

ref(snapshots): Add configurable diff_threshold for snapshot comparison #112728

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.

Sign up for GitHub

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

Merged
NicoHinderling merged 2 commits into from
Apr 10, 2026
Merged

ref(snapshots): Add configurable diff_threshold for snapshot comparison #112728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
418c4ee
ref(snapshots): Add configurable diff_threshold for snapshot comparison
NicoHinderling Apr 10, 2026
2fdde05
ref(snapshots): Validate diff_threshold range and remove odiff workar…
NicoHinderling Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
"additionalProperties": ImageMetadata.schema(),
"maxProperties": 50000,
},
"diff_threshold": {"type": "number", "minimum": 0.0, "exclusiveMaximum": 1.0},
**VCS_SCHEMA_PROPERTIES,
},
"required": ["app_id", "images"],
Expand Down Expand Up @@ -466,6 +467,7 @@ def post(self, request: Request, project: Project) -> Response:

app_id = data.get("app_id")
images = data.get("images", {})
diff_threshold = data.get("diff_threshold")

# VCS info
head_sha = data.get("head_sha")
Expand Down Expand Up @@ -524,7 +526,7 @@ def post(self, request: Request, project: Project) -> Response:
# Write manifest inside the transaction so that a failed objectstore
# write rolls back the DB records, ensuring both succeed or neither does.
session = get_preprod_session(project.organization_id, project.id)
manifest = SnapshotManifest(images=images)
manifest = SnapshotManifest(images=images, diff_threshold=diff_threshold)
manifest_json = manifest.json(exclude_none=True)
session.put(manifest_json.encode(), key=manifest_key)

Expand Down
1 change: 1 addition & 0 deletions src/sentry/preprod/snapshots/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class Config:

class SnapshotManifest(BaseModel):
images: dict[str, ImageMetadata]
diff_threshold: float | None = Field(default=None, ge=0.0, lt=1.0)


class ComparisonImageResult(BaseModel):
Expand Down
21 changes: 20 additions & 1 deletion src/sentry/preprod/snapshots/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,8 @@ def compare_snapshots(
comparison.save(update_fields=["state", "error_code", "date_updated"])
return

diff_threshold = head_manifest.diff_threshold

head_images = head_manifest.images
base_images = base_manifest.images

Expand Down Expand Up @@ -571,12 +573,29 @@ def _fetch_hash(h: str) -> None:
)
session.put(diff_mask_bytes, key=diff_mask_key, content_type="image/png")

is_changed = diff_result.changed_pixels > 0
diff_pct = (
diff_result.changed_pixels / diff_result.total_pixels
if diff_result.total_pixels > 0
else 0
)
effective_threshold = diff_threshold if diff_threshold is not None else 0.0
Comment thread
NicoHinderling marked this conversation as resolved.
is_changed = diff_pct > effective_threshold
Comment thread
sentry[bot] marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 10, 2026

Choose a reason for hiding this comment

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

Missing hash-mismatch safety check described in PR

Medium Severity

The PR description explicitly states "Add a hash-mismatch safety check: if odiff reports 0 diff but content hashes differ, force the image as changed," and the PR discussion shows a hashes_differ = head_hash != base_hash line that was present in an earlier version of the code. This safety check is missing from the final diff. At this point in the code, head_hash != base_hash is guaranteed (filtered at line 414), but odiff can still report 0 changed_pixels due to its own ODIFF_SENSITIVITY_DIFF_THRESHOLD. Without the safety check, such images are incorrectly classified as "unchanged" even though their content hashes differ.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2fdde05. Configure here.

Copy link
Copy Markdown
Contributor Author

@NicoHinderling NicoHinderling Apr 10, 2026

Choose a reason for hiding this comment

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

forgot to update PR description

if is_changed:
changed_count += 1
else:
unchanged_count += 1

logger.debug(
"compare_snapshots: %s diff_pct=%.6f threshold=%s is_changed=%s pixels=%d/%d",
name,
diff_pct,
diff_threshold,
is_changed,
diff_result.changed_pixels,
diff_result.total_pixels,
extra={"head_artifact_id": head_artifact_id},
)

diff_mask_image_id = f"{head_artifact_id}/{base_artifact_id}/diff/{stem}.png"

image_results[name] = {
Expand Down
Loading