From d025523a8b8b30e3ea4862d73e5792953cdea525 Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Wed, 15 Jan 2025 16:11:38 -0500 Subject: [PATCH] Alerting: Log reason for taking image. (#99036) --- pkg/services/ngalert/state/manager.go | 24 +++++++++++++----------- pkg/services/ngalert/state/state.go | 20 ++++++++++++++++---- pkg/services/ngalert/state/state_test.go | 2 +- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/pkg/services/ngalert/state/manager.go b/pkg/services/ngalert/state/manager.go index 619948a45a7..3194527626b 100644 --- a/pkg/services/ngalert/state/manager.go +++ b/pkg/services/ngalert/state/manager.go @@ -24,6 +24,8 @@ var ( ResendDelay = 30 * time.Second ) +type takeImageFn func(reason string) *ngModels.Image + // AlertInstanceManager defines the interface for querying the current alert instances. type AlertInstanceManager interface { GetAll(orgID int64) []*State @@ -327,21 +329,21 @@ func (st *Manager) ProcessEvalResults( logger := st.log.FromContext(ctx) // lazy evaluation of takeImage only once and only if it is requested. - var fn func() *ngModels.Image + var fn takeImageFn { var image *ngModels.Image var imageTaken bool - fn = func() *ngModels.Image { + fn = func(reason string) *ngModels.Image { if imageTaken { return image } - logger.Debug("Taking image", "dashboard", alertRule.GetDashboardUID(), "panel", alertRule.GetPanelID()) + logger.Debug("Taking image", "dashboard", alertRule.GetDashboardUID(), "panel", alertRule.GetPanelID(), "reason", reason) img, err := takeImage(ctx, st.images, alertRule) imageTaken = true if err != nil { logger.Warn("Failed to take an image", "dashboard", alertRule.GetDashboardUID(), - "panel", alertRule.GetPanelID(), + "panel", alertRule.GetPanelID(), "reason", reason, "error", err) return nil } @@ -395,7 +397,7 @@ func (st *Manager) updateLastSentAt(states StateTransitions, evaluatedAt time.Ti return result } -func (st *Manager) setNextStateForRule(ctx context.Context, alertRule *ngModels.AlertRule, results eval.Results, extraLabels data.Labels, logger log.Logger, takeImageFn func() *ngModels.Image) []StateTransition { +func (st *Manager) setNextStateForRule(ctx context.Context, alertRule *ngModels.AlertRule, results eval.Results, extraLabels data.Labels, logger log.Logger, takeImageFn takeImageFn) []StateTransition { if st.applyNoDataAndErrorToAllStates && results.IsNoData() && (alertRule.NoDataState == ngModels.Alerting || alertRule.NoDataState == ngModels.OK || alertRule.NoDataState == ngModels.KeepLast) { // If it is no data, check the mapping and switch all results to the new state // aggregate UID of datasources that returned NoData into one and provide as auxiliary info via annotationa. See: https://github.com/grafana/grafana/issues/88184 var refIds strings.Builder @@ -445,7 +447,7 @@ func (st *Manager) setNextStateForRule(ctx context.Context, alertRule *ngModels. return transitions } -func (st *Manager) setNextStateForAll(alertRule *ngModels.AlertRule, result eval.Result, logger log.Logger, extraAnnotations data.Labels, takeImageFn func() *ngModels.Image) []StateTransition { +func (st *Manager) setNextStateForAll(alertRule *ngModels.AlertRule, result eval.Result, logger log.Logger, extraAnnotations data.Labels, takeImageFn takeImageFn) []StateTransition { currentStates := st.cache.getStatesForRuleUID(alertRule.OrgID, alertRule.UID, false) transitions := make([]StateTransition, 0, len(currentStates)) updated := ruleStates{ @@ -462,7 +464,7 @@ func (st *Manager) setNextStateForAll(alertRule *ngModels.AlertRule, result eval } // Set the current state based on evaluation results -func (st *Manager) setNextState(alertRule *ngModels.AlertRule, currentState *State, result eval.Result, extraAnnotations data.Labels, logger log.Logger, takeImageFn func() *ngModels.Image) StateTransition { +func (st *Manager) setNextState(alertRule *ngModels.AlertRule, currentState *State, result eval.Result, extraAnnotations data.Labels, logger log.Logger, takeImageFn takeImageFn) StateTransition { start := st.clock.Now() currentState.LastEvaluationTime = result.EvaluatedAt @@ -538,8 +540,8 @@ func (st *Manager) setNextState(alertRule *ngModels.AlertRule, currentState *Sta currentState.ResolvedAt = nil } - if shouldTakeImage(currentState.State, oldState, currentState.Image, newlyResolved) { - image := takeImageFn() + if reason := shouldTakeImage(currentState.State, oldState, currentState.Image, newlyResolved); reason != "" { + image := takeImageFn(reason) if image != nil { currentState.Image = image } @@ -606,7 +608,7 @@ func translateInstanceState(state ngModels.InstanceStateType) eval.State { } } -func (st *Manager) deleteStaleStatesFromCache(logger log.Logger, evaluatedAt time.Time, alertRule *ngModels.AlertRule, takeImageFn func() *ngModels.Image) []StateTransition { +func (st *Manager) deleteStaleStatesFromCache(logger log.Logger, evaluatedAt time.Time, alertRule *ngModels.AlertRule, takeImageFn takeImageFn) []StateTransition { // If we are removing two or more stale series it makes sense to share the resolved image as the alert rule is the same. // TODO: We will need to change this when we support images without screenshots as each series will have a different image staleStates := st.cache.deleteRuleStates(alertRule.GetKey(), func(s *State) bool { @@ -626,7 +628,7 @@ func (st *Manager) deleteStaleStatesFromCache(logger log.Logger, evaluatedAt tim if oldState == eval.Alerting { s.ResolvedAt = &evaluatedAt - image := takeImageFn() + image := takeImageFn("stale state") if image != nil { s.Image = image } diff --git a/pkg/services/ngalert/state/state.go b/pkg/services/ngalert/state/state.go index 441efc8e8a5..7a0cf4834ce 100644 --- a/pkg/services/ngalert/state/state.go +++ b/pkg/services/ngalert/state/state.go @@ -577,10 +577,22 @@ func (a *State) IsStale() bool { // shouldTakeImage determines whether a new image should be taken for a given transition. This should return true when // newly transitioning to an alerting state, when no valid image exists, or when the alert has been resolved. -func shouldTakeImage(state, previousState eval.State, previousImage *models.Image, resolved bool) bool { - return resolved || - state == eval.Alerting && previousState != eval.Alerting || - state == eval.Alerting && (previousImage == nil || previousImage.HasExpired()) +func shouldTakeImage(state, previousState eval.State, previousImage *models.Image, resolved bool) string { + if resolved { + return "resolved" + } + if state == eval.Alerting { + if previousState != eval.Alerting { + return "transition to alerting" + } + if previousImage == nil { + return "no image" + } + if previousImage.HasExpired() { + return "expired image" + } + } + return "" } // takeImage takes an image for the alert rule. It returns nil if screenshots are disabled or diff --git a/pkg/services/ngalert/state/state_test.go b/pkg/services/ngalert/state/state_test.go index 11461f51524..2515fe33190 100644 --- a/pkg/services/ngalert/state/state_test.go +++ b/pkg/services/ngalert/state/state_test.go @@ -616,7 +616,7 @@ func TestShouldTakeImage(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, shouldTakeImage(test.state, test.previousState, test.previousImage, test.resolved)) + assert.Equal(t, test.expected, shouldTakeImage(test.state, test.previousState, test.previousImage, test.resolved) != "") }) } }