Add GroupFingerprint to webhook notifications

[kfdm]

When dealing with downstream alert systems, it's useful to have a single fingerprint to use as a lookup. Similar to the Fingerprint field on individual alert messages, fingerprinting the group allows downstream alert systems to recognize the same Fingerprint.

A GroupFingerprint based off of GroupLabels should be generally stable, since group_by is controlled by the Alertmanager configuration, while CommonLabels could potentially change based on alerts firing and being resolved.

[simonpasquier]

We already have groupKey in the webhook's payload. Isn't it sufficient for this purpose?

[kfdm]

While groupKey is similar, I thought that using the same Fingerprint function used elsewhere would provide a more consistent identifier that matches with how it is used in individual alerts.

[kfdm]

In 3269bc3 (before the large refactor in #1929 ) GroupKey was changed from uint64 to a string, however for custom integrations like Pagerduty ( IncidentKey: hashKey(key),) and Victor Ops ( EntityID: hashKey(key), ), they still use a fixed length string for handling upstream integrations.

// hashKey returns the sha256 for a group key as integrations may have
// maximum length requirements on deduplication keys.

A fixed length GroupFingerprint works the same way, allowing downstream implementations to be simpler, without having a groupKey which does not match up to the groupLabels

[lzh-lab]

I think this is a good idea, in my case, I also calcute a groupFingerprint in my db.

[simonpasquier]

It seems like there's a demand for it...
@kfdm could you also update the documentation at https://github.com/prometheus/alertmanager/blob/master/docs/configuration.md#webhook_config?

[brian-brazil]

I'd suggest that anyone who needs a short fingerprint calculate it themselves, but in general to use the full labelset as a key.

This would cause confusion with GroupKey - which sounds confusingly similar. In this particular case, is sounds like the existing GroupKey might actually be what you want.

We've had issues with collisions on these in client libraries and PromQL. So it's not something we should expose to users, and should be looking to remove from any public-facing APIs we currently have, as they'll likely run into the same issues we have over the years.
We only use a hash for Pagerdury/Victorops due to technical limitations, if we could we'd pass the whole GroupKey.

[kfdm]

It seems like there's a demand for it...
@kfdm could you also update the documentation at https://github.com/prometheus/alertmanager/blob/master/docs/configuration.md#webhook_config?

Done

[simonpasquier]

Based on @brian-brazil feedback, I conclude that it's not safe to include GroupFingerprint in the webhook payload. Sorry that you spent some time on this @kfdm.

[kfdm]

I feel like I need to respectfully disagree a bit.

While there is maybe a small concern for collisions, I think having a fingerprint helps simplify a majority of lookups downstream and from my perspective, there is no additional burden on alertmanager by including a fingerprint.

As for the argument of "whoever who needs a fingerprint, should calculate it themselves", then I am curious the reason for having a fingerprint in the alert labelset, or even the groupkey, if the assumption is that downstream should calculate everything.

[brian-brazil]

I consider almost all of common/model to be legacy code, and that the fingerprint in particular to have been unwise given all the problems it has causes us over the years. FNV-1 is not a good hash. Accordingly I think we should be looking to remove uses of them across the code base, and adding it to public APIs would go against that.

More generally, avoid fingerprints when you can use guaranteed unique values instead.

the groupkey, if the assumption is that downstream should calculate everything.

The groupkey cannot be calculated by downstream, which is why it's there: as a unique identifier for a given route's group.

[SoloJacobs]

Hi @kfdm ,

I'm trying to remove some clutter, so closing this PR. Sorry it was declined.

Kind regards, Solomon Jacobs