spike(aci): update and serialize project options attached to ErrorDetector #81388
Conversation
github-actions
bot
added
the
Scope: Backend
❌ 899 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| return attrs | ||
|
|
||
| def serialize(self, obj: Detector, attrs: Mapping[str, Any], user, **kwargs) -> dict[str, Any]: | ||
| return { |
There was a problem hiding this comment.
fyi - it might be nice to call the parent serialize here so we can reuse the data shape in the API. ideally we have a standardized detector serialized model as well for consistency in the UI.
| return self.project.get_option(key, default=default, validate=validate) | ||
|
|
||
|
|
||
| class ErrorDetector(Detector): |
There was a problem hiding this comment.
nit: i think we'd want to move this class out of this file, my guess is we're eventually going to have a lot of detectors.
|
|
||
| @property | ||
| def CONFIG_SCHEMA(self) -> dict[str, Any]: | ||
| return {} |
There was a problem hiding this comment.
plz fill out for real pr :)
| @@ -105,3 +106,28 @@ def detector_handler(self) -> DetectorHandler | None: | |||
| def get_audit_log_data(self) -> dict[str, Any]: | |||
| # TODO: Create proper audit log data for the detector, group and conditions | |||
| return {} | |||
|
|
|||
| def get_option( | |||
There was a problem hiding this comment.
this method should probably only live on ErrorDetector (or just make the query wherever it's being used)
| organization_id=self.context["project"].organization_id, | ||
| name=validated_data["name"], | ||
| # no workflow_condition_group | ||
| type=validated_data["group_type"].slug, |
There was a problem hiding this comment.
Since this is a proxy, and we'll need to re-lookup error detectors based on this field later, i think this would need to be the enum value for error detector (Rather than the group_type slug)
(when processing the workflows, i lookup error detectors by type + project id, since the event won't be able to include the detector id)
There was a problem hiding this comment.
i think this is a leftover from the metric alert detector, but we can change it for all of them if necessary
| @@ -67,14 +68,18 @@ class GroupingConfigLoader: | |||
|
|
|||
| cache_prefix: str # Set in subclasses | |||
|
|
|||
| def get_config_dict(self, project: Project) -> GroupingConfig: | |||
| def get_config_dict(self, project: Project, detector: Detector | None = None) -> GroupingConfig: | |||
There was a problem hiding this comment.
it seems like we're passing around the detector a bit here - is there anything we can do to reduce the blast radius (diff size)? Do we need to pass it to both get_config_id and get enhancements? could we add another method that takes the detector info an handles all this?
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
Replaced by #82533 |
Leave the underlying usage of project options for grouping and auto-resolve. But collect them for serializing an error detector and updating an error detector via the API.