From cb6124c92113040d723adfd51efe75b99755d354 Mon Sep 17 00:00:00 2001 From: gotjosh Date: Tue, 5 Apr 2022 19:36:42 +0100 Subject: [PATCH] Alerting: Accurately set value for prom-compatible APIs (#47216) * Alerting: Accurately set value for prom-compatible APIs Sets the value fields for the prometheus compatible API based on a combination of condition `refID` and the values extracted from the different frames. * Fix an extra test * Ensure a consitent ordering * Address review comments * address review comments --- pkg/services/ngalert/api/api_prometheus.go | 40 +++++- .../ngalert/api/api_prometheus_test.go | 115 ++++++++++++++++++ pkg/services/ngalert/eval/eval.go | 3 +- pkg/services/ngalert/eval/extract_md.go | 1 - pkg/services/ngalert/state/manager.go | 1 + pkg/services/ngalert/state/manager_test.go | 1 + pkg/services/ngalert/state/state.go | 20 +++ 7 files changed, 173 insertions(+), 8 deletions(-) diff --git a/pkg/services/ngalert/api/api_prometheus.go b/pkg/services/ngalert/api/api_prometheus.go index 3dfbc483316..37b84ac21bb 100644 --- a/pkg/services/ngalert/api/api_prometheus.go +++ b/pkg/services/ngalert/api/api_prometheus.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "sort" "strconv" "strings" "time" @@ -47,8 +48,9 @@ func (srv PrometheusSrv) RouteGetAlertStatuses(c *models.ReqContext) response.Re for _, alertState := range srv.manager.GetAll(c.OrgId) { startsAt := alertState.StartsAt valString := "" - if alertState.State == eval.Alerting { - valString = alertState.LastEvaluationString + + if alertState.State == eval.Alerting || alertState.State == eval.Pending { + valString = formatValues(alertState) } alertResponse.Data.Alerts = append(alertResponse.Data.Alerts, &apimodels.Alert{ @@ -63,6 +65,34 @@ func (srv PrometheusSrv) RouteGetAlertStatuses(c *models.ReqContext) response.Re return response.JSON(http.StatusOK, alertResponse) } +func formatValues(alertState *state.State) string { + var fv string + values := alertState.GetLastEvaluationValuesForCondition() + + switch len(values) { + case 0: + fv = alertState.LastEvaluationString + case 1: + for _, v := range values { + fv = strconv.FormatFloat(v, 'e', -1, 64) + break + } + + default: + vs := make([]string, 0, len(values)) + + for k, v := range values { + vs = append(vs, fmt.Sprintf("%s: %s", k, strconv.FormatFloat(v, 'e', -1, 64))) + } + + // Ensure we have a consistent natural ordering after formatting e.g. A0, A1, A10, A11, A3, etc. + sort.Strings(vs) + fv = strings.Join(vs, ", ") + } + + return fv +} + func getPanelIDFromRequest(r *http.Request) (int64, error) { if s := strings.TrimSpace(r.URL.Query().Get("panel_id")); s != "" { return strconv.ParseInt(s, 10, 64) @@ -178,8 +208,8 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Res for _, alertState := range srv.manager.GetStatesForRuleUID(c.OrgId, rule.UID) { activeAt := alertState.StartsAt valString := "" - if alertState.State == eval.Alerting { - valString = alertState.LastEvaluationString + if alertState.State == eval.Alerting || alertState.State == eval.Pending { + valString = formatValues(alertState) } alert := &apimodels.Alert{ @@ -187,7 +217,7 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *models.ReqContext) response.Res Annotations: alertState.Annotations, State: alertState.State.String(), ActiveAt: &activeAt, - Value: valString, // TODO: set this once it is added to the evaluation results + Value: valString, } if alertState.LastEvaluationTime.After(newRule.LastEvaluation) { diff --git a/pkg/services/ngalert/api/api_prometheus_test.go b/pkg/services/ngalert/api/api_prometheus_test.go index f9e7a548c87..74ec3dc9809 100644 --- a/pkg/services/ngalert/api/api_prometheus_test.go +++ b/pkg/services/ngalert/api/api_prometheus_test.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/ngalert/eval" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/state" "github.com/grafana/grafana/pkg/services/ngalert/store" @@ -20,6 +21,64 @@ import ( "github.com/stretchr/testify/require" ) +func Test_FormatValues(t *testing.T) { + val1 := 1.1 + val2 := 1.4 + + tc := []struct { + name string + alertState *state.State + expected string + }{ + { + name: "with no value, it renders the evaluation string", + alertState: &state.State{ + LastEvaluationString: "[ var='A' metric='vector(10) + time() % 50' labels={} value=1.1 ]", + Results: []state.Evaluation{ + {Condition: "A", Values: map[string]*float64{}}, + }, + }, + expected: "[ var='A' metric='vector(10) + time() % 50' labels={} value=1.1 ]", + }, + { + name: "with one value, it renders the single value", + alertState: &state.State{ + LastEvaluationString: "[ var='A' metric='vector(10) + time() % 50' labels={} value=1.1 ]", + Results: []state.Evaluation{ + {Condition: "A", Values: map[string]*float64{"A": &val1}}, + }, + }, + expected: "1.1e+00", + }, + { + name: "with two values, it renders the value based on their refID and position", + alertState: &state.State{ + LastEvaluationString: "[ var='B0' metric='vector(10) + time() % 50' labels={} value=1.1 ], [ var='B1' metric='vector(10) + time() % 50' labels={} value=1.4 ]", + Results: []state.Evaluation{ + {Condition: "B", Values: map[string]*float64{"B0": &val1, "B1": &val2}}, + }, + }, + expected: "B0: 1.1e+00, B1: 1.4e+00", + }, + { + name: "with a high number of values, it renders the value based on their refID and position using a natural order", + alertState: &state.State{ + LastEvaluationString: "[ var='B0' metric='vector(10) + time() % 50' labels={} value=1.1 ], [ var='B1' metric='vector(10) + time() % 50' labels={} value=1.4 ]", + Results: []state.Evaluation{ + {Condition: "B", Values: map[string]*float64{"B0": &val1, "B1": &val2, "B2": &val1, "B10": &val2, "B11": &val1}}, + }, + }, + expected: "B0: 1.1e+00, B10: 1.4e+00, B11: 1.1e+00, B1: 1.4e+00, B2: 1.1e+00", + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expected, formatValues(tt.alertState)) + }) + } +} + func TestRouteGetAlertStatuses(t *testing.T) { orgID := int64(1) @@ -83,6 +142,48 @@ func TestRouteGetAlertStatuses(t *testing.T) { }`, string(r.Body())) }) + t.Run("with two firing alerts", func(t *testing.T) { + _, fakeAIM, api := setupAPI(t) + fakeAIM.GenerateAlertInstances(1, util.GenerateShortUID(), 2, withAlertingState()) + req, err := http.NewRequest("GET", "/api/v1/alerts", nil) + require.NoError(t, err) + c := &models.ReqContext{Context: &web.Context{Req: req}, SignedInUser: &models.SignedInUser{OrgId: orgID}} + + r := api.RouteGetAlertStatuses(c) + require.Equal(t, http.StatusOK, r.Status()) + require.JSONEq(t, ` +{ + "status": "success", + "data": { + "alerts": [{ + "labels": { + "alertname": "test_title_0", + "instance_label": "test", + "label": "test" + }, + "annotations": { + "annotation": "test" + }, + "state": "Alerting", + "activeAt": "0001-01-01T00:00:00Z", + "value": "1.1e+00" + }, { + "labels": { + "alertname": "test_title_1", + "instance_label": "test", + "label": "test" + }, + "annotations": { + "annotation": "test" + }, + "state": "Alerting", + "activeAt": "0001-01-01T00:00:00Z", + "value": "1.1e+00" + }] + } +}`, string(r.Body())) + }) + t.Run("with the inclusion of internal labels", func(t *testing.T) { _, fakeAIM, api := setupAPI(t) fakeAIM.GenerateAlertInstances(orgID, util.GenerateShortUID(), 2) @@ -130,6 +231,20 @@ func TestRouteGetAlertStatuses(t *testing.T) { }) } +func withAlertingState() forEachState { + return func(s *state.State) *state.State { + s.State = eval.Alerting + value := float64(1.1) + s.Results = append(s.Results, state.Evaluation{ + EvaluationState: eval.Alerting, + EvaluationTime: timeNow(), + Values: map[string]*float64{"B": &value}, + Condition: "B", + }) + return s + } +} + func TestRouteGetRuleStatuses(t *testing.T) { timeNow = func() time.Time { return time.Date(2022, 3, 10, 14, 0, 0, 0, time.UTC) } orgID := int64(1) diff --git a/pkg/services/ngalert/eval/eval.go b/pkg/services/ngalert/eval/eval.go index 7ad329a0095..2667c661b0d 100644 --- a/pkg/services/ngalert/eval/eval.go +++ b/pkg/services/ngalert/eval/eval.go @@ -12,6 +12,7 @@ import ( "time" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/expr/classic" "github.com/grafana/grafana/pkg/infra/log" m "github.com/grafana/grafana/pkg/models" @@ -22,8 +23,6 @@ import ( "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana-plugin-sdk-go/data" - - "github.com/grafana/grafana/pkg/expr" ) //go:generate mockery --name Evaluator --structname FakeEvaluator --inpackage --filename evaluator_mock.go --with-expecter diff --git a/pkg/services/ngalert/eval/extract_md.go b/pkg/services/ngalert/eval/extract_md.go index 8ba7bfb490e..e08a7b22bbc 100644 --- a/pkg/services/ngalert/eval/extract_md.go +++ b/pkg/services/ngalert/eval/extract_md.go @@ -20,7 +20,6 @@ func extractEvalString(frame *data.Frame) (s string) { if evalMatches, ok := frame.Meta.Custom.([]classic.EvalMatch); ok { sb := strings.Builder{} - // TODO: Should we simplify when we only have one match and use the name notation of $labels.A? for i, m := range evalMatches { sb.WriteString("[ ") sb.WriteString(fmt.Sprintf("var='%s%v' ", frame.RefID, i)) diff --git a/pkg/services/ngalert/state/manager.go b/pkg/services/ngalert/state/manager.go index c4cbc3b4ba0..a40f8fc0c82 100644 --- a/pkg/services/ngalert/state/manager.go +++ b/pkg/services/ngalert/state/manager.go @@ -171,6 +171,7 @@ func (st *Manager) setNextState(ctx context.Context, alertRule *ngModels.AlertRu EvaluationTime: result.EvaluatedAt, EvaluationState: result.State, Values: NewEvaluationValues(result.Values), + Condition: alertRule.Condition, }) currentState.LastEvaluationString = result.EvaluationString currentState.TrimResults(alertRule) diff --git a/pkg/services/ngalert/state/manager_test.go b/pkg/services/ngalert/state/manager_test.go index b6e1b1d78c4..df34f4b4f3d 100644 --- a/pkg/services/ngalert/state/manager_test.go +++ b/pkg/services/ngalert/state/manager_test.go @@ -1719,6 +1719,7 @@ func TestStaleResultsHandler(t *testing.T) { EvaluationTime: evaluationTime.Add(3 * time.Minute), EvaluationState: eval.Normal, Values: make(map[string]*float64), + Condition: "A", }, }, LastEvaluationTime: evaluationTime.Add(3 * time.Minute), diff --git a/pkg/services/ngalert/state/state.go b/pkg/services/ngalert/state/state.go index 0ed75b50de4..63f3b9d5930 100644 --- a/pkg/services/ngalert/state/state.go +++ b/pkg/services/ngalert/state/state.go @@ -2,6 +2,7 @@ package state import ( "errors" + "strings" "time" "github.com/grafana/grafana-plugin-sdk-go/data" @@ -36,6 +37,8 @@ type Evaluation struct { // Classic conditions can have different values for the same RefID as they can include multiple conditions. // For these, we use the index of the condition in addition RefID as the key e.g. "A0, A1, A2, etc.". Values map[string]*float64 + // Condition is the refID specified as the condition in the alerting rule at the time of the evaluation. + Condition string } // NewEvaluationValues returns the labels and values for each RefID in the capture. @@ -203,3 +206,20 @@ func (a *State) GetLabels(opts ...ngModels.LabelOption) map[string]string { return labels } + +func (a *State) GetLastEvaluationValuesForCondition() map[string]float64 { + if len(a.Results) <= 0 { + return nil + } + + lastResult := a.Results[len(a.Results)-1] + r := make(map[string]float64, len(lastResult.Values)) + + for refID, value := range lastResult.Values { + if strings.Contains(refID, lastResult.Condition) { + r[refID] = *value + } + } + + return r +}