Skip to content

Commit

Permalink
Fix TrendSink median calculation as part of Thresholds.Run
Browse files Browse the repository at this point in the history
This commit addresses the issues outlined in grafana#2755.

We assume that because the trend sink median computation can turn out
to be quite expensive (it involves sorting the array of values), it was
so far delegated to the last moment: thresholds evaluation and summary
generation. This detail appeared to be missed in the most recent
thresholds refactoring, and the `Calc` method was never used by the
`Thresholds.Run` method.

This commit ensures that in the same fashion as with Trend Sinks
percentiles, the thresholds evaluation also computes the sink's median
value when necessary.

This commit adds dedicated tests illustrating the behavior, and
demonstrating that it solves said issue.
  • Loading branch information
oleiade committed Nov 10, 2022
1 parent 25ca1fe commit f63c848
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 11 deletions.
2 changes: 1 addition & 1 deletion metrics/thresholds.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func (ts *Thresholds) Run(sink Sink, duration time.Duration) (bool, error) {
ts.sinked["min"] = sinkImpl.Min
ts.sinked["max"] = sinkImpl.Max
ts.sinked["avg"] = sinkImpl.Avg
ts.sinked["med"] = sinkImpl.Med
ts.sinked["med"] = sinkImpl.P(0.5)

// Parse the percentile thresholds and insert them in
// the sinks mapping.
Expand Down
74 changes: 64 additions & 10 deletions metrics/thresholds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -638,12 +638,13 @@ func TestThresholdsRunAll(t *testing.T) {
}
}

func TestThresholds_Run(t *testing.T) {
func TestThresholdsRun(t *testing.T) {
t.Parallel()

type args struct {
sink Sink
duration time.Duration
sink Sink
thresholdExpressions []string
duration time.Duration
}
tests := []struct {
name string
Expand All @@ -652,30 +653,83 @@ func TestThresholds_Run(t *testing.T) {
wantErr bool
}{
{
name: "Running thresholds of existing sink",
args: args{DummySink{"p(95)": 1234.5}, 0},
name: "Running thresholds of existing sink",
args: args{
sink: DummySink{"p(95)": 1234.5},
thresholdExpressions: []string{"p(95)<2000"},
duration: 0,
},
want: true,
wantErr: false,
},
{
name: "Running thresholds of existing sink but failing threshold",
args: args{
sink: DummySink{"p(95)": 3000},
thresholdExpressions: []string{"p(95)<2000"},
duration: 0,
},
want: false,
wantErr: false,
},
{
name: "Running threshold on non existing sink does not fail",
args: args{
sink: DummySink{"dummy": 0},
thresholdExpressions: []string{"p(95)<2000"},
duration: 0,
},
want: true,
wantErr: false,
},
{
name: "Running threshold on trend sink with no values and passing med statement succeeds",
args: args{
sink: &TrendSink{Values: []float64{}, Med: 0},
thresholdExpressions: []string{"med<39"},
duration: 0,
},
want: true,
wantErr: false,
},
{
name: "Running thresholds of existing sink but failing threshold",
args: args{DummySink{"p(95)": 3000}, 0},
name: "Running threshold on trend sink with no values and non passing med statement fails",
args: args{
sink: &TrendSink{Values: []float64{}, Med: 0},
thresholdExpressions: []string{"med>39"},
duration: 0,
},
want: false,
wantErr: false,
},
{
name: "Running threshold on non existing sink does not fail",
args: args{DummySink{"dummy": 0}, 0},
name: "Running threshold on trend sink with values and passing med statement succeeds",
args: args{
sink: &TrendSink{Values: []float64{70, 80, 90}, Count: 3},
thresholdExpressions: []string{"med>39"},
duration: 0,
},
want: true,
wantErr: false,
},
{
name: "Running threshold on trend sink with values and failing med statement fails",
args: args{
sink: &TrendSink{Values: []float64{70, 80, 90}, Count: 3},
thresholdExpressions: []string{"med<39"},
duration: 0,
},
want: false,
wantErr: false,
},
}
for _, testCase := range tests {
testCase := testCase

t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

thresholds := NewThresholds([]string{"p(95)<2000"})
thresholds := NewThresholds(testCase.args.thresholdExpressions)
gotParseErr := thresholds.Parse()
require.NoError(t, gotParseErr)

Expand Down

0 comments on commit f63c848

Please sign in to comment.