feat(alert-rule): Instrument analytics for Alert Rule UI Components #29552

NisanthanNanthakumar · 2021-10-25T18:29:55Z

Objective:

We want to add logging to Big Query to track adoption metrics of Alert Rule UI Components.

src/sentry/tasks/sentry_apps.py

evanpurkhiser · 2021-10-25T18:37:24Z

src/sentry/tasks/sentry_apps.py

@@ -472,4 +471,56 @@ def send_and_save_webhook_request(sentry_app, app_platform_event, url=None):

        resp.raise_for_status()

+        # On success, record BigQuery analytic event


Remember sentry is an open source code base, while we might be using big query, it's not true that on premise would be using this.

So I would recommend avoiding mentioning 'big query' and just say record analytic event

scefali · 2021-10-25T18:53:35Z

src/sentry/analytics/events/alert_rule_ui_component_webhook_sent.py

+    type = "alert_rule_ui_component_webhook.sent"
+
+    attributes = (
+        analytics.Attribute("installed_org_id", type=str, required=True),


@NisanthanNanthakumar Can we just call this organization_id?

@scefali hmm I think its better if its more specific, because organization_id can also refer to the organization receiving the webhook

@NisanthanNanthakumar If you add a new field to represent the organization_id, you're going to have to update our ETL pipeline so we can read the right field when we push to Amplitude:
https://github.com/getsentry/etl/blob/master/etl/operators/amplitude_insert_analytics_events.py#L77-L80

Also, it makes it harder for analytics if some events don't have an organization_id matching our expected field name. Tilllman and other data folks will have to add special logic for just this event. Unless the event itself has two different organizations, then I would highly recommend just using organization_id to represent the org

@scefali 👍 I'll just add a comment then

scefali · 2021-10-25T18:55:21Z

src/sentry/analytics/events/sentry_app_created.py

@@ -8,6 +8,7 @@ class SentryAppCreatedEvent(analytics.Event):
        analytics.Attribute("user_id"),
        analytics.Attribute("organization_id"),
        analytics.Attribute("sentry_app"),
+        analytics.Attribute("created_alert_rule_ui_component", type=bool, required=False),


@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.

@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.

@NisanthanNanthakumar yea...maybe so. But I think my other comment will impact this: #29552 (comment)

scefali · 2021-10-25T18:56:14Z

src/sentry/analytics/events/sentry_app_updated.py

-    attributes = (analytics.Attribute("user_id"), analytics.Attribute("sentry_app"))
+    attributes = (
+        analytics.Attribute("user_id"),
+        analytics.Attribute("sentry_app"),


@NisanthanNanthakumar do we really not have an organization_id here? IMO it's worth adding

@scefali We can get organization_id from sentry_sentryapp.owner_id if we have the sentry_app.id. I think it might be unnecessary when its on the table.

@NisanthanNanthakumar yep, but ETL won't handle this properly when sending to Amplitude unless we change the ETL pipeline

scefali · 2021-10-25T18:57:55Z

src/sentry/api/endpoints/project_rules.py

@@ -143,7 +150,13 @@ def post(self, request, project):
                sender=self,
                is_api_token=request.auth is not None,
            )
-
+            if created_alert_rule_ui_component:
+                analytics.record(


@NisanthanNanthakumar why are we making a new event for this instead of just modifying the existing alert rule event?

@scefali ahh just realized we use signals for analytics too...

scefali · 2021-10-25T18:58:30Z

src/sentry/incidents/endpoints/serializers.py

                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:


@NisanthanNanthakumar Nit: you don't need an else condition here as the other conditions will just raise an exception

scefali · 2021-10-25T19:00:37Z

@NisanthanNanthakumar what about getting these events into Amplitude as well?

scefali · 2021-10-25T19:43:24Z

src/sentry/analytics/events/alert_created.py

@@ -11,6 +11,7 @@ class AlertCreatedEvent(analytics.Event):
        analytics.Attribute("rule_id"),
        analytics.Attribute("rule_type"),
        analytics.Attribute("is_api_token"),
+        analytics.Attribute("has_alert_rule_ui_component", type=str, required=False),


@NisanthanNanthakumar Maybe this could be a type field? Something a bit less specific than has_alert_rule_ui_component which implies a boolean? Not sure...

@scefali what do you mean by type field?

@NisanthanNanthakumar Some sort of field that says why the alert rule was created? So the value could be ui_component in this case. I'm not sure what field name makes sense though.

scefali · 2021-10-25T19:47:14Z

src/sentry/tasks/sentry_apps.py

@@ -472,4 +471,38 @@ def send_and_save_webhook_request(sentry_app, app_platform_event, url=None):

        resp.raise_for_status()

+        # On success, record analytic event
+        # Handle Metric Alerts
+        if app_platform_event.resource == "metric_alert":


@NisanthanNanthakumar I would probably move all of the analytics logic to the caller function which will be different for metric alerts, issue alerts, etc. I think it will be a lot more clean.

@scefali hmm I wanted to record the analytic event after the webhook was successfully sent, incase we get >=400 status codes. Maybe I'll move into a helper function

@NisanthanNanthakumar if you put the analytics recording after calling send_and_save_webhook_request, it wouldn't get called on 400 status codes because we call raise_for_status

mgaeta · 2021-10-25T20:09:41Z

src/sentry/api/endpoints/project_rules.py

@@ -25,6 +25,7 @@
 def trigger_alert_rule_action_creators(
    actions: Sequence[Mapping[str, str]],
 ) -> None:


This returns bool now.

mgaeta · 2021-10-25T20:25:13Z

src/sentry/incidents/endpoints/serializers.py

                trigger=self.context["trigger"], **validated_data
            )
+            analytics.record(


In general I try to keep only lines that can throw in the try/catch block. It prevents other exceptions from being swallowed and it makes it obvious at invocation which line can actually throw.

@mgaeta hmm yea originally I had it in an else block. I can revert.

mgaeta · 2021-10-25T20:30:44Z

src/sentry/mediators/sentry_apps/creator.py

@@ -130,4 +130,8 @@ def record_analytics(self):
            user_id=self.user.id,
            organization_id=self.organization.id,
            sentry_app=self.sentry_app.slug,
+            created_alert_rule_ui_component="True"


Ok this might be overkill but you could make this

created_alert_rule_ui_component=str( any( element["type"] == "alert-rule-action" for element in self.schema.get("elements", []) ) ),

Or make a new function:

def get_schema_types(self) -> Set[str]: return set(element["type"] for element in self.schema.get("elements", []))

so that you can do

created_alert_rule_ui_component=str("alert-rule-action" in self.get_schema_types()),

because you do something similar in the updater.

mgaeta · 2021-10-25T20:37:01Z

src/sentry/mediators/sentry_apps/updater.py

+            "type", flat=True
+        )
+        elements = [element["type"] for element in self.schema.get("elements", [])]
+        return list(set(elements) - set(current))


Yeah might as well return this as a Set.

mgaeta · 2021-10-26T23:23:05Z

src/sentry/rules/actions/notify_event_service.py

+    # Loop through the triggers for the alert rule event. For each trigger, check if an action is an alert rule UI Component
+    alert_rule_action_ui_component = False
+    for trigger in (
+        app_platform_event.get("data", {})


triggers = ( getattr(app_platform_event, "data", {}) .get("metric_alert", {}) .get("alert_rule", {}) .get("triggers", []) ) for trigger in triggers: ...

mgaeta · 2021-10-26T23:28:30Z

src/sentry/rules/actions/notify_event_service.py

+        .get("alert_rule", {})
+        .get("triggers", [])
+    ):
+        alert_rule_action_ui_component = next(


Can you just halt iteration once you've found the first one?

I thinks this makes more sense as a comprehension

actions = [ action for trigger in triggers for action in trigger["actions"] if (action["type"] == "sentry_app" and action["settings"] is not None) ] if not actions: return False return actions[0]

NisanthanNanthakumar requested review from a team October 25, 2021 18:29

evanpurkhiser reviewed Oct 25, 2021

View reviewed changes

src/sentry/tasks/sentry_apps.py Outdated Show resolved Hide resolved

mgaeta reviewed Oct 25, 2021

View reviewed changes

src/sentry/tasks/sentry_apps.py Outdated Show resolved Hide resolved

evanpurkhiser reviewed Oct 25, 2021

View reviewed changes

vercel bot deployed to Preview – sentry October 25, 2021 18:38 View deployment

vercel bot deployed to Preview – storybook October 25, 2021 18:38 View deployment

evanpurkhiser requested a review from a team October 25, 2021 18:38

vercel bot deployed to Preview – storybook October 25, 2021 18:48 View deployment

vercel bot deployed to Preview – sentry October 25, 2021 18:48 View deployment

NisanthanNanthakumar requested a review from a team October 25, 2021 18:48

scefali reviewed Oct 25, 2021

View reviewed changes

vercel bot deployed to Preview – sentry October 25, 2021 19:20 View deployment

vercel bot deployed to Preview – storybook October 25, 2021 19:20 View deployment

vercel bot deployed to Preview – sentry October 25, 2021 19:22 View deployment

vercel bot deployed to Preview – storybook October 25, 2021 19:23 View deployment

NisanthanNanthakumar requested a review from scefali October 25, 2021 19:34

vercel bot deployed to Preview – sentry October 25, 2021 19:41 View deployment

vercel bot deployed to Preview – storybook October 25, 2021 19:41 View deployment

scefali reviewed Oct 25, 2021

View reviewed changes

mgaeta reviewed Oct 25, 2021

View reviewed changes

mgaeta reviewed Oct 26, 2021

View reviewed changes

vercel bot deployed to Preview – storybook October 28, 2021 17:53 View deployment

vercel bot deployed to Preview – sentry October 28, 2021 17:53 View deployment

Nisanthan Nanthakumar added 17 commits October 28, 2021 14:35

feat(alert-rule): Instrument analytics for Alert Rule UI Components

9abc998

cleanup

5def51c

fix comment wording

4c6ec1a

update

3557c42

update

63ce483

add request.user

3c2360c

use mixin

83eb5e1

move analytics logic to the caller function

9b2359b

use a type field for analytics

1cc3fcd

add comment

b9f2ee1

use booleans

13af080

updates

143a422

updates & types

0789f6b

update

a09b5a7

default empty obj

be9ccd1

add empty set case

ed1f756

refactor

fa6924a

NisanthanNanthakumar force-pushed the API-2194 branch from 76a3fcf to fa6924a Compare October 28, 2021 21:35

vercel bot deployed to Preview – storybook October 28, 2021 21:36 View deployment

vercel bot deployed to Preview – sentry October 28, 2021 21:36 View deployment

update tests

3c4b8be

vercel bot deployed to Preview – sentry October 28, 2021 22:16 View deployment

vercel bot deployed to Preview – storybook October 28, 2021 22:16 View deployment

NisanthanNanthakumar merged commit bcc7a1d into master Oct 28, 2021

NisanthanNanthakumar deleted the API-2194 branch October 28, 2021 23:08

github-actions bot locked and limited conversation to collaborators Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(alert-rule): Instrument analytics for Alert Rule UI Components #29552

feat(alert-rule): Instrument analytics for Alert Rule UI Components #29552

NisanthanNanthakumar commented Oct 25, 2021

evanpurkhiser Oct 25, 2021 •

edited

Loading

scefali Oct 25, 2021

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021 •

edited

Loading

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021

scefali Oct 25, 2021

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021

scefali Oct 25, 2021

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021

scefali commented Oct 25, 2021

scefali Oct 25, 2021 •

edited

Loading

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021

scefali Oct 25, 2021

NisanthanNanthakumar Oct 25, 2021

scefali Oct 25, 2021

mgaeta Oct 25, 2021

mgaeta Oct 25, 2021

NisanthanNanthakumar Oct 25, 2021

mgaeta Oct 25, 2021

mgaeta Oct 25, 2021

mgaeta Oct 25, 2021

mgaeta Oct 26, 2021

mgaeta Oct 26, 2021

mgaeta Oct 26, 2021

		@@ -472,4 +471,56 @@ def send_and_save_webhook_request(sentry_app, app_platform_event, url=None):

		resp.raise_for_status()

		# On success, record BigQuery analytic event

feat(alert-rule): Instrument analytics for Alert Rule UI Components #29552

feat(alert-rule): Instrument analytics for Alert Rule UI Components #29552

Conversation

NisanthanNanthakumar commented Oct 25, 2021

Objective:

evanpurkhiser Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scefali Oct 25, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

scefali commented Oct 25, 2021

scefali Oct 25, 2021 • edited Loading

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

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

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

evanpurkhiser Oct 25, 2021 •

edited

Loading

scefali Oct 25, 2021 •

edited

Loading

scefali Oct 25, 2021 •

edited

Loading