-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(alert-rule): Instrument analytics for Alert Rule UI Components #29552
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
Changes from all commits
9abc998
5def51c
4c6ec1a
3557c42
63ce483
3c2360c
83eb5e1
9b2359b
1cc3fcd
b9f2ee1
13af080
143a422
0789f6b
a09b5a7
be9ccd1
ed1f756
fa6924a
3c4b8be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| from sentry import analytics | ||
|
|
||
|
|
||
| class AlertRuleUiComponentWebhookSentEvent(analytics.Event): | ||
| type = "alert_rule_ui_component_webhook.sent" | ||
|
|
||
| attributes = ( | ||
| # organization_id refers to the organization that installed the sentryapp | ||
| analytics.Attribute("organization_id"), | ||
| analytics.Attribute("sentry_app_id"), | ||
| analytics.Attribute("event"), | ||
| ) | ||
|
|
||
|
|
||
| analytics.register(AlertRuleUiComponentWebhookSentEvent) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| from sentry import analytics | ||
|
|
||
|
|
||
| class MetricAlertWithUiComponentCreatedEvent(analytics.Event): | ||
| type = "metric_alert_with_ui_component.created" | ||
|
|
||
| attributes = ( | ||
| analytics.Attribute("user_id", required=False), | ||
| analytics.Attribute("alert_rule_id"), | ||
| analytics.Attribute("organization_id"), | ||
| ) | ||
|
|
||
|
|
||
| analytics.register(MetricAlertWithUiComponentCreatedEvent) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,12 @@ | |
| class SentryAppUpdatedEvent(analytics.Event): | ||
| type = "sentry_app.updated" | ||
|
|
||
| attributes = (analytics.Attribute("user_id"), analytics.Attribute("sentry_app")) | ||
| attributes = ( | ||
| analytics.Attribute("user_id"), | ||
| analytics.Attribute("organization_id"), | ||
| analytics.Attribute("sentry_app"), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NisanthanNanthakumar do we really not have an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @scefali We can get
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NisanthanNanthakumar yep, but ETL won't handle this properly when sending to Amplitude unless we change the ETL pipeline |
||
| analytics.Attribute("created_alert_rule_ui_component", type=bool, required=False), | ||
| ) | ||
|
|
||
|
|
||
| analytics.register(SentryAppUpdatedEvent) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from django.utils.encoding import force_text | ||
| from rest_framework import serializers | ||
|
|
||
| from sentry import analytics | ||
| from sentry.api.serializers.rest_framework.base import CamelSnakeModelSerializer | ||
| from sentry.api.serializers.rest_framework.environment import EnvironmentField | ||
| from sentry.api.serializers.rest_framework.project import ProjectField | ||
|
|
@@ -82,6 +83,7 @@ class AlertRuleTriggerActionSerializer(CamelSnakeModelSerializer): | |
| - `alert_rule`: The alert_rule related to this action. | ||
| - `organization`: The organization related to this action. | ||
| - `access`: An access object (from `request.access`) | ||
| - `user`: The user from `request.user` | ||
| """ | ||
|
|
||
| id = serializers.IntegerField(required=False) | ||
|
|
@@ -216,13 +218,21 @@ def validate(self, attrs): | |
|
|
||
| def create(self, validated_data): | ||
| try: | ||
| return create_alert_rule_trigger_action( | ||
| action = create_alert_rule_trigger_action( | ||
| trigger=self.context["trigger"], **validated_data | ||
| ) | ||
| except InvalidTriggerActionError as e: | ||
| raise serializers.ValidationError(force_text(e)) | ||
| except ApiRateLimitedError as e: | ||
| raise serializers.ValidationError(force_text(e)) | ||
| else: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @NisanthanNanthakumar Nit: you don't need an else condition here as the other conditions will just raise an exception |
||
| analytics.record( | ||
|
||
| "metric_alert_with_ui_component.created", | ||
| user_id=getattr(self.context["user"], "id", None), | ||
| alert_rule_id=getattr(self.context["alert_rule"], "id"), | ||
| organization_id=getattr(self.context["organization"], "id"), | ||
| ) | ||
| return action | ||
|
|
||
| def update(self, instance, validated_data): | ||
| if "id" in validated_data: | ||
|
|
@@ -241,6 +251,7 @@ class AlertRuleTriggerSerializer(CamelSnakeModelSerializer): | |
| - `alert_rule`: The alert_rule related to this trigger. | ||
| - `organization`: The organization related to this trigger. | ||
| - `access`: An access object (from `request.access`) | ||
| - `user`: The user from `request.user` | ||
| """ | ||
|
|
||
| id = serializers.IntegerField(required=False) | ||
|
|
@@ -304,6 +315,7 @@ def _handle_actions(self, alert_rule_trigger, actions): | |
| "trigger": alert_rule_trigger, | ||
| "organization": self.context["organization"], | ||
| "access": self.context["access"], | ||
| "user": self.context["user"], | ||
| "use_async_lookup": self.context.get("use_async_lookup"), | ||
| "validate_channel_id": self.context.get("validate_channel_id"), | ||
| "input_channel_id": action_data.pop("input_channel_id", None), | ||
|
|
@@ -333,6 +345,7 @@ class AlertRuleSerializer(CamelSnakeModelSerializer): | |
| Serializer for creating/updating an alert rule. Required context: | ||
| - `organization`: The organization related to this alert rule. | ||
| - `access`: An access object (from `request.access`) | ||
| - `user`: The user from `request.user` | ||
| """ | ||
|
|
||
| environment = EnvironmentField(required=False, allow_null=True) | ||
|
|
@@ -672,6 +685,7 @@ def _handle_triggers(self, alert_rule, triggers): | |
| "alert_rule": alert_rule, | ||
| "organization": self.context["organization"], | ||
| "access": self.context["access"], | ||
| "user": self.context["user"], | ||
| "use_async_lookup": self.context.get("use_async_lookup"), | ||
| "input_channel_id": self.context.get("input_channel_id"), | ||
| "validate_channel_id": self.context.get("validate_channel_id"), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| from typing import Set | ||
|
|
||
|
|
||
| class SentryAppMixin: | ||
| def get_schema_types(self) -> Set[str]: | ||
| return {element["type"] for element in (self.schema or {}).get("elements", [])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NisanthanNanthakumar can we make this field a string instead of a boolean? That way if we ever need a third type to distinguish, we don't need a new field and can just make a new value for the string. Booleans are generally not ideal for analytics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scefali yea Im ok with making it a string type. But the name seems pretty binary, idk if other values can fit without being confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NisanthanNanthakumar yea...maybe so. But I think my other comment will impact this: #29552 (comment)