search

influxdata / kapacitor

Open source framework for processing, monitoring, and alerting on time series data
MIT License
2.31k stars 492 forks source link

OpsGenie alerts description is not the AlertNode Details #1130

Open fdhex opened 7 years ago

fdhex commented 7 years ago

Using the current version of the OpsGenie service for alerting, I assumed that the Description field within the alert created on OpsGenie would be the Details property of the alert node (event.State.Details), but it is in fact hardcoded to event.Data.Result, which is the unmodifiable result data of the InfluxQL query which resulted in the alert's creation.

You can see it there: https://github.com/influxdata/kapacitor/blob/master/services/opsgenie/service.go#L224

It seems more intuitive to me as well as more inline with the other alerting services to link the description of the alert in OpsGenie, which accepts HTML code, to the customizable Details property. A rough implementation can be found here: https://github.com/fdhex/kapacitor/commit/95d60595dea8c524d451ed9f43df6271a57e9b4e

I would propose this implementation to be the new default, but I think it would be important to keep the previous functionality as an optional choice. Pinging the original author of the service @ericiles for opinion here.

nathanielc commented 7 years ago

@fdhex Thanks for the detailed write up, I agree leaving the original behavior in tack makes sense as well as enabling the new behavior. I'll defer to @ericiles on further input, but the request seems reasonable to me.

ericiles commented 7 years ago

I agree that if the details property is used, it should definitely take priority over the current functionality, falling back to the current functionality otherwise. This will allow those that upgrade in the future to not have to modify their OpsGenie rules unless they choose to use the details property to customize what gets sent to OpsGenie.

fdhex commented 7 years ago

@ericiles An issue I can see is that Details has a default value (of {{ .json }}), so I'm not sure how to make that transparent so that old rules still get the same output, if they are not modified at all.

ericiles commented 7 years ago

@fdhex if that is the case, and the output contains the same information, just in a slightly different format, perhaps this simply needs to be a breaking change, and as such, rules on the OpsGenie side will need to be modified accordingly. Otherwise, the only other solution would be to require an additional property be set on the rule in order to use the details property, which doesn't seem optimal, as it would be completely different from the other services available.Thoughts @nathanielc ?

andreasnuesslein commented 6 years ago

Hi all!

may I inquire to the status here? :) I agree with @fdhex 's approach but don't understand the issue with Default currently being {{ .json }}. This can still be true for OpsGenie if people don't set |alert().details("...").

Frankly I think people will always prefer having the option to configure the Details rather than just have json in an interface that does not particularly endorse it.

andreasnuesslein commented 6 years ago

If it helps, I can merge @fdhex 's first draft with the option to just fall back to {{ .json }} if nothing is defined and create a PR

andreasnuesslein commented 6 years ago

I ended up copying most of @fdhex 's changes. I realized that it's actually not trivial to not break the old behavior without creating a bunch of extra code. owever I believe that the old behavior was nothing people really liked. The PR is now html.UnEscape-ing just in case. If .details() is not set, at least it's not printing

{"Name":"system","TaskName":"...

But rather: {"Name":"system","TaskName":"...