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

alerting: added slack as a notifier #101

Merged
merged 7 commits into from
Jun 17, 2022
Merged

alerting: added slack as a notifier #101

merged 7 commits into from
Jun 17, 2022

Conversation

darwinz
Copy link
Contributor

@darwinz darwinz commented Jun 13, 2022

Fixes: #52

@darwinz darwinz requested a review from adamdecaf June 13, 2022 15:57
@darwinz darwinz linked an issue Jun 13, 2022 that may be closed by this pull request
@darwinz darwinz marked this pull request as ready for review June 14, 2022 22:40
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #101 (59c0a57) into master (961efe1) will decrease coverage by 0.48%.
The diff coverage is 3.84%.

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   42.08%   41.60%   -0.49%     
==========================================
  Files          87       88       +1     
  Lines        3647     3644       -3     
==========================================
- Hits         1535     1516      -19     
- Misses       1817     1841      +24     
+ Partials      295      287       -8     
Impacted Files Coverage Δ
internal/alerting/alerter.go 0.00% <0.00%> (ø)
internal/alerting/pagerduty.go 0.00% <0.00%> (ø)
internal/alerting/slack.go 0.00% <0.00%> (ø)
internal/service/model_errors.go 13.33% <0.00%> (-4.85%) ⬇️
internal/incoming/odfi/scheduler.go 29.67% <40.00%> (-0.67%) ⬇️
internal/pipeline/aggregate.go 24.63% <66.66%> (-0.87%) ⬇️
internal/consul/leader.go 34.28% <0.00%> (-51.43%) ⬇️
internal/transform/gpg.go 48.38% <0.00%> (-18.28%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 961efe1...59c0a57. Read the comment docs.

@adamdecaf
Copy link
Member

Is there a use-case for warning alerters? We don't have a setup for determining when warnings occur, so I don't see them as being used now.

@darwinz
Copy link
Contributor Author

darwinz commented Jun 15, 2022

Is there a use-case for warning alerters? We don't have a setup for determining when warnings occur, so I don't see them as being used now.

I can remove the warning alerters. I just thought it would be useful to give the option of having a non-critical notifier

@darwinz darwinz force-pushed the slack-alerting branch 3 times, most recently from 301d801 to 0225281 Compare June 16, 2022 00:36
@adamdecaf
Copy link
Member

Is there a use-case for warning alerters? We don't have a setup for determining when warnings occur, so I don't see them as being used now.

I can remove the warning alerters. I just thought it would be useful to give the option of having a non-critical notifier

I can agree that's a useful feature, but the current code doens't handle it well.

Copy link
Member

@adamdecaf adamdecaf left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@adamdecaf adamdecaf merged commit a61f0b5 into master Jun 17, 2022
@adamdecaf adamdecaf deleted the slack-alerting branch June 17, 2022 17:53
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.

errors: include slack notification channel
3 participants