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

[Actionable Observability] o11y rules page #127406

Merged
merged 20 commits into from
Mar 16, 2022
Merged

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Mar 9, 2022

Fixes #126774
Fixes #126775
Fixes #123581
Fixes #127014
Fixes #127011
Fixes #127433

In order to get the new Rule page you should enable following feature flag xpack.observability.unsafe.rules.enabled: true

Acceptance criteria

  • Rules Table should have Rule, Last run, Last response, Status and Actions columns Style EuiBasicTable according to design #126774
    • Rule column shows the rule name and currently the rule type id (another ticket will show the rule type name)
    • Last run column reads the executionStatus.lastExecutionDate field
    • Last response column reads the executionStatus.status field. Possible values are:
      • Active
      • Error
      • OK
      • Pending
      • Unknown
    • Status column can be Enabled, Disabled and Muted. (Muted will be added in another ticket)
    • Actions column should have actions for Edit and Delete
  • User can filter by last response column Filter per last response #127014
  • User can Edit and Delete a rule Edit and Delete Actions #123581
  • User can create new Observability rule types Create new rule #127011
  • Create a useFetchRules hook that loads the Observability rule types Refactor Rules Table to use a useFetchRules hook #126775 and reacts to pagination, sorting and filtering changes. Table supports sorting and pagination
Screen.Recording.2022-03-09.at.20.57.06.mov

Notes for the reviewer

  • Current PR implements the rulesTable, useFetchRules hook and rest sub-components for the table columns inside the observability plugin and not the triggers_actions_ui plugin
  • Current PR is missing a few small things of the current rules_list implementation in the triggers_actions_ui plugin (refresh button, license modal, empty state, no permission prompt, read only users shouldn't do some actions). All these plus adding more filtering and adding the Muted status will be covered in follow up PRs.
  • This is the first step towards moving above files in the triggers_actions_ui plugin later on. Once the implementation is done in the observability plugin, we can move on with sharing functionality

Follow up issues

@mgiota mgiota self-assigned this Mar 10, 2022
@mgiota mgiota added v8.2.0 Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Mar 10, 2022
@mgiota mgiota marked this pull request as ready for review March 10, 2022 10:29
@mgiota mgiota requested a review from a team as a code owner March 10, 2022 10:29
@katrin-freihofner
Copy link
Contributor

@mgiota do you have a follow-up issue for adding search capabilities to the rules view? I wasn't able to find it.

@mgiota
Copy link
Contributor Author

mgiota commented Mar 14, 2022

@katrin-freihofner Good point. I created a new issue to track this #127579

@mgiota
Copy link
Contributor Author

mgiota commented Mar 14, 2022

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

@mgiota
Copy link
Contributor Author

mgiota commented Mar 14, 2022

@elasticmachine merge upstream

@mgiota
Copy link
Contributor Author

mgiota commented Mar 14, 2022

@XavierM I just pushed the Create new rule functionality in this open PR

Screen.Recording.2022-03-14.at.14.49.33.mov

Do you have an estimate when we can get this PR reviewed? The sooner we get it reviewed and merged the better, because it's gonna get longer and longer. I have quite a few subtickets I need to work on once this is merged.

@mgiota mgiota requested a review from a team March 14, 2022 14:41
useEffect(() => {
fetchRules();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(ruleLastResponseFilter), page, sort]);
Copy link
Contributor

Choose a reason for hiding this comment

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

that's kind of dangerous, if JSON.stringify throw an error

Copy link
Contributor

Choose a reason for hiding this comment

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

@mgiota, How about moving JSON.stringify(ruleLastResponseFilter) inside the try/catch block where loadRules function is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XavierM @fkanout I refactored this part of code with the use of useCallback. This way I got rid of the eslint-disable-next-line react-hooks/exhaustive-deps warning and stopped using JSON.stringify. It still works fine!

@@ -47,7 +50,12 @@ export { Plugin };
export * from './plugin';
// TODO remove this import when we expose the Rules tables as a component
export { loadRules } from './application/lib/rule_api/rules';
export { deleteRules } from './application/lib/rule_api/delete';
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to check something about this import to avoid our bundle getting bigger

Copy link
Contributor Author

@mgiota mgiota Mar 16, 2022

Choose a reason for hiding this comment

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

@XavierM Do you have any other suggestion how I could do it differently? I thought creating similar functions under observability that call directly the rules api, but I wanted to avoid duplication as much as possible. In 8.3 plan is to get rid of these imports anyway, right? Let me know what you think and if this is a blocker

Copy link
Contributor

Choose a reason for hiding this comment

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

it is not a blocker, it is more a mental note for me

@mgiota
Copy link
Contributor Author

mgiota commented Mar 16, 2022

@elasticmachine merge upstream

Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

Great work @mgiota!!
LGTM, with some minor and none-blocking things.

useEffect(() => {
fetchRules();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(ruleLastResponseFilter), page, sort]);
Copy link
Contributor

Choose a reason for hiding this comment

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

@mgiota, How about moving JSON.stringify(ruleLastResponseFilter) inside the try/catch block where loadRules function is called?

@fkanout
Copy link
Contributor

fkanout commented Mar 16, 2022

@mgiota The auto refresh is broken, from UI and functionality perspective.
Screenshot 2022-03-16 at 11 10 01

@mgiota
Copy link
Contributor Author

mgiota commented Mar 16, 2022

@mgiota The auto refresh is broken, from UI and functionality perspective. Screenshot 2022-03-16 at 11 10 01

Yep that's true. Plan is to fix this when I work on adding the Refresh button in this ticket #127436. I'll put some description in the ticket to fix the auto-refresh button as well (in worst case we remove it)

@mgiota mgiota requested a review from XavierM March 16, 2022 14:05
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

Let's go!!! Nice work!

@mgiota mgiota enabled auto-merge (squash) March 16, 2022 15:30
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 365 378 +13
triggersActionsUi 325 326 +1
total +14

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
triggersActionsUi 242 271 +29

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 398.2KB 408.6KB +10.4KB
triggersActionsUi 676.6KB 636.4KB -40.2KB
total -29.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observability 89.6KB 89.7KB +138.0B
triggersActionsUi 55.5KB 93.8KB +38.3KB
total +38.4KB
Unknown metric groups

API count

id before after diff
triggersActionsUi 254 283 +29

async chunk count

id before after diff
triggersActionsUi 66 65 -1

ESLint disabled in files

id before after diff
observability 2 3 +1

Total ESLint disabled count

id before after diff
observability 41 42 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mgiota

@mgiota mgiota merged commit ad5c67b into elastic:main Mar 16, 2022
maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022
* style rules table, refactor useFetchRules hook, edit_rule_flyout component,delete, pagination, sorting

* remove unused import

* fix translations

* change name column to rule and statusFilter to lastResponse filter

* remove unused code

* embed create rule flyout & create loadRuleTypesHook that accepts a filteredSolutions param

* fix failing tests

* fix last failing test

* Show rule type name in the Rule column

* PR review comments

* useMemo for panelItems on status_context

* close status context popover when clicking outside of the context menu

* refactor useFetchRules to get rid of react-hooks/exhaustive-deps warning

* remove console log statement

* useCallback for EuiBasicTable onChange

* cleanup async fetchRuleTypes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127406 or prevent reminders by adding the backport:skip label.

5 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127406 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127406 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127406 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127406 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 127406 or prevent reminders by adding the backport:skip label.

@mgiota mgiota added the backport:skip This commit does not require backporting label Mar 28, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 28, 2022
@mgiota mgiota deleted the o11y_rules_page branch April 6, 2022 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.2.0
Projects
None yet
6 participants