Add entity_display_name for VictorOps, use better state_message #769

siavashs · 2017-05-04T12:10:28Z

This PR fixes #580 and uses a better state_message since this field has a high character limit and is intended for a long, verbose, explanation of the problem.

More details about REST endpoint can be found here.

brian-brazil

Do you have a pointer to the docs for this field?

brian-brazil · 2017-05-04T12:18:39Z

template/default.tmpl

@@ -46,9 +46,19 @@ Alerts Resolved:
 {{ define "opsgenie.default.source" }}{{ template "__alertmanagerURL" . }}{{ end }}


-{{ define "victorops.default.state_message" }}{{ template "__subject" . }} | {{ template "__alertmanagerURL" . }}{{ end }}
-{{ define "victorops.default.monitoring_tool" }}{{ template "__alertmanager" . }}{{ end }}
+{{ define "victorops.default.state_message" }}{{ template "__alertmanager" . }}: {{ template "__alertmanagerURL" . }}


Please don't create additional default formats, use what we already have.

I'm using __subject as entity_display_name now, and using a more verbose state_message(similar to opsgenie.default.description) instead of __subject.
In other words having __subject in 2 different fields is not very useful.

Having to maintain a format per notifier is not desirable, this should be a copy (preferably a template) of what we already have for this type of long-form field.

Understood, I will use the same template used for opsgenie.default.description.

siavashs · 2017-05-04T12:31:55Z

Please see entity_display_name and state_message under Required / Recommended Fields

siavashs · 2017-05-09T11:59:05Z

Not sure if this is the right place to mention this: having the generated binary files in repository causes problems with rebasing forks and PRs.

mxinden · 2017-05-09T13:14:59Z

@siavashs In regards to the generated binary files: We plan to address this with the new UI in the future (See ui-rewrite branch)

devinodaniel · 2017-06-08T19:06:57Z

@siavashs Thanks for your work on this thus far. Is there anything I could do to help with getting this merged? Having entity_display_name would be sweet as we use VO but having the subject of an alert be a random hash is killing our mo-jo.

siavashs · 2017-06-09T09:16:50Z

@devinodaniel I'm still waiting for maintainers to merge this. Regarding the entity_id the documentation says:

ID of the Incident [String]
This field is the identity of the incident and must remain the same throughout the life-cycle of an incident. Make sure you use the same entity_id when triggering, acking, or resolving an incident. Example: "/", i.e... "diskspace/db01.mycompany.com"
If no entity_id is included, a random string will be used.

alertmanager uses sha256 hash of the alert group key for this, so it can later resolve the incident using the same entity_id.
It is possible to generate a string like the above example but I'd like the maintainers to comment on this.

ramiamar · 2017-06-22T09:51:08Z

+1
We @ alooma.com would also love to be able to set the entity_display_name

stuartnelson3 · 2017-06-22T11:02:56Z

@siavashs we've talked about how to generate identifiers that would lend themselves to being used as entity_id, but I don't think we've made any active progress on it.

Could you fix the conflict with config/notifiers.go? I'll merge after that, sorry for the long delay.

siavashs · 2017-06-26T09:36:32Z

@stuartnelson3 done, not sure why travis-ci is failing though.

mxinden · 2017-06-26T11:12:52Z

@siavashs Your /template/internal/deftmpl/bindata.go is out of sync, just run make assets and add bindata.go to your commit.

siavashs · 2017-06-26T11:29:17Z

@mxinden When I try to build the assets I get this error:

> make assets
>> writing assets
bindata: Failed to stat input path 'ui/app/script.js': lstat ui/app/script.js: no such file or directory
make: *** [assets] Error 1

mxinden · 2017-06-26T11:44:15Z

@siavashs Ah, yes. This will be fixed with #792 but I haven't updated the PR yet. Can you comment out line 63 - 68 and then try again?

mxinden · 2017-06-26T12:15:51Z

Cool, I guess that worked. I would still like @stuartnelson3 to look over this, as he was more involved so far.

caarlos0

lgtm

stuartnelson3 · 2017-06-27T20:30:38Z

Hey, I'm on vacation for the next two weeks. If this looks good to @mxinden then I say go for it, otherwise I will look when I am back

…

On Tue, Jun 27, 2017, 15:59 Carlos Alexandro Becker < ***@***.***> wrote: ***@***.**** approved this pull request. lgtm — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#769 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVVWBSp2wXs4nL-hCZRUagXg_F2aRPrks5sIQrFgaJpZM4NQlRG> .

fabxc · 2017-06-28T08:50:06Z

@siavashs, sorry about not looking at this earlier. This generally looks good to me. The documentation on state_message is pretty vague by saying "This field has a high character limit...".

We have had problems in the past by our generated messages causing receiver APIs to reject our notifications, which is pretty bad obviously.
Given that these changes put a full list of all alerts into this field and there can be hundreds of alerts in a single notification, we should add some safety checks here.

Can we find out what the actual maximum length is and then truncate the generated text for this field to that length? Limiting to something like 4KB is probably sensible.

siavashs · 2017-06-28T09:20:14Z

@fabxc good point, I sent a request to VictorOps support regarding the maximum length of state_message.

siavashs · 2017-06-28T15:50:39Z

Response from VictorOps support:

The maximum length for the state_message field is 20KB.

I will add a check for state_message length and truncate it if exceeded 20KB.

fabxc · 2017-07-03T09:44:47Z

Thanks

devinodaniel · 2017-07-05T16:32:36Z

Thanks all for getting this merged in, any chance an update to the docs can be made to reflect this capability?

This is clearer behavior and users will notice and fix their textfiles faster than if we just output a warning.

brian-brazil reviewed May 4, 2017

View reviewed changes

siavashs mentioned this pull request May 4, 2017

Docuement VictorOps receiver for the alertmanager prometheus/docs#604

Closed

caarlos0 approved these changes Jun 27, 2017

View reviewed changes

Siavash Safi added 4 commits June 29, 2017 14:44

Add entity_display_name for VictorOps, use better state_message

abcc5e4

Reuse existing long-form templates for state_message

ea6ee30

Rebuild binaries

a686949

Limit state_message to 20K

311a9a1

fabxc merged commit d5f0d17 into prometheus:master Jul 3, 2017

siavashs mentioned this pull request Jul 7, 2017

Update VictorOps docs prometheus/docs#785

Merged

mxinden mentioned this pull request Jul 15, 2017

*: cut v0.8.0 #906

Closed

hh pushed a commit to ii/alertmanager that referenced this pull request Feb 28, 2018

Treat custom textfile metric timestamps as errors (prometheus#769)

864a6ee

This is clearer behavior and users will notice and fix their textfiles faster than if we just output a warning.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add entity_display_name for VictorOps, use better state_message #769

Add entity_display_name for VictorOps, use better state_message #769

siavashs commented May 4, 2017

brian-brazil left a comment

brian-brazil May 4, 2017

siavashs May 4, 2017

brian-brazil May 4, 2017

siavashs May 4, 2017

siavashs commented May 4, 2017 •

edited

Loading

siavashs commented May 9, 2017

mxinden commented May 9, 2017

devinodaniel commented Jun 8, 2017

siavashs commented Jun 9, 2017

ramiamar commented Jun 22, 2017

stuartnelson3 commented Jun 22, 2017

siavashs commented Jun 26, 2017

mxinden commented Jun 26, 2017

siavashs commented Jun 26, 2017

mxinden commented Jun 26, 2017

mxinden commented Jun 26, 2017

caarlos0 left a comment

stuartnelson3 commented Jun 27, 2017 via email

fabxc commented Jun 28, 2017

siavashs commented Jun 28, 2017

siavashs commented Jun 28, 2017

fabxc commented Jul 3, 2017

devinodaniel commented Jul 5, 2017 •

edited

Loading

Add entity_display_name for VictorOps, use better state_message #769

Add entity_display_name for VictorOps, use better state_message #769

Conversation

siavashs commented May 4, 2017

brian-brazil left a comment

Choose a reason for hiding this comment

brian-brazil May 4, 2017

Choose a reason for hiding this comment

siavashs May 4, 2017

Choose a reason for hiding this comment

brian-brazil May 4, 2017

Choose a reason for hiding this comment

siavashs May 4, 2017

Choose a reason for hiding this comment

siavashs commented May 4, 2017 • edited Loading

siavashs commented May 9, 2017

mxinden commented May 9, 2017

devinodaniel commented Jun 8, 2017

siavashs commented Jun 9, 2017

ramiamar commented Jun 22, 2017

stuartnelson3 commented Jun 22, 2017

siavashs commented Jun 26, 2017

mxinden commented Jun 26, 2017

siavashs commented Jun 26, 2017

mxinden commented Jun 26, 2017

mxinden commented Jun 26, 2017

caarlos0 left a comment

Choose a reason for hiding this comment

stuartnelson3 commented Jun 27, 2017 via email

fabxc commented Jun 28, 2017

siavashs commented Jun 28, 2017

siavashs commented Jun 28, 2017

fabxc commented Jul 3, 2017

devinodaniel commented Jul 5, 2017 • edited Loading

siavashs commented May 4, 2017 •

edited

Loading

devinodaniel commented Jul 5, 2017 •

edited

Loading