Alerta support for timeout, tags and service template #1545

m4ce · 2017-08-31T16:53:33Z

Add template support to service field as requested in #1543
Add timeout support as requested in #1264
Add tags support as requested in #1546

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated
Sign CLA (if not already signed)

m4ce · 2017-09-01T22:20:42Z

not exactly sure why it's failing over one test :/

nathanielc

Thanks! These changes look good. I have made a few small comments below, mostly relating to using toml.Duartion vs time.Duration.

nathanielc · 2017-09-01T23:03:28Z

services/alerta/service.go

@@ -22,6 +24,7 @@ const (
 	defaultResource    = "{{ .Name }}"
 	defaultEvent       = "{{ .ID }}"
 	defaultGroup       = "{{ .Group }}"
+	defaultTimeout     = toml.Duration(86400 * time.Second)


This reads a little better as time.Duration(24*time.Hour). Also once the duration has been parsed from the config its best to use time.Duration so that normal time operations work.

nathanielc · 2017-09-01T23:03:58Z

services/alerta/service.go

@@ -69,6 +73,7 @@ func (s *Service) TestOptions() interface{} {
 		Message:     "test alerta message",
 		Origin:      c.Origin,
 		Service:     []string{"testServiceA", "testServiceB"},
+		Timeout:     toml.Duration(86400 * time.Second),


Use the constant defined above here.

nathanielc · 2017-09-01T23:04:43Z

services/alerta/service.go

@@ -185,6 +192,10 @@ func (s *Service) preparePost(token, tokenPrefix, resource, event, environment,
 		origin = c.Origin
 	}

+	if timeout == toml.Duration(0) {


This can be simplified as if timeout == 0 { Go will infer the type of the constant numeric literal.

nathanielc · 2017-09-01T23:06:04Z

services/alerta/service.go

+	Message     string        `json:"message"`
+	Origin      string        `json:"origin"`
+	Service     []string      `json:"service"`
+	Timeout     toml.Duration `json:"timeout"`


Use time.Duration

nathanielc · 2017-09-01T23:06:34Z

services/alerta/service.go

@@ -125,12 +132,12 @@ func (s *Service) Update(newConfig []interface{}) error {
 	return nil
 }

-func (s *Service) Alert(token, tokenPrefix, resource, event, environment, severity, group, value, message, origin string, service []string, data models.Result) error {
+func (s *Service) Alert(token, tokenPrefix, resource, event, environment, severity, group, value, message, origin string, service []string, timeout toml.Duration, tags map[string]string, data models.Result) error {


And here as well you can use time.Duration

nathanielc · 2017-09-01T23:06:56Z

services/alerta/service.go

@@ -158,7 +165,7 @@ func (s *Service) Alert(token, tokenPrefix, resource, event, environment, severi
 	return nil
 }

-func (s *Service) preparePost(token, tokenPrefix, resource, event, environment, severity, group, value, message, origin string, service []string, data models.Result) (*http.Request, error) {
+func (s *Service) preparePost(token, tokenPrefix, resource, event, environment, severity, group, value, message, origin string, service []string, timeout toml.Duration, tags map[string]string, data models.Result) (*http.Request, error) {


And again here time.Duration.

nathanielc · 2017-09-01T23:07:39Z

services/alerta/service.go

@@ -204,6 +215,13 @@ func (s *Service) preparePost(token, tokenPrefix, resource, event, environment,
 	if len(service) > 0 {
 		postData["service"] = service
 	}
+	postData["timeout"] = int64(timeout / toml.Duration(time.Second))


Once the types are changed to time.Duration this can be simplified as int64(timeout/time.Second)

nathanielc · 2017-09-01T23:07:55Z

services/alerta/service.go

@@ -262,6 +280,9 @@ type HandlerConfig struct {

 	// List of effected Services
 	Service []string `mapstructure:"service"`
+
+	// Timeout
+	Timeout toml.Duration `mapstructure:"timeout"`


And again here time.Duration

nathanielc · 2017-09-01T23:11:58Z

services/alerta/config.go

@@ -22,6 +23,8 @@ type Config struct {
 	Environment string `toml:"environment" override:"environment"`
 	// The origin of the alert.
 	Origin string `toml:"origin" override:"origin"`
+	// Optional timeout.
+	Timeout toml.Duration `toml:"timeout" override:"timeout"`


Leave this one as toml.Duration so it parses from the configuration.

nathanielc · 2017-09-01T23:13:48Z

services/alerta/service.go

@@ -204,6 +215,13 @@ func (s *Service) preparePost(token, tokenPrefix, resource, event, environment,
 	if len(service) > 0 {
 		postData["service"] = service
 	}
+	postData["timeout"] = int64(timeout / toml.Duration(time.Second))
+
+	_tags := make([]string, 0)


Nit tagList would be a better name here.

m4ce · 2017-09-02T21:06:07Z

@nathanielc, thanks for the feedback. I've implemented the requested changes. I'm now defaulting timeout using the defaultHanderConfig, that should be okay too?

Looking at it now, there's one test that doesn't go through but can't quite figure out why.

m4ce · 2017-09-03T18:13:54Z

@nathanielc, I've adjusted the tests and are now passing. Hope it looks good now.

m4ce · 2017-09-08T15:27:26Z

@nathanielc, any chance you could review this before cutting the next release? Thanks.

Alerta support for timeout, tags and service template

nathanielc · 2017-09-08T15:52:44Z

@m4ce Thanks for the PR!

m4ce force-pushed the feature/alerta_service_template branch 2 times, most recently from 14a959a to 7416243 Compare August 31, 2017 17:06

m4ce changed the title ~~Alerta service template support~~ Alerta support for timeout and service template Sep 1, 2017

m4ce changed the title ~~Alerta support for timeout and service template~~ Alerta support for timeout, tags and service template Sep 1, 2017

m4ce force-pushed the feature/alerta_service_template branch 10 times, most recently from 80099ef to cd7f1c4 Compare September 1, 2017 21:49

nathanielc suggested changes Sep 1, 2017

View reviewed changes

m4ce force-pushed the feature/alerta_service_template branch 4 times, most recently from 63693e6 to 66f6059 Compare September 2, 2017 21:04

m4ce force-pushed the feature/alerta_service_template branch from 749a993 to 5dfcdaf Compare September 3, 2017 15:10

m4ce force-pushed the feature/alerta_service_template branch from 1cfcab2 to b75b022 Compare September 3, 2017 18:13

Alerta support for timeout, tags and service template

d73ed51

m4ce force-pushed the feature/alerta_service_template branch from b75b022 to d73ed51 Compare September 3, 2017 19:26

nathanielc approved these changes Sep 8, 2017

View reviewed changes

nathanielc merged commit d73ed51 into influxdata:master Sep 8, 2017

nathanielc added a commit that referenced this pull request Sep 8, 2017

Merged pull request #1545 from m4ce/feature/alerta_service_template

d571d95

Alerta support for timeout, tags and service template

This was referenced Sep 13, 2017

Alerta alertnode service template #1543

Closed

Alerta Integration: Variables not being evaluated within services() declaration #1556

Closed

Feature: alerta timeout field and durationField question #1264

Closed

Alerta tags not set #1546

Closed

nogweii mentioned this pull request Nov 28, 2017

[feature request] - Alerta - Allow service to be a template and allow timeouts #494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerta support for timeout, tags and service template #1545

Alerta support for timeout, tags and service template #1545

m4ce commented Aug 31, 2017 •

edited

Loading

m4ce commented Sep 1, 2017

nathanielc left a comment

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

nathanielc Sep 1, 2017

m4ce commented Sep 2, 2017 •

edited

Loading

m4ce commented Sep 3, 2017 •

edited

Loading

m4ce commented Sep 8, 2017

nathanielc commented Sep 8, 2017

Alerta support for timeout, tags and service template #1545

Alerta support for timeout, tags and service template #1545

Conversation

m4ce commented Aug 31, 2017 • edited Loading

Required for all non-trivial PRs

m4ce commented Sep 1, 2017

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4ce commented Sep 2, 2017 • edited Loading

m4ce commented Sep 3, 2017 • edited Loading

m4ce commented Sep 8, 2017

nathanielc commented Sep 8, 2017

m4ce commented Aug 31, 2017 •

edited

Loading

m4ce commented Sep 2, 2017 •

edited

Loading

m4ce commented Sep 3, 2017 •

edited

Loading