chore(slack): introduce SlackClient using slack_sdk #72017
Conversation
github-actions
bot
added
the
Scope: Backend
| integration = Integration.objects.filter(id=integration_id).first() | ||
|
|
||
| if integration is None: | ||
| raise ValueError(f"Integration with id {integration_id} not found") |
There was a problem hiding this comment.
we don't raise any errors if initialization fails in the existing SlackClient. all subsequent calls are no-ops, but i think we should fail loudly so we're aware something is broken
|
|
||
| with pytest.raises(SlackApiError): | ||
| # error raised because it's actually trying to POST | ||
| client.chat_postMessage(channel="#announcements", text="hello") |
There was a problem hiding this comment.
i was having difficulty mocking responses with the Slack SDK client. it seems like we might have to do something like this slackapi/python-slack-sdk#1188 (comment)
There was a problem hiding this comment.
ok i got something working here, please yell loudly if this is not the way to do it
cathteng
force-pushed
the
cathy/simple-slack-sdk-client
branch
from
08df208
to
894a64f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72017 +/- ##
===========================================
+ Coverage 56.86% 77.93% +21.06%
===========================================
Files 6561 6572 +11
Lines 292452 292863 +411
Branches 50480 50550 +70
===========================================
+ Hits 166309 228243 +61934
+ Misses 121415 58369 -63046
- Partials 4728 6251 +1523
|
| if payload_blocks := blocks.get("blocks"): | ||
| payload["blocks"] = orjson.dumps(payload_blocks).decode() | ||
| json_blocks = orjson.dumps(payload_blocks).decode() |
There was a problem hiding this comment.
Is there a test covering this line of code? After #inc-779 I get scared whenever I see orjson.dumps
There was a problem hiding this comment.
i added testing to this area of the code in #72066, it should be covered now
| # temporarily log the payload so we can debug message failures | ||
| log_params["payload"] = orjson.dumps(payload).decode() | ||
|
|
||
| self.logger.info( |
There was a problem hiding this comment.
Why is this just info vs error? Also don't we want this in sentry?
| "channel_name": self.get_option("channel"), | ||
| } | ||
| # temporarily log the payload so we can debug message failures | ||
| log_params["payload"] = orjson.dumps(payload).decode() |
There was a problem hiding this comment.
orjson warning! 😅 please make sure there's test coverage
There was a problem hiding this comment.
added a test that goes through this flow no problem :)
| integration = Integration.objects.filter(id=integration_id).first() | ||
|
|
||
| if integration is None: | ||
| raise ValueError(f"Integration with id {integration_id} not found") |
cathteng
force-pushed
the
cathy/simple-slack-sdk-client
branch
from
1cc8bd9
to
eabe176
Compare
| extra = { | ||
| "integration": "slack", | ||
| "status_string": str(code), | ||
| "error": error, | ||
| } | ||
| logger.info("integration.http_response", extra=extra) |
There was a problem hiding this comment.
are we going to know which method failed though? Like which API call? Any chance we can capture that as well to help us understand which method is failing?
There was a problem hiding this comment.
i think i can pass this in the logger if i pass the method in
There was a problem hiding this comment.
let me do this in a followup, and move the metaclass stuff to integrations.base. currently the only API call is chat.postMessage
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Added slack_sdk here #71881
This PR adds a
SlackClientthat subclassesWebClientfrom the official Slack SDK. To make the usage the same as the in-houseSlackClient, the abstraction acceptsintegration_idand finds the access token before initializing an instance of theWebClient.We also want to track the response outcomes for every outgoing API call to Slack, so I added a wrapper that executes right after every single API call to increment a metric.