diff --git a/pkg/services/ngalert/state/cache.go b/pkg/services/ngalert/state/cache.go index 192e40bfa32..34078dd721d 100644 --- a/pkg/services/ngalert/state/cache.go +++ b/pkg/services/ngalert/state/cache.go @@ -6,8 +6,10 @@ import ( "net/url" "strings" "sync" + "time" "github.com/grafana/grafana-plugin-sdk-go/data" + "github.com/hashicorp/go-multierror" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ngalert/eval" @@ -49,7 +51,14 @@ func (c *cache) getOrCreate(ctx context.Context, log log.Logger, alertRule *ngMo } func (rs *ruleStates) getOrCreate(ctx context.Context, log log.Logger, alertRule *ngModels.AlertRule, result eval.Result, extraLabels data.Labels, externalURL *url.URL) *State { - ruleLabels, annotations := rs.expandRuleLabelsAndAnnotations(ctx, log, alertRule, result, extraLabels, externalURL) + // Merge both the extra labels and the labels from the evaluation into a common set + // of labels that can be expanded in custom labels and annotations. + templateData := template.NewData(mergeLabels(extraLabels, result.Instance), result) + + // For now, do nothing with these errors as they are already logged in expand. + // In the future, we want to show these errors to the user somehow. + labels, _ := expand(ctx, log, alertRule.Title, alertRule.Labels, templateData, externalURL, result.EvaluatedAt) + annotations, _ := expand(ctx, log, alertRule.Title, alertRule.Annotations, templateData, externalURL, result.EvaluatedAt) values := make(map[string]float64) for refID, v := range result.Values { @@ -60,12 +69,12 @@ func (rs *ruleStates) getOrCreate(ctx context.Context, log log.Logger, alertRule } } - lbs := make(data.Labels, len(extraLabels)+len(ruleLabels)+len(result.Instance)) + lbs := make(data.Labels, len(extraLabels)+len(labels)+len(result.Instance)) dupes := make(data.Labels) for key, val := range extraLabels { lbs[key] = val } - for key, val := range ruleLabels { + for key, val := range labels { ruleVal, ok := lbs[key] // if duplicate labels exist, reserved label will take precedence if ok { @@ -135,25 +144,27 @@ func (rs *ruleStates) getOrCreate(ctx context.Context, log log.Logger, alertRule return newState } -func (rs *ruleStates) expandRuleLabelsAndAnnotations(ctx context.Context, log log.Logger, alertRule *ngModels.AlertRule, alertInstance eval.Result, extraLabels data.Labels, externalURL *url.URL) (data.Labels, data.Labels) { - // use labels from the result and extra labels to expand the labels and annotations declared by the rule - templateLabels := mergeLabels(extraLabels, alertInstance.Instance) - - 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, 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) - // Store the original template on error. - expanded[k] = v - } +// expand returns the expanded templates of all annotations or labels for the template data. +// If a template cannot be expanded due to an error in the template the original template is +// maintained and an error is added to the multierror. All errors in the multierror are +// template.ExpandError errors. +func expand(ctx context.Context, log log.Logger, name string, original map[string]string, data template.Data, externalURL *url.URL, evaluatedAt time.Time) (map[string]string, error) { + var ( + errs *multierror.Error + expanded = make(map[string]string, len(original)) + ) + for k, v := range original { + result, err := template.Expand(ctx, name, v, data, externalURL, evaluatedAt) + if err != nil { + log.Error("Error in expanding template", "error", err) + errs = multierror.Append(errs, err) + // keep the original template on error + expanded[k] = v + } else { + expanded[k] = result } - - return expanded } - return expand(alertRule.Labels), expand(alertRule.Annotations) + return expanded, errs.ErrorOrNil() } func (rs *ruleStates) deleteStates(predicate func(s *State) bool) []*State { diff --git a/pkg/services/ngalert/state/cache_test.go b/pkg/services/ngalert/state/cache_test.go index 5f5a2de3428..5a1442de4b8 100644 --- a/pkg/services/ngalert/state/cache_test.go +++ b/pkg/services/ngalert/state/cache_test.go @@ -2,20 +2,112 @@ package state import ( "context" + "errors" "fmt" "net/url" "testing" + "time" "github.com/grafana/grafana-plugin-sdk-go/data" + "github.com/hashicorp/go-multierror" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/state/template" "github.com/grafana/grafana/pkg/util" ) +func Test_expand(t *testing.T) { + ctx := context.Background() + logger := log.NewNopLogger() + + // This test asserts that multierror returns a nil error if there are no errors. + // If the expand function forgets to use ErrorOrNil() then the error returned will + // be non-nil even if no errors have been added to the multierror. + t.Run("err is nil if there are no errors", func(t *testing.T) { + result, err := expand(ctx, logger, "test", map[string]string{}, template.Data{}, nil, time.Now()) + require.NoError(t, err) + require.Len(t, result, 0) + }) + + t.Run("original is expanded with template data", func(t *testing.T) { + original := map[string]string{"Summary": `Instance {{ $labels.instance }} has been down for more than 5 minutes`} + expected := map[string]string{"Summary": "Instance host1 has been down for more than 5 minutes"} + data := template.Data{Labels: map[string]string{"instance": "host1"}} + results, err := expand(ctx, logger, "test", original, data, nil, time.Now()) + require.NoError(t, err) + require.Equal(t, expected, results) + }) + + t.Run("original is returned with an error", func(t *testing.T) { + original := map[string]string{ + "Summary": `Instance {{ $labels. }} has been down for more than 5 minutes`, + } + data := template.Data{Labels: map[string]string{"instance": "host1"}} + results, err := expand(ctx, logger, "test", original, data, nil, time.Now()) + require.NotNil(t, err) + require.Equal(t, original, results) + + // err should be an ExpandError that contains the template for the Summary and an error + var expandErr template.ExpandError + require.True(t, errors.As(err, &expandErr)) + require.EqualError(t, expandErr, "failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}Instance {{ $labels. }} has been down for more than 5 minutes': error parsing template __alert_test: template: __alert_test:1: unexpected <.> in operand") + }) + + t.Run("originals are returned with two errors", func(t *testing.T) { + original := map[string]string{ + "Summary": `Instance {{ $labels. }} has been down for more than 5 minutes`, + "Description": "The instance has been down for {{ $value minutes, please check the instance is online", + } + data := template.Data{Labels: map[string]string{"instance": "host1"}} + results, err := expand(ctx, logger, "test", original, data, nil, time.Now()) + require.NotNil(t, err) + require.Equal(t, original, results) + + // TODO: Please update this test in issue https://github.com/grafana/grafana/issues/63686 + var multierr *multierror.Error + require.True(t, errors.As(err, &multierr)) + require.Equal(t, multierr.Len(), 2) + + // assert each error matches the expected error + var expandErr1 template.ExpandError + require.True(t, errors.As(multierr.Errors[0], &expandErr1)) + require.EqualError(t, expandErr1, "failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}Instance {{ $labels. }} has been down for more than 5 minutes': error parsing template __alert_test: template: __alert_test:1: unexpected <.> in operand") + + var expandErr2 template.ExpandError + require.True(t, errors.As(multierr.Errors[1], &expandErr2)) + require.EqualError(t, expandErr2, "failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}The instance has been down for {{ $value minutes, please check the instance is online': error parsing template __alert_test: template: __alert_test:1: function \"minutes\" not defined") + }) + + t.Run("expanded and original is returned when there is one error", func(t *testing.T) { + original := map[string]string{ + "Summary": `Instance {{ $labels.instance }} has been down for more than 5 minutes`, + "Description": "The instance has been down for {{ $value minutes, please check the instance is online", + } + expected := map[string]string{ + "Summary": "Instance host1 has been down for more than 5 minutes", + "Description": "The instance has been down for {{ $value minutes, please check the instance is online", + } + data := template.Data{Labels: map[string]string{"instance": "host1"}} + results, err := expand(ctx, logger, "test", original, data, nil, time.Now()) + require.NotNil(t, err) + require.Equal(t, expected, results) + + // TODO: Please update this test in issue https://github.com/grafana/grafana/issues/63686 + var multierr *multierror.Error + require.True(t, errors.As(err, &multierr)) + require.Equal(t, multierr.Len(), 1) + + // assert each error matches the expected error + var expandErr template.ExpandError + require.True(t, errors.As(err, &expandErr)) + require.EqualError(t, expandErr, "failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}The instance has been down for {{ $value minutes, please check the instance is online': error parsing template __alert_test: template: __alert_test:1: function \"minutes\" not defined") + }) +} + func Test_getOrCreate(t *testing.T) { url := &url.URL{ Scheme: "http", diff --git a/pkg/services/ngalert/state/template/template.go b/pkg/services/ngalert/state/template/template.go index c9b7a0e14ac..86e2dad21d8 100644 --- a/pkg/services/ngalert/state/template/template.go +++ b/pkg/services/ngalert/state/template/template.go @@ -2,6 +2,7 @@ package template import ( "context" + "fmt" "math" "net/url" "sort" @@ -83,6 +84,17 @@ func NewData(labels map[string]string, res eval.Result) Data { } } +// ExpandError is an error containing the template and the error that occurred +// while expanding it. +type ExpandError struct { + Tmpl string + Err error +} + +func (e ExpandError) Error() string { + return fmt.Sprintf("failed to expand template '%s': %s", e.Tmpl, e.Err) +} + 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 @@ -101,7 +113,7 @@ func Expand(ctx context.Context, name, tmpl string, data Data, externalURL *url. result, err := expander.Expand() if err != nil { - return "", err + return "", ExpandError{Tmpl: tmpl, Err: err} } // We need to replace with [no value] as some integrations think is invalid HTML. For example, diff --git a/pkg/services/ngalert/state/template/template_test.go b/pkg/services/ngalert/state/template/template_test.go index d0ae80abc32..11982b45505 100644 --- a/pkg/services/ngalert/state/template/template_test.go +++ b/pkg/services/ngalert/state/template/template_test.go @@ -66,6 +66,11 @@ func TestValueString(t *testing.T) { } } +func TestExpandError(t *testing.T) { + err := ExpandError{Tmpl: "{{", Err: errors.New("unexpected {{")} + assert.Equal(t, "failed to expand template '{{': unexpected {{", err.Error()) +} + func TestExpandTemplate(t *testing.T) { pathPrefix := "/path/prefix" externalURL, err := url.Parse("http://localhost" + pathPrefix) @@ -167,7 +172,7 @@ func TestExpandTemplate(t *testing.T) { alertInstance: eval.Result{ EvaluationString: "invalid", }, - expectedError: errors.New(`error executing template __alert_test: template: __alert_test:1:79: executing "__alert_test" at : error calling humanize: strconv.ParseFloat: parsing "invalid": invalid syntax`), + expectedError: errors.New(`failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}{{ humanize $value }}': error executing template __alert_test: template: __alert_test:1:79: executing "__alert_test" at : error calling humanize: strconv.ParseFloat: parsing "invalid": invalid syntax`), }, { name: "humanize1024 float64", text: "{{ range $key, $val := $values }}{{ humanize1024 .Value }}:{{ end }}", @@ -202,7 +207,7 @@ func TestExpandTemplate(t *testing.T) { alertInstance: eval.Result{ EvaluationString: "invalid", }, - expectedError: errors.New(`error executing template __alert_test: template: __alert_test:1:79: executing "__alert_test" at : error calling humanize1024: strconv.ParseFloat: parsing "invalid": invalid syntax`), + expectedError: errors.New(`failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}{{ humanize1024 $value }}': error executing template __alert_test: template: __alert_test:1:79: executing "__alert_test" at : error calling humanize1024: strconv.ParseFloat: parsing "invalid": invalid syntax`), }, { name: "humanizeDuration - seconds - float64", text: "{{ range $key, $val := $values }}{{ humanizeDuration .Value }}:{{ end }}", @@ -321,7 +326,7 @@ func TestExpandTemplate(t *testing.T) { alertInstance: eval.Result{ EvaluationString: "invalid", }, - expectedError: errors.New(`error executing template __alert_test: template: __alert_test:1:79: executing "__alert_test" at : error calling humanizeDuration: strconv.ParseFloat: parsing "invalid": invalid syntax`), + expectedError: errors.New(`failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}{{ humanizeDuration $value }}': error executing template __alert_test: template: __alert_test:1:79: executing "__alert_test" at : error calling humanizeDuration: strconv.ParseFloat: parsing "invalid": invalid syntax`), }, { name: "humanizePercentage - float64", text: "{{ -0.22222 | humanizePercentage }}:{{ 0.0 | humanizePercentage }}:{{ 0.1234567 | humanizePercentage }}:{{ 1.23456 | humanizePercentage }}", @@ -333,7 +338,7 @@ func TestExpandTemplate(t *testing.T) { }, { name: "humanizePercentage - string with error", text: `{{ "invalid" | humanizePercentage }}`, - expectedError: errors.New(`error executing template __alert_test: template: __alert_test:1:91: executing "__alert_test" at : error calling humanizePercentage: strconv.ParseFloat: parsing "invalid": invalid syntax`), + expectedError: errors.New(`failed to expand template '{{- $labels := .Labels -}}{{- $values := .Values -}}{{- $value := .Value -}}{{ "invalid" | humanizePercentage }}': error executing template __alert_test: template: __alert_test:1:91: executing "__alert_test" at : error calling humanizePercentage: strconv.ParseFloat: parsing "invalid": invalid syntax`), }, { name: "humanizeTimestamp - float64", text: "{{ 1435065584.128 | humanizeTimestamp }}",