From 0a01391ebeaabfa7c60ae29dc9b49d3d54bb6e86 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Mon, 20 Feb 2023 09:24:11 +0000 Subject: [PATCH] Alerting: Small readability improvements to template.go (#63422) * Alerting: Small readability improvements to template.go * Fix lint --- pkg/services/ngalert/state/cache.go | 2 +- .../ngalert/state/template/template.go | 80 ++++++++++--------- .../ngalert/state/template/template_test.go | 2 +- 3 files changed, 43 insertions(+), 41 deletions(-) diff --git a/pkg/services/ngalert/state/cache.go b/pkg/services/ngalert/state/cache.go index 12a62cb89e9..8df918e87c3 100644 --- a/pkg/services/ngalert/state/cache.go +++ b/pkg/services/ngalert/state/cache.go @@ -142,7 +142,7 @@ func (rs *ruleStates) expandRuleLabelsAndAnnotations(ctx context.Context, log lo expand := func(original map[string]string) map[string]string { expanded := make(map[string]string, len(original)) for k, v := range original { - ev, err := template.Expand(ctx, alertRule.Title, v, templateLabels, alertInstance, externalURL) + ev, err := template.Expand(ctx, alertRule.Title, v, template.NewData(templateLabels, alertInstance), externalURL, alertInstance.EvaluatedAt) expanded[k] = ev if err != nil { log.Error("Error in expanding template", "name", k, "value", v, "error", err) diff --git a/pkg/services/ngalert/state/template/template.go b/pkg/services/ngalert/state/template/template.go index c26455fa862..c2f3966e343 100644 --- a/pkg/services/ngalert/state/template/template.go +++ b/pkg/services/ngalert/state/template/template.go @@ -21,13 +21,11 @@ type Labels map[string]string // String returns the labels as k=v, comma separated, in increasing order. func (l Labels) String() string { - // sort the names of the labels in increasing order sorted := make([]string, 0, len(l)) for k := range l { sorted = append(sorted, k) } sort.Strings(sorted) - // create the string from the sorted labels b := strings.Builder{} for i, k := range sorted { b.WriteString(k) @@ -51,59 +49,63 @@ func (v Value) String() string { return strconv.FormatFloat(v.Value, 'f', -1, 64) } -func NewValues(values map[string]eval.NumberValueCapture) map[string]Value { - m := make(map[string]Value) - for k, v := range values { +func NewValues(caps map[string]eval.NumberValueCapture) map[string]Value { + values := make(map[string]Value) + for refID, cap := range caps { var f float64 - if v.Value != nil { - f = *v.Value + // A RefID might be missing a value if there was no data or an error. + // If that is the case, use "not a number". We don't use 0, or -1, as + // either of those are possible values for a RefID. + if cap.Value != nil { + f = *cap.Value } else { f = math.NaN() } - m[k] = Value{ - Labels: Labels(v.Labels), + values[refID] = Value{ + Labels: Labels(cap.Labels), Value: f, } } - return m + return values } -func Expand( - ctx context.Context, - name, tmpl string, - labels map[string]string, - res eval.Result, - externalURL *url.URL) (string, error) { - name = "__alert_" + name - tmpl = "{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}" + tmpl - data := struct { - Labels map[string]string - Values map[string]Value - Value string - }{ +type Data struct { + Labels map[string]string + Values map[string]Value + Value string +} + +func NewData(labels map[string]string, res eval.Result) Data { + return Data{ Labels: labels, Values: NewValues(res.Values), Value: res.EvaluationString, } +} - expander := template.NewTemplateExpander( - ctx, // This context is only used with the `query()` function - which we don't support yet. - tmpl, - name, - data, - model.Time(timestamp.FromTime(res.EvaluatedAt)), - func(context.Context, string, time.Time) (promql.Vector, error) { - return nil, nil - }, - externalURL, - []string{"missingkey=invalid"}, - ) +func Expand(ctx context.Context, name, tmpl string, data Data, externalURL *url.URL, evaluatedAt time.Time) (string, error) { + // add __alert_ to avoid possible conflicts with other templates + name = "__alert_" + name + // add variables for the labels and values to the beginning of the template + tmpl = "{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}" + tmpl + // ctx and queryFunc are no-ops as `query()` is not supported in Grafana + queryFunc := func(context.Context, string, time.Time) (promql.Vector, error) { + return nil, nil + } + tm := model.Time(timestamp.FromTime(evaluatedAt)) + // Use missingkey=invalid so missing data shows instead of the type's default value + options := []string{"missingkey=invalid"} + + expander := template.NewTemplateExpander(ctx, tmpl, name, data, tm, queryFunc, externalURL, options) expander.Funcs(defaultFuncs) result, err := expander.Expand() - // Replace missing key value to one that does not look like an HTML tag. This can cause problems downstream in some notifiers. - // For example, Telegram in HTML mode rejects requests with unsupported tags. - result = strings.ReplaceAll(result, "", "[no value]") + if err != nil { + return "", err + } - return result, err + // We need to replace with [no value] as some integrations think is invalid HTML. For example, + // Telegram in HTML mode rejects messages with unsupported tags. + result = strings.ReplaceAll(result, "", "[no value]") + return result, nil } diff --git a/pkg/services/ngalert/state/template/template_test.go b/pkg/services/ngalert/state/template/template_test.go index f71d8e323da..db361349842 100644 --- a/pkg/services/ngalert/state/template/template_test.go +++ b/pkg/services/ngalert/state/template/template_test.go @@ -431,7 +431,7 @@ func TestExpandTemplate(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - v, err := Expand(context.Background(), "test", c.text, c.labels, c.alertInstance, externalURL) + v, err := Expand(context.Background(), "test", c.text, NewData(c.labels, c.alertInstance), externalURL, c.alertInstance.EvaluatedAt) if c.expectedError != nil { require.NotNil(t, err) require.EqualError(t, c.expectedError, err.Error())