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

Enhance Slack alerts #1415

Open
wants to merge 93 commits into
base: main
Choose a base branch
from
Open

Enhance Slack alerts #1415

wants to merge 93 commits into from

Conversation

laasem
Copy link

@laasem laasem commented Jan 27, 2019

Changes Slack alerts in 3 ways:

  1. Adds ability to mention Slack users in Slack alerts based on either forced assignment or code authorship to improve error routing (as per Mention code authors in Slack notifications #1363).
  2. Differentiates between exception and notification Slack alerts to make triage more efficient.
  3. Adds affected user and hostname to Slack alerts to enhance the information sent to Slack.

A more detailed discussion of these changes and the motivations behind them can be found here.

I have made sure there are no regressions by updating ./spec/models/notification_service/slack_service_spec.rb accordingly and have noticed by observing other PRs that new tests are usually added for new routes rather than for new model helper methods, but if I need to add tests for each of the model helper methods that this PR adds do let me know.

Update: Travis CI builds seem to fail because the env authentication variable GITHUB_ACCESS_TOKEN isn't passed. Not sure how to fix; have tried not sending GraphQL request if not passed but to no avail. Any help would be much appreciated.

Lara Aasem and others added 30 commits July 19, 2018 16:17
Make slack mention actually link
Make problem.whodunnit return array regardless of size
Remove GITHUB_TO_SLACK_USERS hash and pass as env variable instead
Rename GITHUB_TO_SLACK_USERS to SLACK_USER_ID_MAP
@stevecrozz
Copy link
Member

This is super cool @laasem.

There are a lot of things our users want to do with notifications and my thought is Errbit is never going to be able to do all of them. With the amount of energy we have behind this project, I think the best thing we can do is package what you have created as a microservice (maybe a little sinatra app) and integrate it with Errbit using the 'webhook' notification system. I'd be happy to help with that if you want because then I'll at least have an answer for users who want to do all kinds of fancy stuff.

Also, if it turns out our webhook system needs more features, we can enhance that as well.

@laasem
Copy link
Author

laasem commented Jan 30, 2019

@stevecrozz That sounds great! In that case, I'm guessing it would be better to have it work for all the notification services Errbit currently supports, not just Slack?

@stevecrozz
Copy link
Member

We can discuss different ideas. But my first thought is we should create a 'errbit_notifier_slack' app whose whole job is to accept a web hook from Errbit, build a suitable notification and then send it to slack. We can build one first and then if you still have energy you're certainly welcome to do the others.

I think we could take most of this code and package it in this way, add a Dockerfile and push it to dockerhub.

@laasem
Copy link
Author

laasem commented Aug 27, 2020

@stevecrozz Sorry for the extremely late response and thanks for your feedback! I can start working on packaging this as a separate app if there's still a need for it.

@stevecrozz
Copy link
Member

It's totally up to you. Does the webhook payload from Errbit give you everything you need?

@laasem
Copy link
Author

laasem commented Nov 1, 2020

@stevecrozz It has useful data about the problem, but we would still need some data about the app itself (repo name, branch, etc.) as well as backtraces in order to get the Git blame information. I think we can either add these attributes to the webhook payload or have errbit_slack_notifier read from the same database as Errbit - what do you think?

I'm also not sure how much of the SlackService code we should duplicate between errbit and errbit_slack_notifier. 🤔

@stevecrozz
Copy link
Member

@laasem Adding to the web hook seems like a good idea. If you make a PR to add all the attributes you need to the web hook and a test or two, I'd be happy to merge that.

As far as duplication goes, it wouldn't bother me at all if you copy/pasted all the code you need into errbit_slack_plugin and modify it however it makes sense. IMO it wouldn't be worth the effort to share any of this logic.

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.

None yet

3 participants