feat(uptime): Add UptimeResponseCapture model#106341
Conversation
|
This PR has a migration; here is the generated SQL for for --
-- Create model UptimeResponseCapture
--
CREATE TABLE "uptime_uptimeresponsecapture" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "date_updated" timestamp with time zone NOT NULL, "date_added" timestamp with time zone NOT NULL, "file_id" bigint NOT NULL, "uptime_subscription_id" bigint NOT NULL);
ALTER TABLE "uptime_uptimeresponsecapture" ADD CONSTRAINT "uptime_uptimeresponsecapture_file_id_adfe9aaa_fk_sentry_file_id" FOREIGN KEY ("file_id") REFERENCES "sentry_file" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "uptime_uptimeresponsecapture" VALIDATE CONSTRAINT "uptime_uptimeresponsecapture_file_id_adfe9aaa_fk_sentry_file_id";
ALTER TABLE "uptime_uptimeresponsecapture" ADD CONSTRAINT "uptime_uptimerespons_uptime_subscription__27b6838c_fk_uptime_up" FOREIGN KEY ("uptime_subscription_id") REFERENCES "uptime_uptimesubscription" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "uptime_uptimeresponsecapture" VALIDATE CONSTRAINT "uptime_uptimerespons_uptime_subscription__27b6838c_fk_uptime_up";
CREATE INDEX CONCURRENTLY "uptime_uptimeresponsecapture_file_id_adfe9aaa" ON "uptime_uptimeresponsecapture" ("file_id");
CREATE INDEX CONCURRENTLY "uptime_uptimeresponsecapture_uptime_subscription_id_27b6838c" ON "uptime_uptimeresponsecapture" ("uptime_subscription_id");
CREATE INDEX CONCURRENTLY "uptime_upti_uptime__291edd_idx" ON "uptime_uptimeresponsecapture" ("uptime_subscription_id", "date_added"); |
|
This PR has a migration; here is the generated SQL for for --
-- Add field capture_response_on_failure to uptimesubscription
--
ALTER TABLE "uptime_uptimesubscription" ADD COLUMN "capture_response_on_failure" boolean DEFAULT true NOT NULL;
--
-- Create model UptimeResponseCapture
--
CREATE TABLE "uptime_uptimeresponsecapture" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "date_updated" timestamp with time zone NOT NULL, "date_added" timestamp with time zone NOT NULL, "file_id" bigint NOT NULL, "scheduled_check_time_ms" bigint NOT NULL, "uptime_subscription_id" bigint NOT NULL);
ALTER TABLE "uptime_uptimeresponsecapture" ADD CONSTRAINT "uptime_uptimerespons_uptime_subscription__27b6838c_fk_uptime_up" FOREIGN KEY ("uptime_subscription_id") REFERENCES "uptime_uptimesubscription" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "uptime_uptimeresponsecapture" VALIDATE CONSTRAINT "uptime_uptimerespons_uptime_subscription__27b6838c_fk_uptime_up";
CREATE INDEX CONCURRENTLY "uptime_uptimeresponsecapture_uptime_subscription_id_27b6838c" ON "uptime_uptimeresponsecapture" ("uptime_subscription_id");
CREATE INDEX CONCURRENTLY "uptime_upti_uptime__aa1fbe_idx" ON "uptime_uptimeresponsecapture" ("uptime_subscription_id", "scheduled_check_time_ms"); |
2704aae to
dc977c2
Compare
c061aa0 to
0141374
Compare
0141374 to
fa28a92
Compare
fa28a92 to
cfb8923
Compare
cfb8923 to
bcedaf1
Compare
| uptime_subscription = FlexibleForeignKey( | ||
| "uptime.UptimeSubscription", related_name="response_captures" | ||
| ) | ||
| file_id = BoundedBigIntegerField() |
There was a problem hiding this comment.
Should this model class override delete() and remove the associated File record?
Stores HTTP response data captured during uptime check failures. The response body and headers are stored in an associated File to help users debug why their endpoint is failing. This is part of the Uptime Response Capture feature - storing HTTP response data (body + headers) when downtime incidents are created to help users debug failures. PRs in this series: 1. sentry-kafka-schemas: Schema changes - <link to PR> 2. uptime-checker: Response capture config and logic - <link to PR> 3. uptime-checker: Toggle handling via set_response_capture action - <link to PR> 4. sentry: Storage model (UptimeResponseCapture) (this PR) 5. sentry: Toggle producer (send_response_capture_toggle) 6. sentry: Consumer capture creation 7. sentry: Incident integration 8. sentry: API and UI
bcedaf1 to
d50c5e7
Compare
| uptime_subscription = FlexibleForeignKey( | ||
| "uptime.UptimeSubscription", related_name="response_captures" | ||
| ) |
There was a problem hiding this comment.
Bug: Deleting an UptimeSubscription triggers a database CASCADE that bypasses the custom UptimeResponseCapture.delete() method, orphaning associated File objects.
Severity: HIGH
Suggested Fix
Update the UptimeSubscriptionDeletionTask to explicitly handle the deletion of child UptimeResponseCapture objects. This can be done by overriding the get_child_relations() method in the task to include a relation for UptimeResponseCapture, ensuring their custom delete() logic is properly invoked.
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/uptime/models.py#L308-L310
Potential issue: When an `UptimeSubscription` is deleted via Sentry's custom deletion
task (`UptimeSubscriptionDeletionTask`), the database will perform a `CASCADE` delete on
related `UptimeResponseCapture` instances. This database-level operation bypasses
Django's model-level `delete()` method. As a result, the custom `delete()` method on
`UptimeResponseCapture`, which is responsible for cleaning up the associated `File`
object, is never called. This will lead to orphaned `File` objects in storage, causing a
persistent data leak.
Did we get this right? 👍 / 👎 to inform future reviews.
Stores HTTP response data captured during uptime check failures. The response body and headers are stored in an associated File to help users debug why their endpoint is failing.
Project Context
This is part of the Uptime Response Capture feature - storing HTTP response data (body + headers) when downtime incidents are created to help users debug failures.
PRs in this series: