From 03f3fbec0d35968beb694af4ebe3fc5591c0455a Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Tue, 31 Jan 2023 15:27:51 -0600 Subject: [PATCH] Alerting: Fix handling of special floating-point cases when writing observed values to annotations (#61074) * Fix json serialization of state values * Simplify two of the tests * Fix linter complaint * Don't return error if we fail to look up dashboard, just log it and move on * Address linter complaint --- .../ngalert/state/historian/annotation.go | 26 +++++-- .../state/historian/annotation_test.go | 67 +++++++++++++++++++ 2 files changed, 89 insertions(+), 4 deletions(-) diff --git a/pkg/services/ngalert/state/historian/annotation.go b/pkg/services/ngalert/state/historian/annotation.go index bca76460e78..7d28512ea23 100644 --- a/pkg/services/ngalert/state/historian/annotation.go +++ b/pkg/services/ngalert/state/historian/annotation.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "sort" "strings" "time" @@ -45,7 +46,7 @@ func NewAnnotationBackend(annotations annotations.Repository, dashboards dashboa func (h *AnnotationBackend) RecordStatesAsync(ctx context.Context, rule history_model.RuleMeta, states []state.StateTransition) <-chan error { logger := h.log.FromContext(ctx) // Build annotations before starting goroutine, to make sure all data is copied and won't mutate underneath us. - annotations := h.buildAnnotations(rule, states, logger) + annotations := buildAnnotations(rule, states, logger) panel := parsePanelKey(rule, logger) errCh := make(chan error, 1) go func() { @@ -138,7 +139,7 @@ func (h *AnnotationBackend) QueryStates(ctx context.Context, query ngmodels.Hist return frame, nil } -func (h *AnnotationBackend) buildAnnotations(rule history_model.RuleMeta, states []state.StateTransition, logger log.Logger) []annotations.Item { +func buildAnnotations(rule history_model.RuleMeta, states []state.StateTransition, logger log.Logger) []annotations.Item { items := make([]annotations.Item, 0, len(states)) for _, state := range states { if !shouldRecord(state) { @@ -168,7 +169,7 @@ func (h *AnnotationBackend) recordAnnotationsSync(ctx context.Context, panel *pa dashID, err := h.dashboards.getID(ctx, panel.orgID, panel.dashUID) if err != nil { logger.Error("Error getting dashboard for alert annotation", "dashboardUID", panel.dashUID, "error", err) - return fmt.Errorf("error getting dashboard for alert annotation: %w", err) + dashID = 0 } for i := range annotations { @@ -212,10 +213,27 @@ func buildAnnotationTextAndData(rule history_model.RuleMeta, currentState *state for _, k := range keys { values = append(values, fmt.Sprintf("%s=%f", k, currentState.Values[k])) } - jsonData.Set("values", simplejson.NewFromAny(currentState.Values)) + jsonData.Set("values", jsonifyValues(currentState.Values)) value = strings.Join(values, ", ") } labels := removePrivateLabels(currentState.Labels) return fmt.Sprintf("%s {%s} - %s", rule.Title, labels.String(), value), jsonData } + +func jsonifyValues(vs map[string]float64) *simplejson.Json { + if vs == nil { + return nil + } + + j := simplejson.New() + for k, v := range vs { + switch { + case math.IsInf(v, 0), math.IsNaN(v): + j.Set(k, fmt.Sprintf("%f", v)) + default: + j.Set(k, v) + } + } + return j +} diff --git a/pkg/services/ngalert/state/historian/annotation_test.go b/pkg/services/ngalert/state/historian/annotation_test.go index e94869c9d5a..6c05aa4b997 100644 --- a/pkg/services/ngalert/state/historian/annotation_test.go +++ b/pkg/services/ngalert/state/historian/annotation_test.go @@ -2,6 +2,8 @@ package historian import ( "context" + "encoding/json" + "math" "testing" "time" @@ -12,7 +14,10 @@ import ( "github.com/grafana/grafana/pkg/services/annotations" "github.com/grafana/grafana/pkg/services/annotations/annotationstest" "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/state" + history_model "github.com/grafana/grafana/pkg/services/ngalert/state/historian/model" "github.com/grafana/grafana/pkg/services/ngalert/tests/fakes" ) @@ -64,8 +69,70 @@ func withOrgID(orgId int64) func(rule *models.AlertRule) { } } +func TestBuildAnnotations(t *testing.T) { + t.Run("data wraps nil values when values are nil", func(t *testing.T) { + logger := log.NewNopLogger() + rule := history_model.RuleMeta{} + states := []state.StateTransition{makeStateTransition()} + states[0].State.Values = nil + + items := buildAnnotations(rule, states, logger) + + require.Len(t, items, 1) + j := assertValidJSON(t, items[0].Data) + require.JSONEq(t, `{"values": null}`, j) + }) + + t.Run("data approximately contains expected values", func(t *testing.T) { + logger := log.NewNopLogger() + rule := history_model.RuleMeta{} + states := []state.StateTransition{makeStateTransition()} + states[0].State.Values = map[string]float64{"a": 1.0, "b": 2.0} + + items := buildAnnotations(rule, states, logger) + + require.Len(t, items, 1) + assertValidJSON(t, items[0].Data) + // Since we're comparing floats, avoid require.JSONEq to avoid intermittency caused by floating point rounding. + vs := items[0].Data.MustMap()["values"] + require.NotNil(t, vs) + vals := vs.(*simplejson.Json).MustMap() + require.InDelta(t, 1.0, vals["a"], 0.1) + require.InDelta(t, 2.0, vals["b"], 0.1) + }) + + t.Run("data handles special float values", func(t *testing.T) { + logger := log.NewNopLogger() + rule := history_model.RuleMeta{} + states := []state.StateTransition{makeStateTransition()} + states[0].State.Values = map[string]float64{"nan": math.NaN(), "inf": math.Inf(1), "ninf": math.Inf(-1)} + + items := buildAnnotations(rule, states, logger) + + require.Len(t, items, 1) + j := assertValidJSON(t, items[0].Data) + require.JSONEq(t, `{"values": {"nan": "NaN", "inf": "+Inf", "ninf": "-Inf"}}`, j) + }) +} + +func makeStateTransition() state.StateTransition { + return state.StateTransition{ + State: &state.State{ + State: eval.Alerting, + }, + PreviousState: eval.Normal, + } +} + func withUID(uid string) func(rule *models.AlertRule) { return func(rule *models.AlertRule) { rule.UID = uid } } + +func assertValidJSON(t *testing.T, j *simplejson.Json) string { + require.NotNil(t, j) + ser, err := json.Marshal(j) + require.NoError(t, err) + return string(ser) +}