Support adding custom fields to VictorOps notifications by jay-rob · Pull Request #1420 · prometheus/alertmanager

jay-rob · 2018-06-16T17:57:38Z

This PR is a followup to another PR that was opened #1315.

These changes allow users to define additional fields they would like to pass on to VictorOps and use templates to set the values of those fields.

Example:

- name: default
  victorops_configs:
  - entity_display_name: '{{ .CommonAnnotations.summary}}'
    message_type: '{{ .CommonLabels.severity }}'
    routing_key: default
    state_message: '{{ .CommonAnnotations.description }}'
    custom_fields:
      tier: '{{ .CommonLabels.tier }}'
      storage: '{{ .CommonLabels.storage }}'

Logic has been added to prevent a custom field colliding with/overriding a statically defined field. When this occurs, the field will simply be ignored and a debug level log message will notify of this.

simonpasquier

A few comments but looks good overall.

simonpasquier · 2018-06-18T10:32:32Z

notify/impl.go

+	return n.retry(resp.StatusCode)
+}
+
+//Create the json payload to be sent to victorops api


(nit) add a space before Create + missing dot + s/json/JSON/

fixed the space. whats the second part about the DOT?

notify/impl.go

simonpasquier · 2018-06-18T10:39:24Z

notify/impl.go

+				return nil, fmt.Errorf("templating error: %s", err)
+			}
+		} else {
+			level.Debug(n.logger).Log("msg", "Ignoring custom field %q as it is already defined as a fixed field", "omitted_field", k, "incident", key)


You need to remove the %q formatter in the message.

level.Debug(n.logger).Log("msg", "Ignoring custom field as it is already defined as a fixed field", "field", k, "incident", key)

jay-rob · 2018-06-18T14:10:28Z

Thank you for the feedback @simonpasquier . I have incorporated the changes

simonpasquier

Still a few minor nits. I guess you have the opportunity to test the change against the VictorOps API?

simonpasquier · 2018-06-18T14:45:12Z

notify/impl.go

+	return n.retry(resp.StatusCode)
+}
+
+// Create the json payload to be sent to victorops api


missing dot

Either add a dot at the end or simply remove the comment since it is pretty clear from the function name what it is doing.

notify/impl_test.go

simonpasquier · 2018-06-18T14:45:40Z

notify/impl_test.go

+	//Verify the invalid custom field didn't override the static field
+	require.Equal(t, "message", m["state_message"])
+
+	//Verify that a custom field was added to the payload and templatized


missing space and dot

notify/impl.go

jay-rob · 2018-06-23T23:04:54Z

Sorry for the delay. I went ahead and added validation to the config load logic of Victorops. I left the notifier logic in place, it felt appropriate to keep it there as an extra safety net. Thanks for the feedback @brian-brazil

simonpasquier

Only a few comments.

config/notifiers.go

notify/impl.go

simonpasquier · 2018-06-25T15:26:23Z

notify/impl_test.go

+		MonitoringTool:    `AM`,
+		CustomFields: map[string]string{
+			"Field_A": "{{ .CommonLabels.Message }}",
+			//A field that should be ignored


L291-292 to be removed

notify/impl_test.go

simonpasquier

A few minor comments otherwise LGTM.

simonpasquier · 2018-07-04T10:27:52Z

CHANGELOG.md

 * [CHANGE] Validate Slack field config and only allow the necessary input (#1334)
 * [CHANGE] Remove legacy alert ingest endpoint (#1362)
-* [CHANGE] Moved to memberlist as underlying gossip protocol
+* [CHANGE] Move to memberlist as underlying gossip protocol including cluster flag changes from --mesh.xxx to --cluster.xxx (#1232)


Not sure why you got this change included.

config/notifiers.go

simonpasquier · 2018-07-04T10:36:10Z

notify/impl.go

+	return n.retry(resp.StatusCode)
+}
+
+// Create the json payload to be sent to victorops api


Either add a dot at the end or simply remove the comment since it is pretty clear from the function name what it is doing.

notify/impl_test.go

sta-szek · 2018-12-03T07:58:20Z

Hi, any progress here? I would like to use that feature. :)

simonpasquier · 2018-12-03T09:55:32Z

@jrthrawny can you resolve the conflicts and address the comments?

valery-lezhebokov · 2018-12-04T11:53:26Z

Hi,
I would be interested in that feature too.

jay-rob · 2018-12-08T23:50:20Z

Hello. I will rebase this PR and incorporate the changes suggested in feedback

Signed-off-by: Jason Roberts <jroberts@drud.com>

jay-rob · 2018-12-10T05:14:05Z

I've rebased this from master and cleaned up the comment grammar. Let me know if there is anything else I need to do. Sorry again for the delay

simonpasquier

LGTM. Only one small remark on a comment.

simonpasquier · 2018-12-10T10:50:00Z

notify/impl.go

+	return n.retry(resp.StatusCode)
+}
+
+// Create the json payload to be sent to victorops api.


s/json/JSON/
s/victorops api/the VictorOps API/

Thanks, just pushed the fix

simonpasquier

You miss the DCO on the last commit otherwise 👍

Signed-off-by: Jason Roberts <jroberts@drud.com>

jay-rob · 2018-12-11T19:40:47Z

signed :)

stuartnelson3 · 2019-01-15T10:59:31Z

Just saw this! Maybe @mxinden can add it to the 0.16.0 beta pr :)

) * Support adding custom fields to VictorOps notifications * Response to feedback * Added logic to validate victorops custom fields to config load time * Cleanup victorops notifier of logic duplicated in config check * rebase and further cleanup from feedback * another grammer fix Signed-off-by: Jason Roberts <jroberts@drud.com>

mxinden · 2019-01-15T13:22:51Z

Thanks for the pointer @stuartnelson3. Added to #1708.

jay-rob force-pushed the victorop-custom-fields branch 2 times, most recently from 48efbb4 to 5a9deb0 Compare June 17, 2018 14:00

simonpasquier requested changes Jun 18, 2018

View reviewed changes

simonpasquier reviewed Jun 18, 2018

View reviewed changes

brian-brazil reviewed Jun 18, 2018

View reviewed changes

notify/impl.go Outdated Show resolved Hide resolved

jay-rob force-pushed the victorop-custom-fields branch from 93f5a70 to 31a0576 Compare June 23, 2018 23:00

simonpasquier requested changes Jun 25, 2018

View reviewed changes

jay-rob force-pushed the victorop-custom-fields branch from 9561337 to e32e5d0 Compare June 26, 2018 16:06

simonpasquier requested changes Jul 4, 2018

View reviewed changes

jay-rob added 5 commits December 9, 2018 21:34

Support adding custom fields to VictorOps notifications

3ab5bbc

Signed-off-by: Jason Roberts <jroberts@drud.com>

Response to feedback

863d52c

Signed-off-by: Jason Roberts <jroberts@drud.com>

Added logic to validate victorops custom fields to config load time

e108c15

Signed-off-by: Jason Roberts <jroberts@drud.com>

Cleanup victorops notifier of logic duplicated in config check

2ce0048

Signed-off-by: Jason Roberts <jroberts@drud.com>

rebase and further cleanup from feedback

4952cc0

Signed-off-by: Jason Roberts <jroberts@drud.com>

jay-rob force-pushed the victorop-custom-fields branch from e32e5d0 to 4952cc0 Compare December 10, 2018 04:28

simonpasquier reviewed Dec 10, 2018

View reviewed changes

simonpasquier approved these changes Dec 10, 2018

View reviewed changes

another grammer fix

b23f2c6

Signed-off-by: Jason Roberts <jroberts@drud.com>

jay-rob force-pushed the victorop-custom-fields branch from 4d0a73f to b23f2c6 Compare December 10, 2018 16:01

stuartnelson3 merged commit b02afca into prometheus:master Jan 15, 2019

Conversation

jay-rob commented Jun 16, 2018

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jay-rob commented Jun 18, 2018

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jay-rob commented Jun 23, 2018

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sta-szek commented Dec 3, 2018

Uh oh!

simonpasquier commented Dec 3, 2018

Uh oh!

valery-lezhebokov commented Dec 4, 2018

Uh oh!

jay-rob commented Dec 8, 2018

Uh oh!

jay-rob commented Dec 10, 2018

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

jay-rob commented Dec 11, 2018

Uh oh!

stuartnelson3 commented Jan 15, 2019

Uh oh!

mxinden commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants