Reintroduce AWS SNS notifications #709
Conversation
|
Damn, only just found #391 despite earlier searching - annoying. Will review the feedback to that one & implement here (e.g. templating ARN also, which I was wondering about). |
|
OK, looks ready to review to me - @stuartnelson3 do the tests included look sufficient? |
|
Bump for @stuartnelson3 @fabxc - now you've got the release out the way, could I have some feedback on this? Will rebase to pick up the latest master before final submission, obviously, but presume you have some comments first... |
|
Sorry for not getting to this! I'll give it a look |
|
ping - any chance of this being looked at yet? And yes, I will rebase if there's any interest. I was hoping to go on & produce a similar option to get AlertManager to periodically feed alert status to an AWS CloudWatch Metric, so I could have alarm associated that our enterprise monitoring system would see. But if this relatively simple PR takes months to start review... |
|
The notification providers integrated into the Alertmanagers are meant to immediately notify someone. Enqueuing it in some other mechanism sort of defeats the purpose and additionally makes ensuring that your alerting pipeline works properly significantly more complicated if not impossible as you're inserting yet another fragile distributed system into the equation. I would suggest implementing this support via a webhook. |
|
@brancz - respectfully I disagree. SNS is being used to notify a system as our 'Enterprise alerting systems' which I'm required to use only practically accept input via SNS or CloudWatch Alarms. I hope to trigger them as well as the Slack & other channels we will likely use in the local team. You originally had SNS functionality but wrote it out during a refactor a year or so ago - several other people have commented on #157 and #391 wanting it back. AFAIK you cannot trigger SNS (or CW Alarms) via a webhook directly, only by chaining together other AWS bits like API Gateway/Lambdas ... at which point it does become a fragile distributed system. |
|
This is a discussion we have quite repeatedly. People have different requirements and expectations when it comes to their notifications and literally any system capable of delivering messages in one way or the other could be an integration. Just considering how many pub/sub or message buses there are, it's obvious that we can neither provide all those, nor accept them all into our main code base. We've made our experiences with that for Prometheus service discovery integrations. The webhook format is not intended to be sent to arbitrary integrations directly but to run a small web server that provides integration with other systems. I think the reasonable way here is to provide better examples or maybe a small framework to build such a webhook bridge. I'm sorry that you put the effort to implement this direct integration, but I hope our reservations are understandable. They are not arbitrary but come from consistent experiences. |
|
Not the answer I was hoping for after 3 months, especially as you'd already implemented it once, the earlier PR merely wanted tests & wasn't actively rejected, but it's your project & I do recognise the issue. How about you define a plugin interface (golang 1.8+) & allow those with differing opinions to bear their own burdens, yet be able to share them with the community in a way 100 user-specific webhook implementations never will be? If this is at all possible then you or I can start a new issue to discuss, hopefully with a much quicker resolution. Feel free to chuck out all the other integrations that break into the plugin in the future! |
|
Personally I somewhat like the idea, but others feel differently. Practically it probably doesn't not make a difference. Whether we have 100 user specific plugins or webhooks ends up being roughly the same. Just that webhooks need a bit more initial deployment work. Plugins also have remaining issues: golang/go#19233 |
|
FYI the plugin issue claims to have been fixed in golang 1.9.2 so please reconsider allowing externally provided/maintained plugins. Closing this PR as unacceptable. |
|
I know this is an old thread but https://github.com/DataReply/alertmanager-sns-forwarder |
|
SNS will soon be supported natively: #2559 |
Julien, this is superb. Any idea of the timeline or when it will 'work' if not fully supported by the project yet? Thanks! |
I have no insight of the planning of other contributors. I suggest you to bookmark the pull request page, I expect that you could have something to test in the coming weeks/months. I guess users feedback is also welcome in the comments of #2559. |
As requested in #157, though it was a bit more than a one liner after the re-write.
Requires AWS_REGION environment variable to be set to work currently, but wondering about making that a global setting (maybe also a default Topic_ARN) or default to the one in the ARN, not sure.
I see your comments about testing in #700, but as not an expert golang dev without a similar pattern from any existing notifier I'm at a loss where to start. The only significant logic is in AmazonSns::Notify which if I could fake up the inputs could be called with a bad AWS_REGION to always fail, but that's more integration test than unit.