Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

slackbot: module #1250

Merged
merged 22 commits into from
Apr 19, 2021
Merged

slackbot: module #1250

merged 22 commits into from
Apr 19, 2021

Conversation

scarlettperry
Copy link
Contributor

@scarlettperry scarlettperry commented Apr 6, 2021

Description

PR adds the building blocks for the module that interfaces with the Slack API

  • receives & processes the request from Slack
  • sends the users's message to the bot service, where it's mapped to a reply (for now, just Hello, World! or a default help message)

Testing Performed

Locally

App Mention Event (ie messaging bot in a slack channel)
drawing

IM Message Event (ie messaging bot in a DM)
drawing

GitHub Issue

#8

TODOs

Example:

  • separate ack'ing the slack request and handling the event
  • unit tests

@scarlettperry scarlettperry changed the title start building the slackbot module slackbot: module and service Apr 6, 2021
@scarlettperry
Copy link
Contributor Author

scarlettperry commented Apr 8, 2021

@danielhochman still need to add unit tests but can I get some early feedback? I've kept the bot service really simple for now till i get to the grpc reflection step. Also, lmk if you'd like me to break some parts of the flow into different PRs.

Something I'd like your thoughts on is async processing. Slack docs recommend separating the process of (1) acknowledging receipt of the request and (2) handling the event. In the PR, I left a comment to denote at which step we can do this (after we've verified it's a request from Slack and after we've checked if it's a URL verification event).

We receive 1 event in each request so some thoughts:

  • a simple job queue. create an event channel and start a worker goroutine
  • i need to look at the audit service code more closely but perhaps something similar in how it passes an event to the audit sink which does whatever it wants with it

do you have something else in mind?

Lastly, a topic we can have a converstation on later but wanted to raise: dropped events. Are some dropped events okay? Do we want to persist in memory or externally somewhere (SQS, Kafka)? Just something I've been thinking about.

backend/module/bot/slackbot/slackbot.go Outdated Show resolved Hide resolved
backend/module/bot/slackbot/slack_helper_test.go Outdated Show resolved Hide resolved
backend/module/bot/slackbot/slackbot.go Outdated Show resolved Hide resolved
backend/module/bot/slackbot/slackbot.go Outdated Show resolved Hide resolved
backend/module/bot/slackbot/slackbot_helper.go Outdated Show resolved Hide resolved
@danielhochman
Copy link
Collaborator

danielhochman commented Apr 8, 2021

a simple job queue. create an event channel and start a worker goroutine

yep this sounds good

Lastly, a topic we can have a converstation on later but wanted to raise: dropped events. Are some dropped events okay? Do we want to persist in memory or externally somewhere (SQS, Kafka)? Just something I've been thinking about.

for now, just drop them on the floor, not worth any extra complexity as long as we're doing straightforward read-only request/response commands. later on we may use postgres as a queue layer similar to audit if we require durability.

@scarlettperry
Copy link
Contributor Author

scarlettperry commented Apr 11, 2021

PR's getting big. Going to break out the proto changes and service in separate PRs, and this will just have the module flow.

@scarlettperry scarlettperry changed the title slackbot: module and service slackbot: module Apr 14, 2021
@scarlettperry scarlettperry marked this pull request as ready for review April 15, 2021 01:44
@scarlettperry scarlettperry requested a review from a team as a code owner April 15, 2021 01:44
Copy link
Contributor

@mikecutalo mikecutalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought about capturing some metrics? Maybe success and failure when handling events?

backend/module/bot/slackbot/slackbot.go Show resolved Hide resolved
backend/module/bot/slackbot/slackbot_test.go Show resolved Hide resolved
backend/module/bot/slackbot/slackbot.go Outdated Show resolved Hide resolved
backend/module/bot/slackbot/slackbot.go Outdated Show resolved Hide resolved
backend/module/bot/slackbot/slackbot_helper.go Outdated Show resolved Hide resolved
@scarlettperry
Copy link
Contributor Author

scarlettperry commented Apr 15, 2021

Have you thought about capturing some metrics? Maybe success and failure when handling events?

yep! meant to add a TODO, thanks for the reminder. In a followup PR will collect some metrics

return nil, status.Error(codes.Unauthenticated, err.Error())
}

m.logger.Info("received event from Slack API")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete or add more info

Copy link
Contributor Author

@scarlettperry scarlettperry Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added more info and moved this log lower in the flow when we've confirmed if its an app mention/DM event

// TODO: (sperry) request just provides the id of the channel and user. we'd have to make seperate slack API calls using the ids to get the names
zap.String("channel id", event.Channel),
zap.String("user id", event.User),
)
Copy link
Contributor Author

@scarlettperry scarlettperry Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a todo on getting the channel/user name b/c the ids are not really helpful. we can use these ids to make separate slack API calls to get the channel/user name. though, then we'd want to cache the results to avoid having to make these calls each time there's an event and to avoid getting rate limited by those APIs

@scarlettperry
Copy link
Contributor Author

a few todos in the PR. I'll create issues for some, maybe we'll get contributors :)

@scarlettperry scarlettperry merged commit ef23fd8 into main Apr 19, 2021
@scarlettperry scarlettperry deleted the sperry-slackbot-mod branch April 19, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants