config/notifiers: PagerDuty details override defaults

[BenoitKnecht]

According to the Alertmanager documentation, the details option of pagerduty_config defaults to

[ details: { <string>: <tmpl_string>, ... } | default = {
  firing:       '{{ template "pagerduty.default.instances" .Alerts.Firing }}'
  resolved:     '{{ template "pagerduty.default.instances" .Alerts.Resolved }}'
  num_firing:   '{{ .Alerts.Firing | len }}'
  num_resolved: '{{ .Alerts.Resolved | len }}'
} ]

Which suggests that if details is provided in the configuration, the entire option is overridden.

But instead, the individual keys of details are being set to the values above if the corresponding key isn't present in the configuration. This means it's impossible to remove those keys entirely; setting them to an empty string would still have them displayed in PagerDuty.

This commit implements the documented behavior, i.e. it sets PagerDutyConfig.Details to DefaultPagerdutyDetails only if the former is nil.

[BenoitKnecht]

While I think this corresponds to the documented behavior, I recognize that it would break configuration that relied on the previous implementation.

If you prefer, I can propose a patch that keeps the old behavior but removes the details keys whose values are empty strings, so that it become possible to undefine those default keys.

[levshvarts]

While I think this corresponds to the documented behavior, I recognize that it would break configuration that relied on the previous implementation.

If you prefer, I can propose a patch that keeps the old behavior but removes the details keys whose values are empty strings, so that it become possible to undefine those default keys.

Seems like, from the backwards compatibility point of view, this is the way to go. And even though having an empty string hold a special significance is not super clean, this would not go against any other types of default values in prometheus/alertmanager.

[Preisschild]

Just ran into the same unexpected behavior. Is there a reason why this wasn't reviewed?

[Aaron-ML]

@levshvarts @BenoitKnecht Any chance we can get some eyes on this? Running into this myself and I was able to blank them out in my template however pagerduty stops ordering these fields after exceeding 32 fields total, including blank fields.