-
Notifications
You must be signed in to change notification settings - Fork 117
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
slackbot: module #1250
Conversation
8c34b15
to
f39a4ff
Compare
@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:
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. |
yep this sounds good
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. |
aabe397
to
209a77b
Compare
PR's getting big. Going to break out the proto changes and service in separate PRs, and this will just have the module flow. |
13cdd2b
to
acb45a3
Compare
00d55fb
to
9f1fa98
Compare
There was a problem hiding this 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?
yep! meant to add a TODO, thanks for the reminder. In a followup PR will collect some metrics |
2525c96
to
2e6d83a
Compare
return nil, status.Error(codes.Unauthenticated, err.Error()) | ||
} | ||
|
||
m.logger.Info("received event from Slack API") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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), | ||
) |
There was a problem hiding this comment.
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
a few todos in the PR. I'll create issues for some, maybe we'll get contributors :) |
aac056d
to
2613955
Compare
Description
PR adds the building blocks for the module that interfaces with the Slack API
Hello, World!
or a default help message)Testing Performed
Locally
App Mention Event (ie messaging bot in a slack channel)
IM Message Event (ie messaging bot in a DM)
GitHub Issue
#8
TODOs
Example: