-
Notifications
You must be signed in to change notification settings - Fork 491
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
Feature: Discord Webhook Alert Handler #2287
Feature: Discord Webhook Alert Handler #2287
Conversation
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.
Awesome! I didn't to a complete review, but, here are a few changes to fix up.
Ha, sorry, thought I'd caught all of those, pushing now! |
I'm also happy to add the required stuff to Chronograf, I'd just need a pointer to what needs updating! |
@mattnotmitt is this ready for another review? |
@desa very nearly, there's an issue I'm experiencing with the timestamp boolean - I posted about it in the kapacitor channel in the influxdata community slack. |
@mattnotmitt can you link me to the issue? |
I've pinged you in the thread but here's the text:
I've been having some trouble with using a boolean in the HandlerConfig for
a custom alert handler. If you look at my pr
https://github.com/influxdata/kapacitor/pull/2287/files, the Timestamp is a
boolean field configurable in both in the `kapacitor.conf` file and when
the handler is called inline. Oddly, the value in the `kapacitor.conf` file
is ignored always, and the only way i can decide whether the timestamp
shows or not is if it is set in the tickscript. Could someone with
knowledge of the arcane internals of the kapacitor config loading give me a
hand? I'm sure i could figure it out myself but I'd appreciate some
guidance.
…On Fri, 27 Mar 2020, 16:37 Michael Desa, ***@***.***> wrote:
@mattnotmitt <https://github.com/mattnotmitt> can you link me to the
issue?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQBM7RHIKQNPAKULAIH3RDRJTI4BANCNFSM4KSLJKJQ>
.
|
040e517
to
1729dd0
Compare
@desa Ready for review whenever you're available. |
@mattnotmitt were you able to take a look at my suggestion in slack? |
Yeah I looked and replied. There's no discernable difference between it
being set to false and it just not being set. Decided to just take away the
handler config and keep it in the conf file.
…On Mon, 30 Mar 2020, 16:23 Michael Desa, ***@***.***> wrote:
@mattnotmitt <https://github.com/mattnotmitt> were you able to take a
look at my suggestion in slack?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQBM7TSSUUBBD2GI4UTL6TRKC2QPANCNFSM4KSLJKJQ>
.
|
Typically people use pointers to differentiate between the two. I think that should work. |
I don't really know how to implement that (my go skills aren't that great)
and the timestamp is so tiny that I find it unlikely that someone would
need to hide it. If you need this to merge I might be able to do it in a
few months but I need to knuckle down on some other work now, sorry.
…On Mon, 30 Mar 2020, 17:11 Michael Desa, ***@***.***> wrote:
Typically people use pointers to differentiate between the two. I think
that should work.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQBM7RFCJWK6ME3CDIPCSLRKDACDANCNFSM4KSLJKJQ>
.
|
@mattnotmitt just reaching out with regards to
Is this something that you'd still be interested in doing? I'd be happy to offer an outline of the steps involved. |
Hi @desa, yeah I'd still be happy to work on that! |
@mattnotmitt Awesome. For 2.0 of InfluxDB, you're going to want to take a look at the code in https://github.com/influxdata/flux/blob/8a6ee0356c0837db0ace34cb7a2578be600874d3/stdlib/pagerduty/pagerduty.flux, https://github.com/influxdata/influxdb/tree/master/notification/endpoint, and https://github.com/influxdata/influxdb/tree/master/notification/rule. This part of the 2.0 codebase is a bit rough with a fair amount of repeated logic. For 1.x Chronograf, you'll want to start looking in https://github.com/influxdata/chronograf/blob/2dadcaf8dcb6a6733e027e1d9205f3688b81b62d/ui/src/types/kapacitor.ts and https://github.com/influxdata/chronograf/blob/9d8a49ba0ef8131cdce22d73718859f55f434db2/ui/src/kapacitor/components/handlers/index.js. |
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.
APPROVED!
Required for all non-trivial PRs
Added a Discord Channel Webhook alert handler.
TODO
Screenshot of test notification in Discord client