From 9cebd49a2c0434da2856f2eeedc37b10bc9b87f1 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Wed, 8 Oct 2025 12:11:31 +0530 Subject: [PATCH] fix: anomaly with below operator negates the target (#9288) --- ee/anomaly/seasonal.go | 12 +- ee/query-service/rules/anomaly.go | 5 - pkg/types/ruletypes/api_params.go | 11 +- pkg/types/ruletypes/api_params_test.go | 350 +++++++++++++++++++++++++ 4 files changed, 367 insertions(+), 11 deletions(-) diff --git a/ee/anomaly/seasonal.go b/ee/anomaly/seasonal.go index 6636188b6d..49c136a98d 100644 --- a/ee/anomaly/seasonal.go +++ b/ee/anomaly/seasonal.go @@ -232,7 +232,7 @@ func (p *BaseSeasonalProvider) getPredictedSeries( // moving avg of the previous period series + z score threshold * std dev of the series // moving avg of the previous period series - z score threshold * std dev of the series func (p *BaseSeasonalProvider) getBounds( - series, predictedSeries *qbtypes.TimeSeries, + series, predictedSeries, weekSeries *qbtypes.TimeSeries, zScoreThreshold float64, ) (*qbtypes.TimeSeries, *qbtypes.TimeSeries) { upperBoundSeries := &qbtypes.TimeSeries{ @@ -246,8 +246,8 @@ func (p *BaseSeasonalProvider) getBounds( } for idx, curr := range series.Values { - upperBound := p.getMovingAvg(predictedSeries, movingAvgWindowSize, idx) + zScoreThreshold*p.getStdDev(series) - lowerBound := p.getMovingAvg(predictedSeries, movingAvgWindowSize, idx) - zScoreThreshold*p.getStdDev(series) + upperBound := p.getMovingAvg(predictedSeries, movingAvgWindowSize, idx) + zScoreThreshold*p.getStdDev(weekSeries) + lowerBound := p.getMovingAvg(predictedSeries, movingAvgWindowSize, idx) - zScoreThreshold*p.getStdDev(weekSeries) upperBoundSeries.Values = append(upperBoundSeries.Values, &qbtypes.TimeSeriesValue{ Timestamp: curr.Timestamp, Value: upperBound, @@ -398,8 +398,6 @@ func (p *BaseSeasonalProvider) getAnomalies(ctx context.Context, orgID valuer.UU aggOfInterest := result.Aggregations[0] for _, series := range aggOfInterest.Series { - stdDev := p.getStdDev(series) - p.logger.InfoContext(ctx, "calculated standard deviation for series", "anomaly_std_dev", stdDev, "anomaly_labels", series.Labels) pastPeriodSeries := p.getMatchingSeries(ctx, pastPeriodResult, series) currentSeasonSeries := p.getMatchingSeries(ctx, currentSeasonResult, series) @@ -407,6 +405,9 @@ func (p *BaseSeasonalProvider) getAnomalies(ctx context.Context, orgID valuer.UU past2SeasonSeries := p.getMatchingSeries(ctx, past2SeasonResult, series) past3SeasonSeries := p.getMatchingSeries(ctx, past3SeasonResult, series) + stdDev := p.getStdDev(currentSeasonSeries) + p.logger.InfoContext(ctx, "calculated standard deviation for series", "anomaly_std_dev", stdDev, "anomaly_labels", series.Labels) + prevSeriesAvg := p.getAvg(pastPeriodSeries) currentSeasonSeriesAvg := p.getAvg(currentSeasonSeries) pastSeasonSeriesAvg := p.getAvg(pastSeasonSeries) @@ -435,6 +436,7 @@ func (p *BaseSeasonalProvider) getAnomalies(ctx context.Context, orgID valuer.UU upperBoundSeries, lowerBoundSeries := p.getBounds( series, predictedSeries, + currentSeasonSeries, zScoreThreshold, ) aggOfInterest.UpperBoundSeries = append(aggOfInterest.UpperBoundSeries, upperBoundSeries) diff --git a/ee/query-service/rules/anomaly.go b/ee/query-service/rules/anomaly.go index af988b5d67..f4464d1290 100644 --- a/ee/query-service/rules/anomaly.go +++ b/ee/query-service/rules/anomaly.go @@ -78,11 +78,6 @@ func NewAnomalyRule( opts = append(opts, baserules.WithLogger(logger)) - if p.RuleCondition.CompareOp == ruletypes.ValueIsBelow { - target := -1 * *p.RuleCondition.Target - p.RuleCondition.Target = &target - } - baseRule, err := baserules.NewBaseRule(id, orgID, p, reader, opts...) if err != nil { return nil, err diff --git a/pkg/types/ruletypes/api_params.go b/pkg/types/ruletypes/api_params.go index df04c68382..2ca74f178c 100644 --- a/pkg/types/ruletypes/api_params.go +++ b/pkg/types/ruletypes/api_params.go @@ -207,6 +207,7 @@ func (r *PostableRule) processRuleDefaults() error { q.Expression = qLabel } } + //added alerts v2 fields if r.SchemaVersion == DefaultSchemaVersion { thresholdName := CriticalThresholdName @@ -215,12 +216,20 @@ func (r *PostableRule) processRuleDefaults() error { thresholdName = severity } } + + // For anomaly detection with ValueIsBelow, negate the target + targetValue := r.RuleCondition.Target + if r.RuleType == RuleTypeAnomaly && r.RuleCondition.CompareOp == ValueIsBelow && targetValue != nil { + negated := -1 * *targetValue + targetValue = &negated + } + thresholdData := RuleThresholdData{ Kind: BasicThresholdKind, Spec: BasicRuleThresholds{{ Name: thresholdName, TargetUnit: r.RuleCondition.TargetUnit, - TargetValue: r.RuleCondition.Target, + TargetValue: targetValue, MatchType: r.RuleCondition.MatchType, CompareOp: r.RuleCondition.CompareOp, Channels: r.PreferredChannels, diff --git a/pkg/types/ruletypes/api_params_test.go b/pkg/types/ruletypes/api_params_test.go index 5b33d0b72e..ee3b263675 100644 --- a/pkg/types/ruletypes/api_params_test.go +++ b/pkg/types/ruletypes/api_params_test.go @@ -718,3 +718,353 @@ func TestParseIntoRuleMultipleThresholds(t *testing.T) { assert.Equal(t, 1, len(vector)) } + +func TestAnomalyNegationShouldAlert(t *testing.T) { + tests := []struct { + name string + ruleJSON []byte + series v3.Series + shouldAlert bool + expectedValue float64 + }{ + { + name: "anomaly rule with ValueIsBelow - should alert", + ruleJSON: []byte(`{ + "alert": "AnomalyBelowTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 2.0, + "matchType": "1", + "op": "2", + "selectedQuery": "A" + } + }`), + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: -2.1}, // below & at least once, should alert + {Timestamp: 2000, Value: -2.3}, + }, + }, + shouldAlert: true, + expectedValue: -2.1, + }, + { + name: "anomaly rule with ValueIsBelow; should not alert", + ruleJSON: []byte(`{ + "alert": "AnomalyBelowTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 2.0, + "matchType": "1", + "op": "2", + "selectedQuery": "A" + } + }`), // below & at least once, no value below -2.0 + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: -1.9}, + {Timestamp: 2000, Value: -1.8}, + }, + }, + shouldAlert: false, + }, + { + name: "anomaly rule with ValueIsAbove; should alert", + ruleJSON: []byte(`{ + "alert": "AnomalyAboveTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 2.0, + "matchType": "1", + "op": "1", + "selectedQuery": "A" + } + }`), // above & at least once, should alert + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: 2.1}, // above 2.0, should alert + {Timestamp: 2000, Value: 2.2}, + }, + }, + shouldAlert: true, + expectedValue: 2.1, + }, + { + name: "anomaly rule with ValueIsAbove; should not alert", + ruleJSON: []byte(`{ + "alert": "AnomalyAboveTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 2.0, + "matchType": "1", + "op": "1", + "selectedQuery": "A" + } + }`), + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: 1.1}, + {Timestamp: 2000, Value: 1.2}, + }, + }, + shouldAlert: false, + }, + { + name: "anomaly rule with ValueIsBelow and AllTheTimes; should alert", + ruleJSON: []byte(`{ + "alert": "AnomalyBelowAllTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 2.0, + "matchType": "2", + "op": "2", + "selectedQuery": "A" + } + }`), // below and all the times + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: -2.1}, // all below -2 + {Timestamp: 2000, Value: -2.2}, + {Timestamp: 3000, Value: -2.5}, + }, + }, + shouldAlert: true, + expectedValue: -2.1, // max value when all are below threshold + }, + { + name: "anomaly rule with ValueIsBelow and AllTheTimes; should not alert", + ruleJSON: []byte(`{ + "alert": "AnomalyBelowAllTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 2.0, + "matchType": "2", + "op": "2", + "selectedQuery": "A" + } + }`), + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: -3.0}, + {Timestamp: 2000, Value: -1.0}, // above -2, breaks condition + {Timestamp: 3000, Value: -2.5}, + }, + }, + shouldAlert: false, + }, + { + name: "anomaly rule with ValueOutsideBounds; should alert", + ruleJSON: []byte(`{ + "alert": "AnomalyOutOfBoundsTest", + "ruleType": "anomaly_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 7.0, + "matchType": "1", + "op": "7", + "selectedQuery": "A" + } + }`), + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: -8.0}, // abs(−8) >= 7, alert + {Timestamp: 2000, Value: 5.0}, + }, + }, + shouldAlert: true, + expectedValue: -8.0, + }, + { + name: "non-anomaly threshold rule with ValueIsBelow; should alert", + ruleJSON: []byte(`{ + "alert": "ThresholdTest", + "ruleType": "threshold_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 90.0, + "matchType": "1", + "op": "2", + "selectedQuery": "A" + } + }`), + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: 80.0}, // below 90, should alert + {Timestamp: 2000, Value: 85.0}, + }, + }, + shouldAlert: true, + expectedValue: 80.0, + }, + { + name: "non-anomaly rule with ValueIsBelow - should not alert", + ruleJSON: []byte(`{ + "alert": "ThresholdTest", + "ruleType": "threshold_rule", + "condition": { + "compositeQuery": { + "queryType": "builder", + "queries": [{ + "type": "builder_query", + "spec": { + "name": "A", + "signal": "metrics", + "aggregations": [{"metricName": "test", "spaceAggregation": "p50"}], + "stepInterval": "5m" + } + }] + }, + "target": 50.0, + "matchType": "1", + "op": "2", + "selectedQuery": "A" + } + }`), + series: v3.Series{ + Labels: map[string]string{"host": "server1"}, + Points: []v3.Point{ + {Timestamp: 1000, Value: 60.0}, // below, should alert + {Timestamp: 2000, Value: 90.0}, + }, + }, + shouldAlert: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rule := PostableRule{} + err := json.Unmarshal(tt.ruleJSON, &rule) + if err != nil { + t.Fatalf("Failed to unmarshal rule: %v", err) + } + + ruleThreshold, err := rule.RuleCondition.Thresholds.GetRuleThreshold() + if err != nil { + t.Fatalf("unexpected error from GetRuleThreshold: %v", err) + } + + resultVector, err := ruleThreshold.ShouldAlert(tt.series, "") + if err != nil { + t.Fatalf("unexpected error from ShouldAlert: %v", err) + } + + shouldAlert := len(resultVector) > 0 + + if shouldAlert != tt.shouldAlert { + t.Errorf("Expected shouldAlert=%v, got %v. %s", + tt.shouldAlert, shouldAlert, tt.name) + } + + if tt.shouldAlert && len(resultVector) > 0 { + sample := resultVector[0] + if sample.V != tt.expectedValue { + t.Errorf("Expected alert value=%.2f, got %.2f. %s", + tt.expectedValue, sample.V, tt.name) + } + } + }) + } +}