-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Custom threshold] Add missing prefill fields to the custom threshold rule #174690
Conversation
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
Can you add to the PR the prefill for alertOnGroupDisappear
?
@@ -277,23 +273,13 @@ export default function Expressions(props: Props) { | |||
[ruleParams.criteria, setRuleParams] | |||
); | |||
|
|||
const preFillFilterQuery = useCallback(() => { |
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.
@fkanout I changed how we prefill filter
so it matches how we save it in the rule. It will also allow us to extend this filter more easily in the future. Here is what the new structure looks like:
searchConfiguration: {
index: 'metrics-*',
query: {
query: 'host.name: host-1',
language: 'kuery',
},
},
So we don't need to prefill the filter query separately anymore.
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 tested it locally, and it works as expected! Good job @maryam-saeidi
One nit thing: I just noticed that there is no prefill for the optional label field. Could you add it, too?
@fkanout I tested locally and the optional field worked as expected, was there any issue with it in your test? Update: |
timeUnit: 'm', | ||
comparator: Comparator.LT_OR_EQ, | ||
equation: 'A * B', | ||
label: 'prefill label', |
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.
Nice 👍🏻
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
… rule (elastic#174690) Fixes elastic#174824 ## Summary This PR adds prefill functionality for `time size`, `time unit`, and `alertOnGroupDisappear` in the custom threshold. It also fixes the filter query by changing the structure to match how we save information in the rule saved object, like this: ``` searchConfiguration: { index: 'metrics-*', query: { query: 'host.name: host-1', language: 'kuery', }, }, ``` This will be more compatible with future changes in case we want to support query filters other than `kuery`. --------- Co-authored-by: Faisal Kanout <[email protected]>
Fixes #174824
Summary
This PR adds prefill functionality for
time size
,time unit
, andalertOnGroupDisappear
in the custom threshold.It also fixes the filter query by changing the structure to match how we save information in the rule saved object, like this:
This will be more compatible with future changes in case we want to support query filters other than
kuery
.