adding source for sensu alert as parameter #1314
Conversation
server/server_test.go
Outdated
| @@ -7751,6 +7756,7 @@ stream | |||
| .message('message') | |||
| .details('details') | |||
| .crit(lambda: TRUE) | |||
| .sensu() | |||
There was a problem hiding this comment.
What is this change doing? The alert should be sent to sensu via the topic handler, not explicitly in the TICKscript.
There was a problem hiding this comment.
The sensu TestServer_AlertHandlers test was failing without that being set.
server_test.go:7784: sensu: unexpected sensu request: exp [{Name:id Source:Kapacitor Output:message Status:2}] got []
Is there something I can do to make it work without that?
There was a problem hiding this comment.
This bit of code here should be registering a sensu handler for the topic test. So it should get an alert event, this hasn't changed in your code so I am not sure why you would be getting that error. But it should definitely be passing without the .sensu() bit as it was before.
There was a problem hiding this comment.
It looks like I am getting this error in services/sensu/sensutest/sensutest.go line 53
"accept tcp 127.0.0.1:48578: use of closed network connection"
Not sure what I changed that would cause this to happen. Any suggestions on how to fix the issue?
There was a problem hiding this comment.
Hmm, nothing is jumping out at me either. Maybe later I'll pull down the branch and take a look.
There was a problem hiding this comment.
some more digging found that the Sensu service Handler() method is being called however the Handle() method is not. It is called if the .sensu() is in the tick script. How does the topic handler know to use the sensu service?
services/sensu/service.go
Outdated
| return | ||
| } | ||
| sourceStr := buf.String() | ||
| buf.Reset() |
There was a problem hiding this comment.
No need to reset the buffer here as it isn't being used later.
services/alert/service.go
Outdated
| if err != nil { | ||
| return handler{}, err | ||
| } | ||
| h, err := s.SensuService.Handler(c, s.logger) |
There was a problem hiding this comment.
This is where a topic handler calls into the SensuService for the sensu handler. And it looks correct.
There was a problem hiding this comment.
I found the bug it is super subtle, the above line should read
h, err = s.SensuService.Handler(c, s.logger)
this is because we want to assign to the var h Handler from the function scope not our local scope. I should redesign this so that isn't so easy to mess up, but for now it will fix your test.
There was a problem hiding this comment.
sweet thanks for the help! will get this in right away!
server/server_test.go
Outdated
| @@ -7391,6 +7393,9 @@ func TestServer_AlertHandlers(t *testing.T) { | |||
| { | |||
| handler: client.TopicHandler{ | |||
| Kind: "sensu", | |||
| Options: map[string]interface{}{ | |||
| "Source": "Kapacitor", | |||
There was a problem hiding this comment.
The key should be lower case to follow convention.
|
@henry-megarry-tr The changes look good. :) Just a few house keeping items to finish up
Thanks for this great PR! 🎉 |
henry-megarry
force-pushed
the
dynamicSensuSource
branch
from
fd7ef9e to
9d8b9e5
Compare
henry-megarry
force-pushed
the
dynamicSensuSource
branch
from
9d8b9e5 to
ce759fd
Compare
adding source for sensu alert as parameter
|
@henry-megarry-tr Thanks, merged. Closed #1132 |
Required for all non-trivial PRs