feat(ACI): Detector details PUT endpoint #84098

ceorourke · 2025-01-27T19:10:24Z

Create a PUT endpoint for detector details.

Addresses https://getsentry.atlassian.net/browse/ACI-114

codecov · 2025-01-27T19:28:40Z

Codecov Report

Attention: Patch coverage is 93.86503% with 10 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...gine/endpoints/validators/metric_alert_detector.py	85.29%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #84098      +/-   ##
==========================================
+ Coverage   87.97%   87.99%   +0.01%     
==========================================
  Files        9631     9632       +1     
  Lines      543870   545756    +1886     
  Branches    21089    21089              
==========================================
+ Hits       478492   480241    +1749     
- Misses      65075    65212     +137     
  Partials      303      303

src/sentry/incidents/endpoints/validators.py

src/sentry/workflow_engine/endpoints/project_detector_details.py

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

cathteng · 2025-02-13T23:36:29Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

+    def update_data_source(self, instance, data_source):
+        try:
+            source_instance = DataSource.objects.get(detector=instance)
+        except DataSource.DoesNotExist:


does the data source need to be created if you add it to an existing detector?

I am not sure if that scenario would be possible in the UI, I believe you're always creating both a data source and a detector at the same time

cathteng · 2025-02-13T23:37:37Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

+    def update(self, instance, validated_data):
+        instance.name = validated_data.get("name", instance.name)
+        instance.type = validated_data.get("detector_type", instance.group_type).slug
+        condition_group = validated_data.pop("condition_group")


nit: is there a reason this is pop instead of get?

🤔 I wanna say I had a reason at one point but I don't think it applies anymore

oh wait the reason is because when I call instance.save() the instance here is a Detector and if I have the dict of condition group data that's bad cause it's not data that gets saved to that model

🤔 should this be the Detector.workflow_condition_group or is this just because of the API -> models mapping?

What gets passed here are the data conditions, the condition group doesn't change

saponifi3d

taking a step back, is it an anti-pattern to be creating new models in the validator?

just by the naming i would expect a validator to only ensure data exists as expected, not create new models. i'm not super familiar with this layer, so it's def more of a smell than anything for me.

saponifi3d · 2025-02-19T18:51:27Z

src/sentry/workflow_engine/endpoints/project_detector_details.py

+            404: RESPONSE_NOT_FOUND,
+        },
+    )
+    def put(self, request: Request, project: Project, detector: Detector) -> Response:


i really like how clean these api handlers are so clean and well documented 💯

tests/sentry/workflow_engine/endpoints/test_project_detector_details.py

saponifi3d · 2025-02-19T19:24:57Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

@@ -1,12 +1,17 @@
+from datetime import timedelta


once this pr is merged, mind if i move this validator to the incidents directory to colocate it with the detector it's validating?

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

saponifi3d · 2025-02-19T20:34:07Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

+            query_type=data_source.get("query_type", snuba_query.type),
+            dataset=data_source.get("dataset", snuba_query.dataset),
+            query=data_source.get("query", snuba_query.query),
+            aggregate=data_source.get("aggregate", snuba_query.aggregate),
+            time_window=timedelta(minutes=data_source.get("time_window", snuba_query.time_window)),
+            resolution=timedelta(seconds=data_source.get("resolution", snuba_query.resolution)),
+            environment=data_source.get("environment", snuba_query.environment),
+            event_types=data_source.get("event_types", [event_types]),


🤔 the data_source won't have these attributes, it's just the lookup like you have on line 87.

is this like new_snuba_query rather than data_source?

I think I based this off of https://github.com/getsentry/sentry/blob/master/tests/sentry/workflow_engine/endpoints/test_project_detector_index.py#L62-L70 and https://www.notion.so/sentry/UI-APIs-1888b10e4b5d80dfa47fcbffa2a7c9af?pvs=4#1928b10e4b5d801da849e25bc24915f3 - we'd need all of this information if the snuba query is changing. Do you feel strongly about renaming it? I'll have to check with Nate to make sure he's aware that it'll be changing.

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

saponifi3d · 2025-02-19T20:38:06Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

+    def update(self, instance, validated_data):
+        instance.name = validated_data.get("name", instance.name)
+        instance.type = validated_data.get("detector_type", instance.group_type).slug
+        condition_group = validated_data.pop("condition_group")


🤔 should this be the Detector.workflow_condition_group or is this just because of the API -> models mapping?

saponifi3d

lgtm - we can update the nits in upcoming work.

saponifi3d · 2025-02-21T21:19:41Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

+class DataConditionType(TypedDict):
+    id: int | None
+    comparison: int
+    type: Condition
+    condition_result: DetectorPriorityLevel
+    condition_group_id: int


🤔 should this be in workflow_engine/types.py? (we can update later)

saponifi3d · 2025-02-21T21:21:00Z

src/sentry/workflow_engine/endpoints/validators/metric_alert_detector.py

+    condition_group_id: int
+
+
+class DataSourceType(TypedDict):


Since this seems to be specific to snuba query's as the data source:

Suggested change

class DataSourceType(TypedDict):

class SnubaQueryDataSourceType(TypedDict):

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 27, 2025

vercel bot deployed to Preview January 27, 2025 19:13 View deployment

vercel bot deployed to Preview January 27, 2025 19:31 View deployment

ceorourke commented Jan 27, 2025

View reviewed changes

src/sentry/incidents/endpoints/validators.py Outdated Show resolved Hide resolved

ceorourke commented Jan 27, 2025

View reviewed changes

src/sentry/incidents/endpoints/validators.py Outdated Show resolved Hide resolved

ceorourke marked this pull request as ready for review January 27, 2025 20:02

ceorourke requested review from a team as code owners January 27, 2025 20:02

vercel bot deployed to Preview January 27, 2025 20:02 View deployment

vercel bot deployed to Preview January 27, 2025 20:25 View deployment

ceorourke marked this pull request as draft January 27, 2025 20:42

ceorourke commented Jan 27, 2025

View reviewed changes

src/sentry/incidents/endpoints/validators.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview January 27, 2025 21:28 View deployment

vercel bot deployed to Preview January 27, 2025 22:15 View deployment

ceorourke force-pushed the ceorourke/detector-details-put branch from 394cdec to 4330776 Compare February 10, 2025 23:52

vercel bot deployed to Preview February 11, 2025 00:02 View deployment

ceorourke force-pushed the ceorourke/detector-details-put branch from 3137949 to ec4b7fe Compare February 11, 2025 01:18

vercel bot deployed to Preview February 11, 2025 01:25 View deployment

ceorourke force-pushed the ceorourke/detector-details-put branch from 22b212a to c9992a3 Compare February 12, 2025 00:48

vercel bot deployed to Preview February 12, 2025 00:52 View deployment

ceorourke force-pushed the ceorourke/detector-details-put branch from c9992a3 to 2ab2d9c Compare February 12, 2025 18:46

vercel bot deployed to Preview February 12, 2025 18:55 View deployment

ceorourke force-pushed the ceorourke/detector-details-put branch from 449820d to 12075bf Compare February 12, 2025 19:14

vercel bot deployed to Preview February 12, 2025 19:19 View deployment

ceorourke mentioned this pull request Feb 12, 2025

chore(ACI): Update validators to handle actual payload format #85110

Merged

ceorourke added 4 commits February 13, 2025 12:14

feat(aci): Add detector details PUT endpoint

d2b4918

update condition group

eb61228

handle updating condition group

03fd1be

update data source

0fc9389

vercel bot deployed to Preview February 13, 2025 22:32 View deployment

cathteng reviewed Feb 13, 2025

View reviewed changes

typing

0fd2617

vercel bot deployed to Preview February 14, 2025 18:15 View deployment

wait this is just a str

e484463

vercel bot deployed to Preview February 14, 2025 18:51 View deployment

use .save() to trigger enforce_comparison_schema

72fd31d

vercel bot deployed to Preview February 14, 2025 21:34 View deployment

a desperate attempt to make this test pass in ci

a6772de

vercel bot deployed to Preview February 14, 2025 22:14 View deployment

saponifi3d reviewed Feb 19, 2025

View reviewed changes

some typing

b98a85b

vercel bot deployed to Preview February 20, 2025 23:25 View deployment

more typing

202e68c

vercel bot deployed to Preview February 20, 2025 23:44 View deployment

update test

bbf8d0d

vercel bot deployed to Preview February 20, 2025 23:55 View deployment

desperate attempt to see if I can get this test to pass in CI

e1bfc29

vercel bot deployed to Preview February 21, 2025 00:20 View deployment

Well that didn't help

ab367e4

vercel bot deployed to Preview February 21, 2025 17:38 View deployment

another attempt

e1f49f8

vercel bot deployed to Preview February 21, 2025 17:47 View deployment

forgot to look up through querysubscription

34d9af0

vercel bot deployed to Preview February 21, 2025 19:05 View deployment

ceorourke requested a review from saponifi3d February 21, 2025 19:31

saponifi3d approved these changes Feb 21, 2025

View reviewed changes

ceorourke merged commit 4a76908 into master Feb 21, 2025
49 checks passed

ceorourke deleted the ceorourke/detector-details-put branch February 21, 2025 21:24

ceorourke mentioned this pull request Feb 21, 2025

chore(ACI): Metric alert detector nits #85692

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ACI): Detector details PUT endpoint #84098

feat(ACI): Detector details PUT endpoint #84098

ceorourke commented Jan 27, 2025

codecov bot commented Jan 27, 2025 •

edited

Loading

cathteng Feb 13, 2025

ceorourke Feb 14, 2025

cathteng Feb 13, 2025

ceorourke Feb 14, 2025

ceorourke Feb 14, 2025

saponifi3d Feb 19, 2025 •

edited

Loading

ceorourke Feb 20, 2025

saponifi3d left a comment

saponifi3d Feb 19, 2025

saponifi3d Feb 19, 2025

saponifi3d Feb 19, 2025

ceorourke Feb 20, 2025

saponifi3d Feb 19, 2025 •

edited

Loading

saponifi3d left a comment

saponifi3d Feb 21, 2025

saponifi3d Feb 21, 2025

	class DataSourceType(TypedDict):
	class SnubaQueryDataSourceType(TypedDict):

feat(ACI): Detector details PUT endpoint #84098

feat(ACI): Detector details PUT endpoint #84098

Conversation

ceorourke commented Jan 27, 2025

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2025 •

edited

Loading

saponifi3d Feb 19, 2025 •

edited

Loading

saponifi3d Feb 19, 2025 •

edited

Loading