Move away from deprecated slack upload endpoints #41974
Conversation
calherries
added
the
backport
|
src/metabase/pulse.clj
Outdated
| @@ -541,6 +543,8 @@ | |||
| (defmethod send-notification! :slack | |||
| [{:keys [channel-id message attachments]}] | |||
| (let [attachments (create-and-upload-slack-attachments! attachments)] | |||
| ;; Cal 2024-03-04: Without this sleep, attached images don't always appear. I tried with 1000ms and it wasn't enough. | |||
| (Thread/sleep 2000) | |||
There was a problem hiding this comment.
Frankly I don't know why this is happening. I can only guess it's an issue with slack's state.
Without this sleep, instead of getting a result like this:
I would get this:
even though the sent message and attachments were the same in both cases, based on the logs
2024-04-29 17:06:54,493 TRACE integrations.slack :: Slack API request: "https://slack.com/api/chat.postMessage" {:form-params {:channel "@cal", :username "MetaBot", :icon_url "http://static.metabase.com/metabot_slack_avatar_whitebg.png", :text nil, :attachments "[{\"blocks\":[{\"type\":\"header\",\"text\":{\"type\":\"plain_text\",\"text\":\"subdash\",\"emoji\":true}},{\"type\":\"section\",\"fields\":[{\"type\":\"mrkdwn\",\"text\":\"Sent by Crowberto Corv\"}]}]},{\"title\":\"Orders, Count\",\"title_link\":\"http://localhost:3001/question/30\",\"fallback\":\"Orders, Count\",\"text\":\"18,760\"},{\"title\":\"Best selling products\",\"title_link\":\"http://localhost:3001/question/23\",\"fallback\":\"Best selling products\",\"image_url\":\"https://files.slack.com/files-pri/T078VCLCR-F071306S06R/image.png\"},{\"blocks\":[{\"type\":\"divider\"},{\"type\":\"context\",\"elements\":[{\"type\":\"mrkdwn\",\"text\":\"<http://localhost:3001/dashboard/2|*Sent from Metabase Test*>\"}]}]}]"}}
ideas welcome
There was a problem hiding this comment.
I hit this again with a 2000ms sleep, so I've extended it to 3000ms.
The issue appears to be caused by a race condition between the message being sent to the subscription's destination, and the actual upload of the file to the files channel. The latter needs to finish first, and the former needs to finish afterwards. The race condition is happening on the slack side, because on our side we're completing the requests in the right order.
Here's when the app integration uploaded the image to the files channel, from slacks' point of view:
And here's when the message was sent to me, from slack's point of view:
The upload request (files.completeUploadExternal) was completed first, but in slack they appear the other way around.
There was a problem hiding this comment.
Seems like others are running into this issue too. slackapi/python-slack-sdk#1363
We could try polling conversations_history on the file channel, but that seems pretty invasive and I wonder if we're allowed to do that.
using conversations_history is one of the recommended approaches to retrieve your message's details such as the timestamp
it could take a while to confirm the images uploaded:
I tried to add a pause 1.5 sec before asking the history, but this still not guaranteed the right timings.
Polling that endpoint to wait for the last file to be uploaded would significantly complicate things. Perhaps we should just sleep for 5 seconds and hope for the best?
There was a problem hiding this comment.
Hard to believe slack screwed up this API so bad. They must have been in a rush
There was a problem hiding this comment.
Update on this for reference:
I discovered this PR changing the python slack SDK slackapi/python-slack-sdk#1408
This pull request updates the internals of files_upload_v2 method to eliminate files.info API calls, which are no longer necessary because the server-side now returns the same metadata as part of files.completeUploadExternal API responses.
so I discovered I could just poll files.completeUploadExternal until the file is shared
| "response_metadata" : { | ||
| "warnings" : [ "superfluous_charset" ] | ||
| } | ||
| "ok": true, |
There was a problem hiding this comment.
I copied this from a real test and replaced some IDs
src/metabase/integrations/slack.clj
Outdated
| (do (join-channel! channel-id) | ||
| (complete!)) | ||
| (throw e))))] | ||
| (u/prog1 (get-in complete-response [:files 0 :url_private]) |
There was a problem hiding this comment.
I think this is a lot easier to read with named functions and less complex let destructuring. Something like:
(defn slack-upload-url
[filename file]
(POST "files.getUploadExternal" {:query-params {:filename filename
:length (count file)}}))
(defn upload-file-or-throw!
[file {upload-url :upload_url file-id :file_id}]
(let [response (http/post upload-url {:multipart [{:name "file"
:content "file"}]})]
(if (= (:status response) 200)
response
(throw (ex-info "Failed to upload file to Slack:" (select-keys response [:status :body]))))))
(defn complete-upload!*
[channel-id filename file-id]
(POST "files.completeUploadExternal"
{:query-params {:files (json/generate-string [{:id file-id :title filename}])
:channel_id channel-id}}))
(defn complete-upload!
[channel-id filename file-id]
(try
(complete-upload!* channel-id filename file-id)
(catch Throwable e
;; If file upload fails with a [etc]
(if (= (:error-code (ex-data e)) "not_in_channel")
(doto channel-id
(join-channel!)
(complete-upload!* filename file-id))
(throw e)))))
(defn upload-file!
[file filename channel-id]
(let [external-url (slack-upload-url filename file)
_ (upload-file-or-throw! file external-url)
complete-response (complete-upload! (maybe-lookup-id channel-id (slack-cached-channels-and-usernames))
filename
(:file_id external-url))
image-url (get-in complete-response [:files 0 :url_private])]
(log/debug "Uploaded image" image-url)
image-url))There was a problem hiding this comment.
I didn't transcribe all the comments / write docstrings / etc (and I should have) but you get the idea
There was a problem hiding this comment.
Thanks, this is good stuff. I'll try it
There was a problem hiding this comment.
try this: f351704
(lots of plagiarism here)
src/metabase/integrations/slack.clj
Outdated
| ;; If it takes more than 10 seconds, something else is wrong and we should abort. | ||
| :timeout-ms 10000 | ||
| :interval-ms 500})) | ||
| (throw (Exception. "Confirming the file was uploaded to a Slack channel timed out.")))] |
There was a problem hiding this comment.
ex-info + filename in there maybe? So you know which file was not uploaded?
src/metabase/integrations/slack.clj
Outdated
| (loop [] | ||
| (let [response (thunk)] | ||
| (if (done? response) | ||
| true |
There was a problem hiding this comment.
I think for this case it's fine, but I'd rather have response or nil returned here, it seems like a generally more useful abstraction then.
|
@piranha thanks for your enhancements, I've applied them |
|
@calherries Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
|
@metabase-bot backport release-x.48.x |
|
@calherries something went wrong while creating a backport [Logs] |
Fixes #41890
This PR changes
metabase.integrations.slack/upload-file!from using one deprecated endpoint to use the new set of three endpoints (why slack, why?). I used the python slack SDK as a reference implementation.I'm doing this in the laziest simplest way possible, making three requests for each file in series. A simple optimization to follow this up would be to make the first two requests for each file in parallel, and then when all those requests complete call
files.completeUploadExternalonly once for all files. Given this was pretty slow and unoptimized already (we could have been executing requests in parallel before) I'm assuming it's not a huge issue for now, and I've left a TODO in the code explaining this possibility.This needs manual testing because there's a lot of mocking going on in our automated tests. How to test: