chore(integrations): Add metrics coverage for threaded replies#79034
Conversation
github-actions
Bot
added
the
Scope: Backend
| thread_ts = get_thread_ts(lifecycle) | ||
|
|
||
| # If this flow is triggered again for the same issue, we want it to be seen in the main channel | ||
| reply_broadcast = thread_ts is not None |
There was a problem hiding this comment.
This simplification (settings reply_broadcast based on whether we got a non-null thread_ts rather than as part of the if-else flow) assumes that we would never expect to get a null value for parent_notification_message.message_identifier in line 157 of the old solution. This is possible according to the type annotations on IssueAlertNotificationMessage but I don't think it can happen in practice.
The comment seems to back this up (we want to broadcast if and only if we are replying to a thread) so AFAICT this would be more correct behavior anyway in the case where parent_notification_message.message_identifier is null.
There was a problem hiding this comment.
I agree with your change.
but I don't think it can happen in practice
Is the case where parent is not valid anymore because of slack retention reasons one of that? Or this parent is only a sentry object.
There was a problem hiding this comment.
Good question. I don't think so? My assumption would be that self._repository.get_parent_notification_message would return a null value if the message had been deleted. My changes would affect the value of reply_broadcast only if a non-null parent_notification_message value were returned, but with a null message_identifier attribute.
Or this parent is only a sentry object.
Could you elaborate? Would that be a case where parent_notification_message.message_identifier is None, to your knowledge?
Regardless, I'm fairly confident in saying that reply_broadcast = thread_ts is not None is the business logic we want anyway.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #79034 +/- ##
=======================================
Coverage 78.29% 78.29%
=======================================
Files 7128 7128
Lines 313810 313816 +6
Branches 51226 51226
=======================================
+ Hits 245682 245690 +8
- Misses 61697 61699 +2
+ Partials 6431 6427 -4 |
| thread_ts = get_thread_ts(lifecycle) | ||
|
|
||
| # If this flow is triggered again for the same issue, we want it to be seen in the main channel | ||
| reply_broadcast = thread_ts is not None |
There was a problem hiding this comment.
I agree with your change.
but I don't think it can happen in practice
Is the case where parent is not valid anymore because of slack retention reasons one of that? Or this parent is only a sentry object.
| self._extra.update(extra) | ||
| self._terminate(EventLifecycleOutcome.FAILURE, exc) | ||
|
|
||
| def record_halt(self, exc: BaseException | None = None) -> None: |
There was a problem hiding this comment.
I think unfortunately we have more cases like this. Can you share this broadly with the team so they can reuse it in their integration code?
| and rule_id | ||
| ): | ||
| parent_notification_message = None | ||
| def get_thread_ts(lifecycle: EventLifecycle) -> str | None: |
There was a problem hiding this comment.
Are there any existing tests covering this?
There was a problem hiding this comment.
Yep, the methods with names beginning "test_after" in tests.sentry.integrations.slack.actions.notification.test_slack_notify_service_action.TestInit.
Refactor flow in SlackNotifyServiceAction to accommodate a metrics context around the code where we find a Slack message to reply to in a thread. Make it easier to explicitly mark a "halted" outcome in an EventLifecycle.
2 participants
Refactor flow in SlackNotifyServiceAction to accommodate a metrics context around the code where we find a Slack message to reply to in a thread.
Make it easier to explicitly mark a "halted" outcome in an EventLifecycle.