Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alertmanager: Change timestamp label to .StartsAt #795

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

tdemin
Copy link
Contributor

@tdemin tdemin commented Apr 13, 2024

notification-controller posted all outgoing Alertmanager alerts with "timestamp" label, effectively preventing grouping alerts related to the same resource and forcing users to configure a separate alert receiver with send_resolved: false.

This changes it to instead set "startsAt", which was previously set (automatically by Alertmanager) to alert posting time. "endsAt" remains unset, as we have no way of figuring that out but the reconciliation interval of the resource that generated the alert, which can currently only be found out by making a Kubernetes API round-trip.

Note that this requires users to adapt alert templates that relied on .Labels.Timestamp.

Fixes #793.

@stefanprodan stefanprodan added the area/alerting Alerting related issues and PRs label Apr 15, 2024
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notification-controller posted all outgoing Alertmanager alerts with
"timestamp" label, effectively preventing grouping alerts related to the
same resource and forcing users to configure a separate alert receiver
with `send_resolved: false`.

This changes it to instead set "startsAt", which was previously set
(automatically by Alertmanager) to alert posting time. "endsAt" remains
unset, as we have no way of figuring that out but the reconciliation
interval of the resource that generated the alert, which can currently
only be found out by making a Kubernetes API round-trip.

Note that this requires users to adapt alert templates that relied on
.Labels.Timestamp.

Signed-off-by: Timur Demin <me@tdem.in>
@tdemin
Copy link
Contributor Author

tdemin commented Apr 27, 2024

@stefanprodan sorry for taking a while to update this. I've expanded the docs on this.

While we're at it, I also updated fmt.Stringer / json.Marshaler implementations of AlertManagerTime to use value receiver types instead of pointers, as this is what Go stdlib docs tell us to do:

Programs using times should typically store and pass them as values, not pointers. That is, time variables and struct fields should be of type time.Time, not *time.Time.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @tdemin 🏅

@stefanprodan stefanprodan merged commit 6d79130 into fluxcd:main Apr 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alerts sent to Alertmanager contain timestamps as labels, preventing alert grouping
2 participants