Socket Mode support #885

mumoshu · 2021-01-14T00:46:06Z

This is a ~~WIP~~ implementation of Socket Mode support for this library based on the official documentation at https://api.slack.com/apis/connections/socket and some reverse engineering on https://github.com/slackapi/node-slack-sdk/blob/main/packages/socket-mode.

Please read the official annoucement for more information about Socket Mode.

Resolves #883

Notes for reviewers:

This PR introduces NO API changes to existing types
socketmode.Client is added, which is a bit similar to the existing slack.RTM
Unit tests for the new feature
Documentation update (where should I add this to?)

Pull Request Guidelines

These are recommendations for pull requests.
They are strictly guidelines to help manage expectations.

PR preparation

Run make pr-prep from the root of the repository to run formatting, linting and tests.

Should this be an issue instead

is it a convenience method? (no new functionality, streamlines some use case)
exposes a previously private type, const, method, etc.
is it application specific (caching, retry logic, rate limiting, etc)
is it performance related.

API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

no tests, if you're adding to the API include at least a single test of the happy case.
If you can accomplish your goal without changing the API, then do so.
dependency changes. updates are okay. adding/removing need justification.

Examples of API changes that do not meet guidelines:

in library cache for users. caches are use case specific.
Convenience methods for Sending Messages, update, post, ephemeral, etc. consider opening an issue instead.

This is a WIP implementation of Socket Mode support for this library based on the official documentation at https://api.slack.com/apis/connections/socket and some reverse engineering on https://github.com/slackapi/node-slack-sdk/blob/main/packages/socket-mode. Please read [the official annoucement](https://medium.com/slack-developer-blog/socket-to-me-3d122f96d955) for more information about Socket Mode. Resolves slack-go#883

seratch

@mumoshu You are so quick! I am happy to be of help. Please feel free to ask me or other folks in the github.com/SlackAPI team any questions 👋

seratch · 2021-01-14T01:26:31Z

socket_mode_client.go

+	Type       string                   `json:"type"`
+	Reason     string                   `json:"reason"`
+	Payload    SocketModeMessagePayload `json:"payload"`
+	EnvelopeID string                   `json:"envelope_id"`


FYI: https://github.com/slackapi/python-slack-sdk/blob/v3.2.0/slack_sdk/socket_mode/request.py#L7-L12

A Socket Mode envelope can have the following attributes.

AcceptsResponsePayload bool

RetryAttempt *int // only for Events API

RetryReason string // only for Events API

Thanks! I've added a bunch of fields according to your info and my observation

https://github.com/mumoshu/slack/blob/852061eaff58f9acb2a18f70ac871341d0f57982/socketmode/request.go#L15-L38

examples/socketmode/events.go

socket_mode_managed_conn.go

examples/socketmode/events.go

socket_mode.go

…ycle

…pkg and slacksocketmode pkg

…nect

…nsistency

examples/socketmode/events.go

slacksocketmode/socket_mode_managed_conn.go

seratch · 2021-01-16T00:45:14Z

slacksocketmode/socket_mode_managed_conn.go

+	smc.Debugln("Sending ACK ", envelopeID)
+
+	// See https://github.com/slackapi/node-slack-sdk/blob/c3f4d7109062a0356fb765d53794b7b5f6b3b5ae/packages/socket-mode/src/SocketModeClient.ts#L417
+	msg := map[string]interface{}{"envelope_id": envelopeID}


The response message can have "payload" in addition to "envelop_id" for interactive / slash_commands types, while events_api requires only "envelope_id".

https://api.slack.com/apis/connections/socket-implement#acknowledge

https://github.com/slackapi/java-slack-sdk/tree/v1.5.1/slack-api-client/src/main/java/com/slack/api/socket_mode/response

slack.OutgoingMessage is not compatible with Socket Mode.

Ah thanks! First of all, I didn't realize till now that slash_commands and interactive are top-level types that are comparable to events_api and hello and so on.

And it's really good to know that the way we ack the socket mode "request" (or "event", which do you prefer btw? I thought you call it SocketModeRequest in python-sdk and event in node-sdk)

I will soon try modifying examples/socketmode and the library and test it accordingly and circle back to you.

The first changeset for this: 88722e9#diff-f5d9e76c79011a908909fa0f14332b25faba7ef70c64f12682937d35382f3f47R395-R425

slacksocketmode/socket_mode_managed_conn.go

…clarity

kanata2

Great work! Thank you.
I've add a few comments, but I will do another detailed review when you finish implementing.

internal/misc/misc.go

examples/socketmode/socketmode.go

slacksocketmode/socket_mode_managed_conn.go

…equest counterpart

…ing and handling

mumoshu · 2021-01-17T09:30:03Z

Added proper support (as I think) for disconnect handling - Client.Run() has been redesigned to handle reconnection when Slack sends disconnect-type request over the Socket Mode connection.

Also note that the Client struct has much less fields = states now, which I believe to help maintainability.

seratch

I just left a few more comments. This pull request looks almost done 👍

(Please feel free to mention me whenever you and the maintainers of this project need my help.)

socketmode/client.go

seratch · 2021-01-18T05:38:47Z

socketmode/request.go

+	// `events_api` type only
+	EnvelopeID string `json:"envelope_id"`
+	// TODO Can it really be a non-object type?
+	// See https://github.com/slackapi/python-slack-sdk/blob/3f1c4c6e27bf7ee8af57699b2543e6eb7848bcf9/slack_sdk/socket_mode/request.py#L26-L31


Ah, never mind about L30-31 in the Python SDK (the code should be improved). You can safely assume the "payload" is an object.

Got it. Thanks for clarification 😃

socketmode/socketmode.go

seratch · 2021-01-18T05:53:01Z

socketmode/socket_mode_managed_conn.go

+					// We treat the `disconnect` request from Slack as an error internally,
+					// so that we can tell the consumer of this function to reopen the connection on it.
+					return ErrorRequestedDisconnect{}


How does the handling code look like? Do developers need to write a handler on their own? If so, having the code in the example should be helpful. The disconnect requests can be regularly sent to all Socket Mode apps. This is not a rare case.

If this library can have the reconnection logic (= acquiring a new WSS URL and establishing a new connection to the endpoint under the hood), that's amazing!

If this library can have the reconnection logic (= acquiring a new WSS URL and establishing a new connection to the endpoint under the hood), that's amazing!

Good question - Yes I've made the client to handle the reconnection on its own, following all the awesome Socket Mode clients you've written for other languages 😄

For your reference, the below is where we implement the infinite reconnection loop:

https://github.com/mumoshu/slack/blob/e9e7822646b5341295e0854167d774e4fb5cd33b/socketmode/socket_mode_managed_conn.go#L36-L42

BTW In Go anything that is NOT under internal package whose name starts with an uppercase letter is considered public - so ErrorRequestedDisconnect is fairly confusing as it can give users misconception that one might want to use this outside of this package.

I'm going to make it private so that it is clear that the consumer of the client does not need to know about this internal error happens on disconnect request.

so ErrorRequestedDisconnect is fairly confusing as it can give users misconception that one might want to use this outside of this package.

Yes, actually I was thinking so too. Making it private should be the right direction 👍

Also, I've verified the reconnecting logic works properly. Great work!

kanata2

Added few comments, but looks good to me!

socketmode/socket_mode_managed_conn.go

socket_mode.go

socketmode/client.go

mumoshu · 2021-01-19T00:10:54Z

Thanks to everyone for reviewing! I believe I've addressed all the feedback we had so far.

kanata2

LGTM!!!

@seratch How do you think?

seratch

Although I am not a maintainer of this project, please allow me to give an approval. I am pretty sure that this Socket Mode client implementation is production-ready 👍

kanata2 · 2021-01-19T05:49:53Z

released! https://github.com/slack-go/slack/releases/tag/v0.8.0

prologic · 2023-12-11T04:25:04Z

Hey 👋 Did the slacktest package ever get updated at all? Doesn't look like it did by this PP, and that means using it to unit test your bot's business logic doesn't currently work :/ e.g:

time="2023-12-11T14:14:58+10:00" level=debug msg="Starting SocketMode" file=socketmode/socket_mode_managed_conn.go line=293 logger=log2Logrus
2023/12/11 14:14:58 (127.0.0.1:57589 -) "POST /apps.connections.open HTTP/1.1" 404 19 41µs
time="2023-12-11T14:14:58+10:00" level=debug msg="HTTP/1.1 404 Not Found\r\nContent-Length: 19\r\nContent-Type: text/plain; charset=utf-8\r\nDate: Mon, 11 Dec 2023 04:14:58 GMT\r\nX-Content-Type-Options: nosniff\r\n\r\n404 page not found\n\n" file=misc.go line=266 logger=log2Logrus

mumoshu mentioned this pull request Jan 14, 2021

Feature request: support new Socket Mode feature #883

Closed

seratch reviewed Jan 14, 2021

View reviewed changes

mumoshu added 12 commits January 15, 2021 09:31

Use app token for opening socket mode connection

14923f2

Move Socket Mode support to the slacksocketmode pkg to avoid import c…

ff6e556

…ycle

Move backoff to the internal/backoff pkg for sharing between slack …

88c913f

…pkg and slacksocketmode pkg

Fix the comment on ack method

493fd6f

Remove invalid comment left due to copy-paste on SocketModeClient.con…

d472ffd

…nect

Fix SocketModeClient receiver name

e97de46

Various renames on Socket Mode related symbols for ease of use and co…

f5c72e4

…nsistency

Move statusCodeError into the internal pkg for use from slacksocketmode

08f68aa

The initial version that manually verified to work

155b2bc

Fix log message

cf7f3ca

Better debug logs (may be removed before merging)

38b0347

Change socketmode example source file name for consistency

b53014b

seratch reviewed Jan 16, 2021

View reviewed changes

mumoshu added 9 commits January 16, 2021 10:15

Call it a Socket Mode "Request" rather than "Message" or "Event" for …

4e25959

…clarity

More restructuring for readability

528677c

Refactor socket mode deadman for clarity

5e41380

More Event and Request distinction in code-base

1a23793

Fix socket mode debug log message

0d6994c

Remove unnecessary call to SetCloseHandler

1da2c76

Send back PONG to Slack via WebSocket in Socket Mode

5b8a218

Delete unused handleAck function for slacksocketmode

e5d2f1a

Serialize writes to WebSocket for Socket Mode ACKs

c269a17

kanata2 reviewed Jan 16, 2021

View reviewed changes

internal/misc/misc.go Show resolved Hide resolved

examples/socketmode/socketmode.go Outdated Show resolved Hide resolved

slacksocketmode/socket_mode_managed_conn.go Outdated Show resolved Hide resolved

slacksocketmode/socket_mode_managed_conn.go Outdated Show resolved Hide resolved

mumoshu added 4 commits January 16, 2021 16:12

Rename slacksocketmode to socketmode

852061e

Merge internalEvent and externalEvent

11f20f3

Apply goimports

3cd2344

Give socketmode Response a dedicated source file

7e8a5a7

mumoshu added 3 commits January 17, 2021 18:20

socket mode: Make Run() a bit more easy to read

e5a4822

socket mode: Make slash-command related event value consistent with r…

5a2d8db

…equest counterpart

Fix examples/socketmode for invalid member-joined-channel event match…

18c4fd2

…ing and handling

mumoshu added 3 commits January 17, 2021 18:32

socketmode: Add support for Go 1.12

9335441

More unit tests for socket mode request parsing and some tweaks

b2bfdf1

socketmode: Fix logger for correct line numbers in messages

eaaf0e0

seratch reviewed Jan 18, 2021

View reviewed changes

kanata2 reviewed Jan 18, 2021

View reviewed changes

mumoshu added 11 commits January 19, 2021 08:30

socketmode: Remove unused idGen

a606305

socketmode: Fix comment on Client.Open()

93df48b

socketmode: make receiver name "smc"

d7a26ac

socketmode: Remove RTM-ness from comments on log messages

2cf901c

socketmode: Use EventType constants everywhere

2cbdf3b

socketmode: Update the example with connectino lifecycle events

04ead06

socketmode: prepare costly log data only on debug mode

e9e7822

Remove unnecessary commented-out line

c0cd3da

Fix comment on StartSocketModeContext

fcf2b1e

socketmode: Brush up comment on socketmode.Client

0f4bd7d

socketmode: Make internal error private to the pkg as it should

0fbd39a

kanata2 approved these changes Jan 19, 2021

View reviewed changes

kanata2 added this to the v0.8.0 milestone Jan 19, 2021

seratch approved these changes Jan 19, 2021

View reviewed changes

kanata2 merged commit 483202e into slack-go:master Jan 19, 2021

mumoshu deleted the socket-mode branch January 19, 2021 23:53

dependabot bot mentioned this pull request Mar 18, 2021

⬆️ Bump github.com/slack-go/slack from 0.7.4 to 0.8.2 asin9749/teler#17

Closed

prologic mentioned this pull request Dec 11, 2023

Fix SocketMode support in slacktest #1244

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket Mode support #885

Socket Mode support #885

mumoshu commented Jan 14, 2021 •

edited

Loading

seratch left a comment

seratch Jan 14, 2021

mumoshu Jan 16, 2021

seratch Jan 16, 2021

mumoshu Jan 16, 2021 •

edited

Loading

mumoshu Jan 16, 2021

kanata2 left a comment

mumoshu commented Jan 17, 2021

seratch left a comment

seratch Jan 18, 2021

mumoshu Jan 18, 2021

seratch Jan 18, 2021 •

edited

Loading

mumoshu Jan 19, 2021 •

edited

Loading

mumoshu Jan 19, 2021 •

edited

Loading

seratch Jan 19, 2021

kanata2 left a comment

mumoshu commented Jan 19, 2021

kanata2 left a comment

seratch left a comment

kanata2 commented Jan 19, 2021

prologic commented Dec 11, 2023

Socket Mode support #885

Socket Mode support #885

Conversation

mumoshu commented Jan 14, 2021 • edited Loading

Pull Request Guidelines

PR preparation

Should this be an issue instead

API changes

Examples of API changes that do not meet guidelines:

seratch left a comment

Choose a reason for hiding this comment

seratch Jan 14, 2021

Choose a reason for hiding this comment

mumoshu Jan 16, 2021

Choose a reason for hiding this comment

seratch Jan 16, 2021

Choose a reason for hiding this comment

mumoshu Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

mumoshu Jan 16, 2021

Choose a reason for hiding this comment

kanata2 left a comment

Choose a reason for hiding this comment

mumoshu commented Jan 17, 2021

seratch left a comment

Choose a reason for hiding this comment

seratch Jan 18, 2021

Choose a reason for hiding this comment

mumoshu Jan 18, 2021

Choose a reason for hiding this comment

seratch Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

mumoshu Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

mumoshu Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

seratch Jan 19, 2021

Choose a reason for hiding this comment

kanata2 left a comment

Choose a reason for hiding this comment

mumoshu commented Jan 19, 2021

kanata2 left a comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

kanata2 commented Jan 19, 2021

prologic commented Dec 11, 2023

mumoshu commented Jan 14, 2021 •

edited

Loading

mumoshu Jan 16, 2021 •

edited

Loading

seratch Jan 18, 2021 •

edited

Loading

mumoshu Jan 19, 2021 •

edited

Loading

mumoshu Jan 19, 2021 •

edited

Loading