Skip to content

Alert history log support#5602

Closed
emanlodovice wants to merge 3 commits intocortexproject:masterfrom
emanlodovice:alert-history-log
Closed

Alert history log support#5602
emanlodovice wants to merge 3 commits intocortexproject:masterfrom
emanlodovice:alert-history-log

Conversation

@emanlodovice
Copy link
Contributor

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Emmanuel Lodovice <lodovice@amazon.com>
@yeya24
Copy link
Contributor

yeya24 commented Oct 18, 2023

IIUC, this requires some prs to upstream alertmanager first?

@emanlodovice
Copy link
Contributor Author

emanlodovice commented Oct 18, 2023

yes, this PR needs this PR in upstream alertmanager prometheus/alertmanager#3461 so I am keeping it as draft so I can get some preliminary review

Signed-off-by: Emmanuel Lodovice <lodovice@amazon.com>

func (o *LogAlertLifeCycleObserver) PipelineStart(alerts []*types.Alert, meta alertobserver.AlertEventMeta) {
logLvl := o.limiter.Level()
if logLvl < 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we create a function as shouldLog? so that we can put all level control logic in there

reason = "Unknown"
}
for _, a := range alerts {
o.logWithAlert(a, true, "msg", "Rejected", "reason", reason)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need reject per log? maybe log rejected alerts in the same line better for less log volume?

return
}
for _, a := range alerts {
o.logWithAlert(a, true, "msg", "Added to aggregation group", "groupKey", groupKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Signed-off-by: Emmanuel Lodovice <lodovice@amazon.com>
"github.com/prometheus/alertmanager/cluster/clusterpb"
"github.com/prometheus/alertmanager/config"
"github.com/prometheus/alertmanager/dispatch"
"github.com/prometheus/alertmanager/featurecontrol"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this belong to this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to support the latest changes in AM. This change prometheus/alertmanager#3045 adds a featurecontrol parameter to NewPipelineBuilder.

@qinxx108
Copy link
Contributor

LGTM

@stale
Copy link

stale bot commented Apr 26, 2025

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 26, 2025
@stale stale bot closed this Jul 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants