feat(integrations): Introduce common abstraction for messaging SLOs #77820
Conversation
github-actions
bot
added
the
Scope: Backend
| arg_input = cmd_input.adjust(slug) | ||
| return callback(arg_input) | ||
| @dataclass(frozen=True) | ||
| class CandidateHandler: |
There was a problem hiding this comment.
If you're wondering why I've nested this class definition in such a strange place, it's so that R can have the same meaning inside and out. The alternative is kind of ugly: see 07b5401.
|
|
||
| elif self.custom_id.startswith(CustomIds.ASSIGN): | ||
| logger.info( | ||
| "discord.interaction.component.assign", | ||
| extra={**logging_data, "assign_to": self.request.get_selected_options()[0]}, | ||
| ) | ||
| return self.assign() | ||
| with record_event(MessagingInteractionType.ASSIGN).capture(): |
There was a problem hiding this comment.
Is there ever a scenario where we wouldn't want to record the event type? I guess I'm curious if we can make this logic more generic now that we have a MessagingInteractionType enum to key off of.
There was a problem hiding this comment.
I don't know. I'm thinking not? It depends on what we log in the MessagingInteractionEvent.record_* methods.
If there's a use case for logging "an interaction happened where we know the integration but not what the user did", then sure, we could make this optional.
There was a problem hiding this comment.
I think I phrased this poorly. I'm asking if we need to manually wrap every interaction we want to record a metric for with a capture context manager, vs finding some way to automate this and capture metrics for every handled messaging interaction type instead.
There was a problem hiding this comment.
I see. Are you envisioning, like, one flow for determining what action/command is being executed, which is common to all the messaging integrations and outputs the interaction type? That's more or less what I spiked on earlier this week and decided against trying to tackle right now.
Some gestures in that direction include linking MessagingIntegrationCommands to them, and this mapping onto an existing enum (albeit for only one integration).
| @@ -9,11 +9,13 @@ | |||
| from sentry.models.notificationaction import ActionService | |||
| from sentry.rules.actions import IntegrationEventAction | |||
|
|
|||
| PROVIDER = "slack" | |||
There was a problem hiding this comment.
Mainly curious about this, but is there any chance someone can accidentally pollute their imports with this being identical between specs?
There was a problem hiding this comment.
Er, yes, very much so. 😅
Instead of referring to this constant, it might be safer in other places to just write SlackMessagingSpec().provider_slug, which is clearer but also more semantically noisy. (It looks weird at first glance to instantiate the class just to retrieve a single property. There's an argument for making provider_slug a class variable, but then we couldn't annotate it as an @abstactmethod in the same way.)
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## messaging-command-abstraction #77820 +/- ##
===============================================================
Coverage 78.09% 78.10%
===============================================================
Files 6980 6981 +1
Lines 309812 309913 +101
Branches 50710 50732 +22
===============================================================
+ Hits 241957 242062 +105
+ Misses 61381 61379 -2
+ Partials 6474 6472 -2 |
This is a preliminary sketch of a way to have consistent metrics on actions fired from messaging integrations. The goal is to unify both the different messaging integrations (Slack, MsTeams, Discord) and the legacy differences in ways of firing the actions (chat commands and webhook actions).
MessagingInteractionTypeis our new inventory of, roughly, every kind of user action on which we might want to establish SLOs.MessagingInteractionEventis an abstraction over metrics on those actions.For the chat commands, we exploit the new common dispatcher introduced in #77169, meaning we only have to set up
MessagingInteractionEvents in that one place. For the webhook actions, each of the three messaging integrations has a different place where we have to add new code.Left to do (maybe or not in this PR):
MessagingInteractionEvent.record_*methods, which will probably just bemetrics.incr. The only big question is whether to reuse (and move) existing keys, or just create a new namespace (sentry.integrations.slo.*perhaps).MessagingInteractionTypes.