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.

Sign up for GitHub

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

Jump to bottom

allowing sensu handler to be specified #1299

Merged
merged 1 commit into from
May 1, 2017
Merged

allowing sensu handler to be specified #1299

merged 1 commit into from
May 1, 2017

Conversation

davidloutsch
Copy link
Contributor

@davidloutsch davidloutsch commented Apr 4, 2017 •
edited
Loading

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

nathanielc
nathanielc reviewed Apr 4, 2017
View reviewed changes
Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I had a question below...

pipeline/alert.go Outdated
@@ -949,6 +949,7 @@ func (a *AlertaHandler) Services(service ...string) *AlertaHandler {
// enabled = true
// url = "http://sensu:3030"
// source = "Kapacitor"
// handlers = "sns"
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you list multiple handlers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you want different handlers per alert or global to all alerts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions. I checked in some code that would allow for a list of handlers. I only needed one, but lists work as well.
I could understand a need for having different handlers per alert, but we only need a single global one at the moment.

@nathanielc
Copy link
Contributor

Can you fix this failing test?

--- FAIL: TestServer_UpdateConfig (0.37s)
	server_test.go:6510: unexpected defaults for sensu/: sensu: section element 0 are not equal: extra option "Handlers" with value <nil>
	server_test.go:6525: unexpected update result 0 for sensu/: sensu: section element 0 are not equal: extra option "Handlers" with value <nil>

@davidloutsch
Copy link
Contributor Author

Sure thing. Let me know if you need anything else.

@nathanielc
Copy link
Contributor

I could understand a need for having different handlers per alert, but we only need a single global one at the moment.

I think it would be more complete to add the list of handlers to the each instance of a sensu handler. That way you can specify them globally and per alert. It shouldn't be too much more work and here is an open PR doing something very similar #1314 .

Do you think you could take a crack at adding Handlers on a per alert basis?

nathanielc
nathanielc reviewed Apr 12, 2017
View reviewed changes
services/sensu/config.go Outdated
@@ -11,6 +11,8 @@ type Config struct {
Addr string `toml:"addr" override:"addr"`
// The JIT sensu source name of the alert.
Source string `toml:"source" override:"source"`
// The sensu handler to use
Handlers []string `toml:"handlers"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the override tag here will make it so that the tests above use the lower case name to be consistent.

@henry-megarry henry-megarry force-pushed the addSensuHandler branch 4 times, most recently from 6095a59 to 08b7dfa Compare April 20, 2017 19:06
@henry-megarry
Copy link

henry-megarry commented Apr 25, 2017 •
edited
Loading

Hey @nathanielc, I have added the changes to allow handlers on a per alert basis. Let me know if you need anything else.

nathanielc
nathanielc suggested changes Apr 27, 2017
View reviewed changes
Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

Look great! Just one small change requested.

services/sensu/service.go Outdated
Name: "testName",
Source: "Kapacitor",
Output: "testOutput",
Handlers: []string{"testHandlers"},
Copy link
Contributor

Choose a reason for hiding this comment

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

By default I think it would be best to have an empty list of Handlers. Otherwise a default call to test the sensu service will probably fail.

@nathanielc nathanielc merged commit b6f27d5 into influxdata:master May 1, 2017
nathanielc added a commit that referenced this pull request May 1, 2017
@nathanielc
Merged pull request #1299 from thomsonreuters/addSensuHandler
4548593 
allowing sensu handler to be specified
nathanielc added a commit that referenced this pull request May 1, 2017
@nathanielc
update changelog after merge of #1299
3cff8ab
@lcaflc lcaflc mentioned this pull request Jun 29, 2017
Open
fdhex pushed a commit to fdhex/kapacitor that referenced this pull request Jun 12, 2018
@nathanielc @floriandudouet-swisscom
update changelog after merge of influxdata#1299
52d9631
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanielc nathanielc nathanielc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidloutsch @nathanielc @henry-megarry