Skip to content
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

feat(aci): Add new GroupOpenPeriod table #87813

Merged
merged 13 commits into from
Mar 28, 2025
Merged

Conversation

snigdhas
Copy link
Member

@snigdhas snigdhas commented Mar 24, 2025

Adds a new table GroupOpenPeriod to track the times an issue is open, i.e. having a status that is not resolved. This will be used for detector-based issues in place of events. For example, instead of relying on incidents, open periods will be used to determine the start and end of a metric alert firing for metric issues.

spec

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 24, 2025
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0846_add_group_open_periods.py ()

--
-- Create model GroupOpenPeriod
--
CREATE TABLE "sentry_groupopenperiod" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "user_id" bigint NULL, "date_started" timestamp with time zone NOT NULL, "date_ended" timestamp with time zone NULL, "data" text NULL, "data_condition_id" bigint NULL, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "resolution_activity_id" bigint NULL);
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_data_condition_id_ac18ad82_fk_workflow_" FOREIGN KEY ("data_condition_id") REFERENCES "workflow_engine_datacondition" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_data_condition_id_ac18ad82_fk_workflow_";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr" FOREIGN KEY ("group_id") REFERENCES "sentry_groupedmessage" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id" FOREIGN KEY ("project_id") REFERENCES "sentry_project" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac" FOREIGN KEY ("resolution_activity_id") REFERENCES "sentry_activity" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac";
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_user_id_807f0152" ON "sentry_groupopenperiod" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_data_condition_id_ac18ad82" ON "sentry_groupopenperiod" ("data_condition_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_group_id_f4411413" ON "sentry_groupopenperiod" ("group_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_project_id_5ef8bacc" ON "sentry_groupopenperiod" ("project_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_resolution_activity_id_cc5cc897" ON "sentry_groupopenperiod" ("resolution_activity_id");

Copy link

codecov bot commented Mar 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #87813       +/-   ##
===========================================
+ Coverage   33.09%   87.77%   +54.67%     
===========================================
  Files        8433     9982     +1549     
  Lines      470146   564343    +94197     
  Branches    22274    22166      -108     
===========================================
+ Hits       155608   495328   +339720     
+ Misses     314103    68599   -245504     
+ Partials      435      416       -19     

@snigdhas snigdhas marked this pull request as ready for review March 25, 2025 18:14
@snigdhas snigdhas requested a review from a team as a code owner March 25, 2025 18:14
@snigdhas snigdhas requested a review from a team March 25, 2025 18:14
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

I thought the plan here was to use activity to figure this all out?

Will this end up creating an extra row for each group that is created?

Comment on lines 33 to 35
data_condition = FlexibleForeignKey(
"workflow_engine.DataCondition", null=True, on_delete=models.SET_NULL
)
Copy link
Member

Choose a reason for hiding this comment

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

What will this be used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

we'll need this for the short-term to continue support existing Incident-related endpoints while we give users a heads up and time to make any changes. We can remove the column when we remove those endpoints.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really follow what we're using it for though, how are we joining here?

Copy link
Member

Choose a reason for hiding this comment

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

The longer writeup is here but basically we will be updating the existing rule, alert rule, and incident serializers to read from the new ACI tables as much as possible but the main sticking point is that we need to continue returning the correct IDs for the aforementioned models.

For this specifically, the one additional piece of tricky data is the incident status returned in the serializer response here. The only way I've been able to come up with us recreating this (outside of temporarily duplicating the column on this model) is to have a FK to DataCondition so we can infer the status based on the data condition (for example, a data condition with a condition result of DetectorPriorityLevel.HIGH is the equivalent to a critical trigger today).

I'm completely open to other ideas btw

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed in slack and we should be able to use GroupOpenPeriod -> Group -> Group.priority or GroupOpenPeriod -> Activity (type=SET_PRIORITY) instead of the FK

date_started = models.DateTimeField(default=timezone.now)
date_ended = models.DateTimeField(null=True)

data: models.Field[dict[str, Any] | None, dict[str, Any]] = GzippedDictField(null=True)
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to store values that are pickled? Generally I wouldn't recommend adding more fields like this. Pickled objects in the database is fragile. You could use models.JsonField instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha, updated to models.JsonField

Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0849_add_group_open_periods.py ()

--
-- Create model GroupOpenPeriod
--
CREATE TABLE "sentry_groupopenperiod" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "user_id" bigint NULL, "date_started" timestamp with time zone NOT NULL, "date_ended" timestamp with time zone NULL, "data" jsonb NOT NULL, "data_condition_id" bigint NULL, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "resolution_activity_id" bigint NULL);
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_data_condition_id_ac18ad82_fk_workflow_" FOREIGN KEY ("data_condition_id") REFERENCES "workflow_engine_datacondition" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_data_condition_id_ac18ad82_fk_workflow_";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr" FOREIGN KEY ("group_id") REFERENCES "sentry_groupedmessage" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id" FOREIGN KEY ("project_id") REFERENCES "sentry_project" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac" FOREIGN KEY ("resolution_activity_id") REFERENCES "sentry_activity" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac";
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_user_id_807f0152" ON "sentry_groupopenperiod" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_data_condition_id_ac18ad82" ON "sentry_groupopenperiod" ("data_condition_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_group_id_f4411413" ON "sentry_groupopenperiod" ("group_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_project_id_5ef8bacc" ON "sentry_groupopenperiod" ("project_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_resolution_activity_id_cc5cc897" ON "sentry_groupopenperiod" ("resolution_activity_id");

@snigdhas
Copy link
Member Author

I thought the plan here was to use activity to figure this all out?

It was, but we'll be needing open periods in multiple places and as something we can easily query for the issue stream, the issue details graph and the datepicker in issue details. We'll also need the open periods to support existing Incident APIs. With all of that, it makes more sense to be able to query the lastest open period and update it as needed, and this should give us more nuanced info for each open period as we built it out.

Will this end up creating an extra row for each group that is created?

Yea it will -- it's not ideal given we have grouphistory and activity which both serve similar purposes but this feels like the cleanest way to support open periods more natively without having to query and recalculate open periods multiple times.

Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/migrations/0852_add_group_open_periods.py ()

--
-- Create model GroupOpenPeriod
--
CREATE TABLE "sentry_groupopenperiod" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "user_id" bigint NULL, "date_started" timestamp with time zone NOT NULL, "date_ended" timestamp with time zone NULL, "data" jsonb NOT NULL, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "resolution_activity_id" bigint NULL);
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr" FOREIGN KEY ("group_id") REFERENCES "sentry_groupedmessage" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id" FOREIGN KEY ("project_id") REFERENCES "sentry_project" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac" FOREIGN KEY ("resolution_activity_id") REFERENCES "sentry_activity" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac";
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_user_id_807f0152" ON "sentry_groupopenperiod" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_group_id_f4411413" ON "sentry_groupopenperiod" ("group_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_project_id_5ef8bacc" ON "sentry_groupopenperiod" ("project_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_resolution_activity_id_cc5cc897" ON "sentry_groupopenperiod" ("resolution_activity_id");

Copy link
Contributor

github-actions bot commented Mar 27, 2025

This PR has a migration; here is the generated SQL for src/sentry/migrations/0853_add_group_open_periods.py ()

--
-- Creates extension btree_gist
--
CREATE EXTENSION IF NOT EXISTS "btree_gist";
--
-- Create model GroupOpenPeriod
--
CREATE TABLE "sentry_groupopenperiod" ("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, "user_id" bigint NULL, "date_started" timestamp with time zone NOT NULL, "date_ended" timestamp with time zone NULL, "data" jsonb NOT NULL, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "resolution_activity_id" bigint NULL, CONSTRAINT "exclude_open_period_overlap" EXCLUDE USING GIST ((TSTZRANGE("date_started", "date_ended", '[)')) WITH &&));
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr" FOREIGN KEY ("group_id") REFERENCES "sentry_groupedmessage" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_group_id_f4411413_fk_sentry_gr";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id" FOREIGN KEY ("project_id") REFERENCES "sentry_project" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperiod_project_id_5ef8bacc_fk_sentry_project_id";
ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac" FOREIGN KEY ("resolution_activity_id") REFERENCES "sentry_activity" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "sentry_groupopenperiod" VALIDATE CONSTRAINT "sentry_groupopenperi_resolution_activity__cc5cc897_fk_sentry_ac";
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_user_id_807f0152" ON "sentry_groupopenperiod" ("user_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_group_id_f4411413" ON "sentry_groupopenperiod" ("group_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_project_id_5ef8bacc" ON "sentry_groupopenperiod" ("project_id");
CREATE INDEX CONCURRENTLY "sentry_groupopenperiod_resolution_activity_id_cc5cc897" ON "sentry_groupopenperiod" ("resolution_activity_id");
CREATE INDEX CONCURRENTLY "sentry_grou_group_i_4bffd0_idx" ON "sentry_groupopenperiod" ("group_id", "date_started");

Comment on lines +28 to +29
date_started = models.DateTimeField(default=timezone.now)
date_ended = models.DateTimeField(null=True)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to guard against overlapping open periods?

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL about ExclusionConstraint - not quite sure if I've set it up right

models.Index(fields=("group", "date_started")),
)
constraints = (
ExclusionConstraint(
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@snigdhas snigdhas merged commit 7111b1d into master Mar 28, 2025
48 checks passed
@snigdhas snigdhas deleted the snigdha/add-open-periods branch March 28, 2025 20:12
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.

4 participants