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
[RAM][O11Y] Integrate Conditional Actions with several Observability rule types #159522
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
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.
@Zacqary the changes look good overall 👍 I've spotted a few potential problems and left comments regarding that but it doesn't look critical so I'm approving in advance.
|
||
const aadFields = useAsync(async () => { | ||
if (!ruleTypeId) return []; | ||
const fields = await http.get<DataViewField[]>(`${BASE_RAC_ALERTS_API_PATH}/aad_fields`, { |
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.
What if an http request fails for some reason?
@@ -243,13 +244,13 @@ export const RuleActionsField: React.FC<Props> = ({ | |||
setActionFrequencyProperty: setActionFrequency, | |||
setActionAlertsFilterProperty, | |||
featureId: SecurityConnectorFeatureId, | |||
producerId: AlertConsumers.SIEM, |
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.
This line is a bit confused as a producer
isn't the same as a consumer
. Making this assignment without knowing what's going on under the hood leads to questions like "Why is it the same?". As I see it's just used to conditionally display alerts filter. I'm pretty sure it will confuse the code readers later on. A boolean flag showActionAlertsFilter
was quite clear. Have you considered using an optional context or leaving a comment to make the intentions transparent?
@@ -37,6 +37,7 @@ export default function listRuleTypes({ getService }: FtrProviderContext) { | |||
name: 'Recovered', | |||
}, | |||
enabled_in_license: true, | |||
has_fields_for_a_a_d: false, |
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.
Such a diff highlights a disadvantage such bulk tests. Instead of testing one specific feature it works with the whole data set so any contract changes require changes here. If the test fails it's hard to say what's a real cause. I'd recommend considering partial asserting via expect.objectContaining
and etc.
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.
Rules management area changes LGTM
@elasticmachine merge upstream |
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.
Infra changes LGTM!
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.
AO changes LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
…rule types (elastic#159522) ## Summary Closes elastic#159520 <img width="573" alt="Screenshot 2023-06-12 at 3 12 27 PM" src="https://github.com/elastic/kibana/assets/1445834/ec16b8d7-25a5-435c-bf29-7392747b8c0f"> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com> (cherry picked from commit faadf34)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…ility rule types (#159522) (#160666) # Backport This will backport the following commits from `main` to `8.9`: - [[RAM][O11Y] Integrate Conditional Actions with several Observability rule types (#159522)](#159522) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Zacqary Adam Xeper","email":"Zacqary@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-06-27T16:36:49Z","message":"[RAM][O11Y] Integrate Conditional Actions with several Observability rule types (#159522)\n\n## Summary\r\n\r\nCloses #159520 \r\n\r\n<img width=\"573\" alt=\"Screenshot 2023-06-12 at 3 12 27 PM\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1445834/ec16b8d7-25a5-435c-bf29-7392747b8c0f\">\r\n\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>","sha":"faadf347ae903eba7e9035e417be135f3e36df17","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:ResponseOps","release_note:feature","Feature:Alerting/RulesManagement","v8.9.0","v8.10.0"],"number":159522,"url":"#159522 Integrate Conditional Actions with several Observability rule types (#159522)\n\n## Summary\r\n\r\nCloses #159520 \r\n\r\n<img width=\"573\" alt=\"Screenshot 2023-06-12 at 3 12 27 PM\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1445834/ec16b8d7-25a5-435c-bf29-7392747b8c0f\">\r\n\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>","sha":"faadf347ae903eba7e9035e417be135f3e36df17"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"#159522 Integrate Conditional Actions with several Observability rule types (#159522)\n\n## Summary\r\n\r\nCloses #159520 \r\n\r\n<img width=\"573\" alt=\"Screenshot 2023-06-12 at 3 12 27 PM\"\r\nsrc=\"https://github.com/elastic/kibana/assets/1445834/ec16b8d7-25a5-435c-bf29-7392747b8c0f\">\r\n\r\n\r\n### Checklist\r\n\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>","sha":"faadf347ae903eba7e9035e417be135f3e36df17"}}]}] BACKPORT--> Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>
…rule types (#159522) ## Summary Closes #159520 <img width="573" alt="Screenshot 2023-06-12 at 3 12 27 PM" src="https://github.com/elastic/kibana/assets/1445834/ec16b8d7-25a5-435c-bf29-7392747b8c0f"> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
Summary
Closes #159520
Checklist