Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: test notification missing for anomaly alert #6391

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Nov 6, 2024

Summary

The missing part of #5469

The current test rule doesn't identify the anomaly rule

if parsedRule.RuleType == RuleTypeThreshold {
// add special labels for test alerts
parsedRule.Annotations[labels.AlertSummaryLabel] = fmt.Sprintf("The rule threshold is set to %.4f, and the observed metric value is {{$value}}.", *parsedRule.RuleCondition.Target)
parsedRule.Labels[labels.RuleSourceLabel] = ""
parsedRule.Labels[labels.AlertRuleIdLabel] = ""
// create a threshold rule
rule, err = NewThresholdRule(
alertname,
parsedRule,
m.featureFlags,
m.reader,
m.opts.UseLogsNewSchema,
WithSendAlways(),
WithSendUnmatched(),
)
if err != nil {
zap.L().Error("failed to prepare a new threshold rule for test", zap.String("name", rule.Name()), zap.Error(err))
return 0, newApiErrorBadData(err)
}
} else if parsedRule.RuleType == RuleTypeProm {
// create promql rule
rule, err = NewPromRule(
alertname,
parsedRule,
m.logger,
m.reader,
m.opts.PqlEngine,
WithSendAlways(),
WithSendUnmatched(),
)
if err != nil {
zap.L().Error("failed to prepare a new promql rule for test", zap.String("name", rule.Name()), zap.Error(err))
return 0, newApiErrorBadData(err)
}
} else {
return 0, newApiErrorBadData(fmt.Errorf("failed to derive ruletype with given information"))
}
To make it recognize, we need to anomaly to else clause. We can't import the ee licensed code into MIT licensed code. That means we add ee's own test rule capability. This change adds a test notification in ee with anomaly and default in MIT code without anomaly.

Other than that, I added support for {{$variable}} syntax and anomaly to telemetry tracking.


Important

Adds support for anomaly alerts in test notifications and updates template syntax handling.

  • Behavior:
    • Adds TestNotification function in ee/query-service/rules/manager.go to handle anomaly alerts.
    • Updates makeRulesManager in ee/query-service/app/server.go to include PrepareTestRuleFunc for anomaly alerts.
    • Adds AnomalyBasedAlerts to AlertsInfo in pkg/query-service/model/response.go.
  • Templates:
    • Updates preprocessTemplate in pkg/query-service/rules/templates.go to support {{$variable}} syntax.
    • Adds tests for new template syntax in pkg/query-service/rules/templates_test.go.
  • Misc:
    • Removes unused functions in pkg/query-service/rules/api_params.go.
    • Fixes struct initialization in pkg/query-service/rules/api_params_test.go.

This description was created by Ellipsis for 23d9a64. It will automatically update as commits are pushed.

@github-actions github-actions bot added the bug Something isn't working label Nov 6, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 23d9a64 in 2 minutes and 46 seconds

More details
  • Looked at 648 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. ee/query-service/rules/manager.go:87
  • Draft comment:
    The TestNotification function here duplicates logic found in pkg/query-service/rules/test_notification.go. Consider refactoring to avoid code duplication.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment points out potential code duplication, which is a valid concern for code quality. However, without access to the mentioned file, I cannot verify if the logic is indeed duplicated. The comment is speculative without strong evidence from the diff itself.
    I might be missing the context of the other file, which could confirm the duplication. However, the comment does not provide specific evidence from the diff itself.
    Without evidence from the diff, the comment remains speculative. The rules suggest removing speculative comments unless there's strong evidence.
    Remove the comment as it is speculative and lacks strong evidence from the diff itself.
2. pkg/query-service/rules/test_notification.go:16
  • Draft comment:
    The defaultTestNotification function duplicates logic found in ee/query-service/rules/manager.go. Consider refactoring to avoid code duplication.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pkg/query-service/model/response.go:620
  • Draft comment:
    The new field AnomalyBasedAlerts is added to track anomaly-based alerts. Ensure this is consistently used across the codebase where AlertsInfo is utilized.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new field AnomalyBasedAlerts in the AlertsInfo struct. This field is used in multiple places to track anomaly-based alerts. The implementation seems consistent with the existing pattern for other alert types.
4. pkg/query-service/rules/api_params_test.go:11
  • Draft comment:
    The use of concise struct initialization syntax improves readability. Ensure this is consistently applied across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The PR modifies the TestIsAllQueriesDisabled test function to use a more concise syntax for initializing structs. This is a good practice as it improves readability and reduces redundancy.
5. pkg/query-service/rules/api_params_test.go:23
  • Draft comment:
    The use of concise struct initialization syntax improves readability. Ensure this is consistently applied across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The PR modifies the TestIsAllQueriesDisabled test function to use a more concise syntax for initializing structs. This is a good practice as it improves readability and reduces redundancy.
6. pkg/query-service/rules/api_params_test.go:26
  • Draft comment:
    The use of concise struct initialization syntax improves readability. Ensure this is consistently applied across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The PR modifies the TestIsAllQueriesDisabled test function to use a more concise syntax for initializing structs. This is a good practice as it improves readability and reduces redundancy.
7. pkg/query-service/rules/api_params_test.go:37
  • Draft comment:
    The use of concise struct initialization syntax improves readability. Ensure this is consistently applied across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The PR modifies the TestIsAllQueriesDisabled test function to use a more concise syntax for initializing structs. This is a good practice as it improves readability and reduces redundancy.
8. pkg/query-service/rules/api_params_test.go:40
  • Draft comment:
    The use of concise struct initialization syntax improves readability. Ensure this is consistently applied across the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The PR modifies the TestIsAllQueriesDisabled test function to use a more concise syntax for initializing structs. This is a good practice as it improves readability and reduces redundancy.
9. ee/query-service/app/server.go:31
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
  • Reason this comment was not posted:
    Comment was on unchanged code.
10. ee/query-service/app/server.go:347
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
  • Reason this comment was not posted:
    Marked as duplicate.
11. ee/query-service/app/server.go:764
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_wh1w4zujdRKtiOBE


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

pkg/query-service/rules/manager.go Show resolved Hide resolved
@srikanthccv srikanthccv enabled auto-merge (squash) November 8, 2024 15:31
@srikanthccv srikanthccv merged commit 831540e into develop Nov 8, 2024
16 checks passed
@srikanthccv srikanthccv deleted the anomaly-gaps branch November 8, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants