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

Create Detector State Model #78244

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

saponifi3d
Copy link
Contributor

Description

Add the detector state model.

This model is used to track the state of the detector for Alerts Create Issues

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Sep 27, 2024
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Copy link
Contributor

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

--
-- Create model DetectorState
--
CREATE TABLE "workflow_engine_detectorstate" ("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, "detector_group_key" varchar(200) NULL, "status" varchar(200) NOT NULL, "state" varchar(200) NULL, "detector_id" bigint NOT NULL);
ALTER TABLE "workflow_engine_detectorstate" ADD CONSTRAINT "workflow_engine_dete_detector_id_72d03dd7_fk_workflow_" FOREIGN KEY ("detector_id") REFERENCES "workflow_engine_detector" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_detectorstate" VALIDATE CONSTRAINT "workflow_engine_dete_detector_id_72d03dd7_fk_workflow_";
CREATE INDEX CONCURRENTLY "workflow_engine_detectorstate_detector_id_72d03dd7" ON "workflow_engine_detectorstate" ("detector_id");

@saponifi3d saponifi3d marked this pull request as ready for review September 27, 2024 04:20
@saponifi3d saponifi3d requested review from a team as code owners September 27, 2024 04:20
@saponifi3d saponifi3d requested a review from a team September 27, 2024 15:31
class DetectorState(DefaultFieldsModel):
__relocation_scope__ = RelocationScope.Organization

class Type(models.TextChoices):
Copy link
Member

Choose a reason for hiding this comment

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

nit: StatusType? assuming there will be some kind of Type var for state too

@saponifi3d saponifi3d marked this pull request as draft September 27, 2024 16:50
Comment on lines 24 to 27
status = models.CharField(max_length=200, choices=Type.choices)

# The current state of the detector
state = models.CharField(max_length=200, blank=True, 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.

Is the idea here that for a metric alert, we'd have

status: active, state: warning
status: active, state: critical
status: inactive, state: null?

or status:ok/warning/critical?

Copy link
Contributor Author

@saponifi3d saponifi3d Sep 27, 2024

Choose a reason for hiding this comment

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

The former, or close to it. i was thinking:

status: active, state: warning
status: active, state: critical
status: inactive, state: ok

I was trying to design for a bunch of cases tho.. for performance issues, i was thinking:

status:inactive, state: ok
status:active, state: detected_n+1

Since those have an autoresolve, we could flip the status to inactive and leave the state:

status: inactive, state: detected_n+1

that could give us some other insights, and if we keep seeing the issue not be addressed we could add an escalation policy.

An escalation policy could be something like...

if status inactive -> active with !ok state
    update redis counter by detector / id.

if redis counter > escalation number
    trigger escalation

it also allows us to easily evaluate any kind of detector by the status without having to check it's state - but if we want to simplify for now, we could probably create a shared DetectorState.OK enum value. i've just found that a little harder to maintain across multiple teams / products than a required field (🤔 should probably just make it an active boolean though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ pushed a quick update to it that i talked myself into in that comment 🤣 also addresses josh's concern about the status Type name.

Copy link
Contributor

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

--
-- Create model DetectorState
--
CREATE TABLE "workflow_engine_detectorstate" ("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, "detector_group_key" varchar(200) NULL, "active" boolean NOT NULL, "state" varchar(200) NOT NULL, "detector_id" bigint NOT NULL);
ALTER TABLE "workflow_engine_detectorstate" ADD CONSTRAINT "workflow_engine_dete_detector_id_72d03dd7_fk_workflow_" FOREIGN KEY ("detector_id") REFERENCES "workflow_engine_detector" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_detectorstate" VALIDATE CONSTRAINT "workflow_engine_dete_detector_id_72d03dd7_fk_workflow_";
CREATE INDEX CONCURRENTLY "workflow_engine_detectorstate_detector_id_72d03dd7" ON "workflow_engine_detectorstate" ("detector_id");

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/testutils/factories.py 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #78244   +/-   ##
=======================================
  Coverage   78.08%   78.08%           
=======================================
  Files        7067     7069    +2     
  Lines      311797   311831   +34     
  Branches    50958    50963    +5     
=======================================
+ Hits       243464   243491   +27     
- Misses      61988    61993    +5     
- Partials     6345     6347    +2     

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 Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants