-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Synthetics] Fix recover alert while monitor is down #237479
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
[Synthetics] Fix recover alert while monitor is down #237479
Conversation
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
| } else { | ||
| from = this.previousStartedAt | ||
| ? moment(this.previousStartedAt).subtract(1, 'minute').toISOString() | ||
| : 'now-2m'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Work to get rid of this by ensuring that either useLatestChecks or timeWindow are true. From the getConditionType they should always be the case. You can then return the range directly within the conditionals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of the problematic 2 minute fallback in this commit. Does it look good?
| return { from, to: 'now' }; | ||
| } | ||
|
|
||
| // `timeWindow` is guaranteed to be defined here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above useLatestChecks condition will be true for both default rules and custom "number of checks" rules.
In case of no default rule or no custom "number of checks" rule, we know for sure it is a time window rule. That's why I considered it redundant to explicitly check for if (timeWindow)
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| const maxPeriod = 60000; // 1 minute | ||
|
|
||
| it('should return the correct range for a default rule', () => { | ||
| const range = statusRule.getRange(maxPeriod); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the rest of this file and I'm not sure this is testing the default rule in the way we expect.
We want to test a rule that does not have a condition.
However, I noticed that the statusRule variable is defined on line 78 in the describe block, and in later it statements the params value is changed. However the statusRule is never set back to it's original in an beforeEach clause, for example.
So to have certainty that you're testing with a rule that actually does not have a condition yet, define your fixture in the same way you did on line 596 by explicitly defining params with condition undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dominiqueclarke Yep you are absolutely right. I pass empty params in this commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean to nit, but I think the pattern of mutating the statusRule statusRule.params = {} is bad. Not just here of course, but the way it's being handled across the file is not a good pattern.
Instead please just create a new custom rule
const customStatusRule = new StatusRuleExecutor(esClient, serverMock, monitorClient, {
params: {},
services: {
uiSettingsClient,
savedObjectsClient: soClient,
scopedClusterClient: { asCurrentUser: mockEsClient },
},
rule: {
name: 'test',
},
} as any);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here. I fixed only my part. Let me know if you want me to refactor the other places rule is being mutated.
|
@mgiota as a bug fix, this needs to be backported to 8.19.x, 9.1.x and 9.2.0 |
36d539c to
efc817b
Compare
|
@dominiqueclarke I fixed the test and added backport labels. |
💚 Build Succeeded
Metrics [docs]
History
cc @mgiota |
dominiqueclarke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Starting backport for target branches: 8.19, 9.1, 9.2 |
Fixes elastic#237480 This PR fixes a bug with Synthetics Alerting, where a down monitor triggered recovered alerts when it shouldn't. This affected the default monitor status alert. Because the lookback window was too small, this caused the default rule to often execute queries that fundamentally resulted in no data. As a workaround the default rule now makes use of the max period when determining the lookback period, similar to how custom rules are implemented. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit c930787)
Fixes elastic#237480 This PR fixes a bug with Synthetics Alerting, where a down monitor triggered recovered alerts when it shouldn't. This affected the default monitor status alert. Because the lookback window was too small, this caused the default rule to often execute queries that fundamentally resulted in no data. As a workaround the default rule now makes use of the max period when determining the lookback period, similar to how custom rules are implemented. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit c930787)
Fixes elastic#237480 This PR fixes a bug with Synthetics Alerting, where a down monitor triggered recovered alerts when it shouldn't. This affected the default monitor status alert. Because the lookback window was too small, this caused the default rule to often execute queries that fundamentally resulted in no data. As a workaround the default rule now makes use of the max period when determining the lookback period, similar to how custom rules are implemented. --------- Co-authored-by: Elastic Machine <[email protected]> (cherry picked from commit c930787)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…239416) # Backport This will backport the following commits from `main` to `9.2`: - [[Synthetics] Fix recover alert while monitor is down (#237479)](#237479) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Panagiota Mitsopoulou","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-10-16T17:31:43Z","message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e","branchLabelMapping":{"^v9.3.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:obs-ux-management","backport:version","v9.2.0","v9.3.0","v9.1.6","v8.19.6"],"title":"[Synthetics] Fix recover alert while monitor is down","number":237479,"url":"https://github.com/elastic/kibana/pull/237479","mergeCommit":{"message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e"}},"sourceBranch":"main","suggestedTargetBranches":["9.2","9.1","8.19"],"targetPullRequestStates":[{"branch":"9.2","label":"v9.2.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.3.0","branchLabelMappingKey":"^v9.3.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/237479","number":237479,"mergeCommit":{"message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e"}},{"branch":"9.1","label":"v9.1.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Panagiota Mitsopoulou <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
…#239414) # Backport This will backport the following commits from `main` to `8.19`: - [[Synthetics] Fix recover alert while monitor is down (#237479)](#237479) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Panagiota Mitsopoulou","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-10-16T17:31:43Z","message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e","branchLabelMapping":{"^v9.3.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:obs-ux-management","backport:version","v9.2.0","v9.3.0","v9.1.6","v8.19.6"],"title":"[Synthetics] Fix recover alert while monitor is down","number":237479,"url":"https://github.com/elastic/kibana/pull/237479","mergeCommit":{"message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e"}},"sourceBranch":"main","suggestedTargetBranches":["9.2","9.1","8.19"],"targetPullRequestStates":[{"branch":"9.2","label":"v9.2.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.3.0","branchLabelMappingKey":"^v9.3.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/237479","number":237479,"mergeCommit":{"message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e"}},{"branch":"9.1","label":"v9.1.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Panagiota Mitsopoulou <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
…239415) # Backport This will backport the following commits from `main` to `9.1`: - [[Synthetics] Fix recover alert while monitor is down (#237479)](#237479) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Panagiota Mitsopoulou","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-10-16T17:31:43Z","message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e","branchLabelMapping":{"^v9.3.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:obs-ux-management","backport:version","v9.2.0","v9.3.0","v9.1.6","v8.19.6"],"title":"[Synthetics] Fix recover alert while monitor is down","number":237479,"url":"https://github.com/elastic/kibana/pull/237479","mergeCommit":{"message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e"}},"sourceBranch":"main","suggestedTargetBranches":["9.2","9.1","8.19"],"targetPullRequestStates":[{"branch":"9.2","label":"v9.2.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.3.0","branchLabelMappingKey":"^v9.3.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/237479","number":237479,"mergeCommit":{"message":"[Synthetics] Fix recover alert while monitor is down (#237479)\n\nFixes https://github.com/elastic/kibana/issues/237480\n\nThis PR fixes a bug with Synthetics Alerting, where a down monitor\ntriggered recovered alerts when it shouldn't. This affected the default\nmonitor status alert. Because the lookback window was too small, this\ncaused the default rule to often execute queries that fundamentally\nresulted in no data.\n\nAs a workaround the default rule now makes use of the max period when\ndetermining the lookback period, similar to how custom rules are\nimplemented.\n\n---------\n\nCo-authored-by: Elastic Machine <[email protected]>","sha":"c930787f2ee5bad16f23be8c24fc60647bc0c03e"}},{"branch":"9.1","label":"v9.1.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.6","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Panagiota Mitsopoulou <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Fixes elastic#237480 This PR fixes a bug with Synthetics Alerting, where a down monitor triggered recovered alerts when it shouldn't. This affected the default monitor status alert. Because the lookback window was too small, this caused the default rule to often execute queries that fundamentally resulted in no data. As a workaround the default rule now makes use of the max period when determining the lookback period, similar to how custom rules are implemented. --------- Co-authored-by: Elastic Machine <[email protected]>
Fixes elastic#237480 This PR fixes a bug with Synthetics Alerting, where a down monitor triggered recovered alerts when it shouldn't. This affected the default monitor status alert. Because the lookback window was too small, this caused the default rule to often execute queries that fundamentally resulted in no data. As a workaround the default rule now makes use of the max period when determining the lookback period, similar to how custom rules are implemented. --------- Co-authored-by: Elastic Machine <[email protected]>
Fixes #237480
This PR fixes a bug with Synthetics Alerting, where a down monitor triggered recovered alerts when it shouldn't. This affected the default monitor status alert. Because the lookback window was too small, this caused the default rule to often execute queries that fundamentally resulted in no data.
As a workaround the default rule now makes use of the max period when determining the lookback period, similar to how custom rules are implemented.