Skip to content

Commit

Permalink
Missing title and severity columns in results are user errors
Browse files Browse the repository at this point in the history
If a query fails to generate expected title or severity columns, adx-mon
should consider this a user error rather than an issue with the running
adx-mon instance. By tagging these problems with an appropriate error
type, adx-mon will notify the downstream creator of alerts that their
query is missing important context in its results.
  • Loading branch information
Mike Keesey committed Jul 6, 2023
1 parent 099c7b9 commit b8e2350
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 7 deletions.
4 changes: 1 addition & 3 deletions alerter/engine/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,7 @@ func (e *Executor) HandlerFn(ctx context.Context, endpoint string, qc *QueryCont
case "severity":
v, err := e.asInt64(value)
if err != nil {
metrics.QueryHealth.WithLabelValues(qc.Rule.Namespace, qc.Rule.Name).Set(0)
return err
return &NotificationValidationError{err.Error()}
}
res.Severity = v
case "recipient":
Expand All @@ -133,7 +132,6 @@ func (e *Executor) HandlerFn(ctx context.Context, endpoint string, qc *QueryCont
}

if err := res.Validate(); err != nil {
metrics.QueryHealth.WithLabelValues(qc.Rule.Namespace, qc.Rule.Name).Set(0)
return err
}

Expand Down
15 changes: 14 additions & 1 deletion alerter/engine/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ func TestExecutor_Handler_MissingTitle(t *testing.T) {
require.NoError(t, iter.Mock(rows))

row, _, _ := iter.NextRowOrError()
require.ErrorContains(t, e.HandlerFn(context.Background(), "http://endpoint", qc, row), "title must be between 1 and 512 chars")
err = e.HandlerFn(context.Background(), "http://endpoint", qc, row)
require.ErrorContains(t, err, "title must be between 1 and 512 chars")
require.True(t, isUserError(err))
}

func TestExecutor_Handler_Severity(t *testing.T) {
Expand All @@ -61,6 +63,16 @@ func TestExecutor_Handler_Severity(t *testing.T) {
rows: value.Values{value.String{Value: "Title", Valid: true}},
err: "severity must be specified",
},
{
desc: "severity not convertable to a number",
columns: table.Columns{
{Name: "Title", Type: types.String},
{Name: "Severity", Type: types.String}},
rows: value.Values{
value.String{Value: "Title", Valid: true},
value.String{Value: "not a number", Valid: false}},
err: "failed to convert severity to int",
},
{
desc: "severity as long",
columns: table.Columns{
Expand Down Expand Up @@ -139,6 +151,7 @@ func TestExecutor_Handler_Severity(t *testing.T) {
require.Equal(t, tt.severity, client.alert.Severity)
} else {
require.ErrorContains(t, err, tt.err)
require.True(t, isUserError(err))
}
})
}
Expand Down
13 changes: 10 additions & 3 deletions alerter/engine/types.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package engine

import (
"fmt"
"math"
)

Expand Down Expand Up @@ -31,10 +30,18 @@ type Notification struct {

func (i Notification) Validate() error {
if len(i.Title) == 0 || len(i.Title) > 512 {
return fmt.Errorf("title must be between 1 and 512 chars")
return &NotificationValidationError{"title must be between 1 and 512 chars"}
}
if i.Severity == math.MinInt64 {
return fmt.Errorf("severity must be specified")
return &NotificationValidationError{"severity must be specified"}
}
return nil
}

type NotificationValidationError struct {
Msg string
}

func (e *NotificationValidationError) Error() string {
return e.Msg
}
9 changes: 9 additions & 0 deletions alerter/engine/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,19 @@ func isUserError(err error) bool {
return false
}

// User specified a database in their CRD that adx-mon does not have configured.
var unknownDB *UnknownDBError
if errors.As(err, &unknownDB) {
return true
}

// User's query results are missing a required column, or they are the wrong type.
var validationErr *NotificationValidationError
if errors.As(err, &validationErr) {
return true
}

// Look to see if a kusto query error is specific to how the query was defined and not due to problems with adx-mon itself.
var kerr *kerrors.HttpError
if errors.As(err, &kerr) {
if kerr.Kind == kerrors.KClientArgs {
Expand Down
37 changes: 37 additions & 0 deletions alerter/engine/worker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,43 @@ func TestWorker_UnknownDB(t *testing.T) {
require.Equal(t, QueryHealthHealthy, gaugeValue)
}

func TestWorker_MissingColumnsFromResults(t *testing.T) {
kcli := &fakeKustoClient{
log: logger.Default,
queryErr: &NotificationValidationError{"invalid result"},
}

var createCalled bool
alertCli := &fakeAlerter{
createFn: func(ctx context.Context, endpoint string, alert alert.Alert) error {
createCalled = true
return nil
},
}

rule := &rules.Rule{
Namespace: "namespace",
Name: "name",
}
w := &worker{
rule: rule,
Region: "eastus",
kustoClient: kcli,
AlertAddr: "",
AlertCli: alertCli,
HandlerFn: nil,
}

// default healthy
metrics.QueryHealth.WithLabelValues(rule.Namespace, rule.Name).Set(QueryHealthHealthy)

w.ExecuteQuery(context.Background())
require.True(t, createCalled, "Create alert should be called")
gaugeValue := getGaugeValue(t, metrics.QueryHealth.WithLabelValues(rule.Namespace, rule.Name))
// user caused error
require.Equal(t, QueryHealthHealthy, gaugeValue)
}

func getGaugeValue(t *testing.T, metric prometheus.Metric) float64 {
t.Helper()

Expand Down

0 comments on commit b8e2350

Please sign in to comment.