chore(slack): Use Slack SDK for SlackEventEndpoints #73333
Conversation
github-actions
bot
added
the
Scope: Backend
| client.post("/chat.postMessage", data=payload, json=True) | ||
| except ApiError as e: | ||
| logger.error("slack.event.on-message-error", extra={"error": str(e)}) | ||
| if options.get("slack.event-endpoint-sdk"): |
There was a problem hiding this comment.
I am using an option here which isn't scoped to an org b/c adding an extra 1/2 RPC call will take longer and i am afraid we will start timing out.
| # An unfurl may have multiple links to unfurl | ||
| for item in data.get("links", []): | ||
| try: | ||
| url = item["url"] | ||
| slack_shared_link = parse_link(url) |
There was a problem hiding this comment.
deleted this b/c it created a log for every unfurl link which is expensive and we don't track this currently.
| client.post("/chat.postMessage", data=payload, json=True) | ||
| except ApiError as e: | ||
| logger.error("slack.event.on-message-error", extra={"error": str(e)}) | ||
| if options.get("slack.event-endpoint-sdk"): |
| client.chat_unfurl( | ||
| channel=data["channel"], | ||
| ts=data["message_ts"], | ||
| unfurls=orjson.dumps(results).decode(), |
There was a problem hiding this comment.
instead of dumping twice, we could also store this in a variable
| @patch( | ||
| "sentry.integrations.slack.webhooks.event.match_link", | ||
| # match_link will be called twice, for each our links. Resolve into | ||
| # two unique links and one duplicate. | ||
| side_effect=[ | ||
| (LinkType.DISCOVER, {"arg1": "value1"}), | ||
| (LinkType.DISCOVER, {"arg1", "value2"}), | ||
| (LinkType.DISCOVER, {"arg1": "value1"}), | ||
| ], | ||
| ) | ||
| @patch("sentry.integrations.slack.requests.event.has_discover_links", return_value=True) | ||
| @patch( | ||
| "sentry.integrations.slack.webhooks.event.link_handlers", | ||
| { | ||
| LinkType.DISCOVER: Handler( | ||
| matcher=[re.compile(r"test")], | ||
| arg_mapper=make_type_coercer({}), | ||
| fn=Mock(return_value={"link1": "unfurl", "link2": "unfurl"}), | ||
| ) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
if you have the same patch decorators for most functions in a test class, you can move them to the top of the class and it will apply to every function
There was a problem hiding this comment.
ill do this when i ga and delete the old client tests
iamrajjoshi
force-pushed
the
raj/mes-ref/slack/sdk-2.18
branch
from
d256c52
to
0840525
Compare
| if options.get("slack.event-endpoint-sdk") and slack_request.integration.id in options.get( | ||
| "slack.event-endpoint-sdk-integration-ids" | ||
| ): |
There was a problem hiding this comment.
i think this should be OR
either the feature is GA'd OR the org is part of LA.
can you add a test for just the LA part?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #73333 +/- ##
========================================
Coverage 78.03% 78.04%
========================================
Files 6640 6644 +4
Lines 297033 297167 +134
Branches 51152 51137 -15
========================================
+ Hits 231797 231910 +113
- Misses 58961 58985 +24
+ Partials 6275 6272 -3
|
| channel=slack_request.channel_id, | ||
| user=slack_request.user_id, | ||
| text=payload["text"], | ||
| **SlackPromptLinkMessageBuilder(associate_url).as_payload(), |
There was a problem hiding this comment.
nit: this message builder only returns {"blocks": ...}, can we just get the blocks out of the payload instead of building them again?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Updated SlackEventEndpoint to use the the SlackSdkClient.