[RAM] Add shareable rule tag filter #130710

JiaweiWu · 2022-04-20T17:39:31Z

Summary

Adds a shareable rule tags filter, this is a new component that does not exist as a part of the rule list currently. Therefore, it is not being used, except as a shareable component (hidden behind a feature flag).

This change also required new aggregations on the tags field since we need a list of all tags that the user can filter on. So the alerting aggregation function was modified. API and E2E tests were written to cover this.

…-fix'

mgiota · 2022-04-20T18:32:37Z

@JiaweiWu LGTM so far!

For the tags filter to work, I expect one change in this function call https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/application/lib/rule_api/rules.ts#L40 where we need to pass the new tagsFilter along with the rest filters that are currently being passed. This means that this file https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/application/lib/rule_api/map_filters_to_kql.ts needs to be changed as well.

JiaweiWu · 2022-04-26T04:20:14Z

@elasticmachine merge upstream

…-fix'

JiaweiWu · 2022-04-26T06:30:24Z

@mgiota I added changes to the loadRules and API changes, feel free to give it a test

JiaweiWu · 2022-04-26T19:48:26Z

@elasticmachine merge upstream

elasticmachine · 2022-04-28T00:31:09Z

Pinging @elastic/response-ops (Team:ResponseOps)

mgiota · 2022-04-28T13:19:48Z

@JiaweiWu I gave it a try and yep it works!

I will have a look again a bit later and approve the PR.

mgiota · 2022-04-28T19:39:27Z

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

@@ -930,6 +942,13 @@ export const RulesList: React.FunctionComponent = () => {
    );
  };

+  const getRuleTagFilter = () => {
+    if (isRuleTagFilterEnabled) {
+      return [<RuleTagFilter tags={tags} selectedTags={tagsFilter} onChange={setTagsFilter} />];


@JiaweiWu Since I had to do the loadRuleAggregations logic on our side same way you did above in order to setTags, I was wondering if it would make sense to make the tags prop optional and have this logic implemented within the RuleTagFilter, if you know what I mean. The code I had to add is similar to what you have on your side, that's why I am wondering if we could avoid this repetition.

I don't mind merging current PR and have a follow up PR when there's more time. I just wanted to hear your opinion, regarding moving the aggregations on the tags field within the RuleTagFilter, so that consumer doesn't have to write this logic. What do you think?

We had a chat and I brought up the issue that the rules_list aggregations require filter parameters from other filters as well (actionTypes, ruleStatuses, etc.) which makes it outside of the responsibility of the tag filter component.

I'm currently working on refactoring and making the rules list shareable, which will involves moving the fetching logic into hooks. So I think that will indirectly solve this problem.

mgiota

I approve this PR! Later on we could refactor so that the server requests the tags and the consumer doesn't have to pass any tags prop.

mgiota · 2022-04-29T07:23:18Z

It would be great if we could get this PR merged soon, so that I can working on #127013

gmmorris · 2022-04-29T11:11:19Z

x-pack/plugins/alerting/server/rules_client/rules_client.ts

@@ -990,6 +1000,9 @@ export class RulesClient {
      snoozed: snoozedBuckets.reduce((acc, bucket) => acc + bucket.doc_count, 0),
    };

+    const tagsBuckets = resp.aggregations.tags.buckets;


Are we confident the buckets are always there?
ES has a tendency to drop keys in aggs when there are no hits (such as a cluster with no rules in it or when a filter is applied).

Worth treading with caution and checking for undefined keys.

I tested it with rules that have no tags, but I'll add a fall back just in case

Always build these things defensively, otherwise the UI breaks and we have no idea unless someone files an SDH.

Thanks 🙏

chrisronline

Looking great so far, had a couple of thoughts/questions in my first pass

chrisronline · 2022-04-29T14:33:17Z

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

@@ -226,6 +232,7 @@ export const RulesList: React.FunctionComponent = () => {
    JSON.stringify(typesFilter),
    JSON.stringify(actionTypesFilter),
    JSON.stringify(ruleStatusesFilter),
+    JSON.stringify(tagsFilter),


I don't think you need to worry about this in this PR but this code should be improved. It looks like we're doing multiple JSON.stringify calls every render - we should redo this a bit to be more performant. WDYT?

@JiaweiWu In case it is useful, here's what I used https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/hooks/use_fetch_rules.ts#L36 when I moved this code under observability in a use_fetch_rules hook. I got rid of JSON.stringify and used useCallback.

Yep, this is getting improved in my rules_list refactor

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

...ns/triggers_actions_ui/public/application/sections/rules_list/components/rule_tag_filter.tsx

JiaweiWu · 2022-05-03T22:32:47Z

@elasticmachine merge upstream

JiaweiWu · 2022-05-03T22:46:06Z

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

@@ -345,6 +356,24 @@ export const RulesList: React.FunctionComponent = () => {
    }
  }

+  async function loadRuleTags() {


I want to give more context to this function here. So there's an awkward behaviour when I added the tag filters to the aggregation, because we use 1 aggregation function to get a list of tags and all of the rule execution status values, if we started filtering down the aggregation with tag filters, the list of tags would also get filtered, and thus it would replace the list of available tags with a smaller subset of tags (which is incorrect).

To fix this, i considered only calling setState on the tags list if the tagsFilter is empty. But this is also incorrect because we miss the edge case where the user adds new tags or a new rule while the tag filter is applied.

From there, I decided to add this function, which calls the aggregation with empty params, as a part of the data fetch. I think the downside here is we're doing the aggregation twice now, which has performance implications.

But aside from doing something client side like merging in new tags in react, we might eventually want to have a separate aggregation just to get a list of tags. Let me know if we want to do that in this PR or I could do it in another PR.

Is it possible to understand this more? How can I simulate this? Or can I see this happening on an older version of this PR?

I didn't have the tag filters passed into the aggregation for the previous commit, you can simulate it by checking out the previous commit and passing the tag filters:

rules_list.tsx: async function loadRuleAggs() { try { const rulesAggs = await loadRuleAggregations({ http, searchText, typesFilter, actionTypesFilter, ruleExecutionStatusesFilter, ruleStatusesFilter, tagsFilter, }); ... }

aggregate.ts: export async function loadRuleAggregations({ const filters = mapFiltersToKql({ typesFilter, actionTypesFilter, ruleExecutionStatusesFilter, ruleStatusesFilter, tagsFilter, }); ... }

Switching the experiment on and filter by tags using the dropdown, you'll notice the entire list of available tags gets filtered as well

Hmm

What about something like:

async function loadRuleAggs() { try { const rulesAggs = await loadRuleAggregations({ http, searchText, typesFilter, actionTypesFilter, ruleStatusesFilter, tagsFilter, }); if (rulesAggs?.ruleExecutionStatus) { setRulesStatusesTotal(rulesAggs.ruleExecutionStatus); } if (rulesAggs?.ruleTags && rulesAggs?.ruleTags.length > tags.length) { setTags(rulesAggs.ruleTags); } }

I guess this doesn't work if we delete a rule and a tag is only associated with that rule though

I think we should separate these two things out into separate "things" - api that is called that is assumed to return filtered data and a separate api call that is expected to return total data. We might need to add something to the lib/rule_api layer too but it's too confusing to keep this logic as-is - definitely recommend some delineation here to make it clear in the future.

yea thought so, ok I'll add a new endpoint to expose tags

kibana-ci · 2022-05-06T00:03:01Z

💔 Build Failed

Failed CI Steps

Post-Build

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	373	376	+3

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
`alerting`	316	317	+1
`triggersActionsUi`	316	320	+4
total			+5

Async chunks

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

id	before	after	diff
`triggersActionsUi`	728.2KB	732.4KB	+4.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`triggersActionsUi`	27	28	+1

Page load bundle

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

id	before	after	diff	errors
`triggersActionsUi`	100.5KB	101.1KB	+611.0B	❌ 169.0B over limit

Unknown metric groups

API count

id	before	after	diff
`alerting`	325	326	+1
`triggersActionsUi`	330	334	+4
total			+5

async chunk count

id	before	after	diff
`triggersActionsUi`	80	81	+1

ESLint disabled line counts

id	before	after	diff
`triggersActionsUi`	171	172	+1

Total ESLint disabled count

id	before	after	diff
`triggersActionsUi`	177	178	+1

History

💚 Build #42881 succeeded 7e2d6df
💚 Build #42269 succeeded 74fd8ab
💚 Build #41023 succeeded f0b2615
💔 Build #41018 failed df698fc
💛 Build #40944 was flaky 3ca21d7
💔 Build #40689 failed 93c1358

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

chrisronline

LGTM! Great work here

JiaweiWu · 2022-05-06T17:17:23Z

@elasticmachine merge upstream

JiaweiWu · 2022-05-06T17:37:53Z

Hey there! It seems like I have to slightly bump the bundle size for triggers_actions_ui again. For context, we have determined ways to lower our bundle size (#130728) + our current efforts to make make our components shareable to other plugins (which is what this PR is doing). Once that happens, our bundle size should go down quite a bit, however, right now we're in this awkward point where there's a lot of PRs happening to try and make this happen from various teams, so we're going to work on reducing the bundle size near the end of this release once the dust settles a bit.

packages/kbn-optimizer/limits.yml

JiaweiWu · 2022-05-06T19:41:25Z

@elasticmachine merge upstream

kibana-ci · 2022-05-06T20:42:23Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`triggersActionsUi`	373	376	+3

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
`alerting`	316	317	+1
`triggersActionsUi`	316	320	+4
total			+5

Async chunks

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

id	before	after	diff
`triggersActionsUi`	725.3KB	729.4KB	+4.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`triggersActionsUi`	27	28	+1

Page load bundle

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

id	before	after	diff
`triggersActionsUi`	100.2KB	100.8KB	+611.0B

Unknown metric groups

API count

id	before	after	diff
`alerting`	325	326	+1
`triggersActionsUi`	330	334	+4
total			+5

async chunk count

id	before	after	diff
`triggersActionsUi`	80	81	+1

ESLint disabled line counts

id	before	after	diff
`triggersActionsUi`	171	172	+1

Total ESLint disabled count

id	before	after	diff
`triggersActionsUi`	177	178	+1

History

💛 Build #43123 was flaky 869545a
💔 Build #43104 failed 5428990
💔 Build #42927 failed c35c626
💚 Build #42881 succeeded 7e2d6df
💚 Build #42269 succeeded 74fd8ab
💚 Build #41023 succeeded f0b2615

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

…hromium-to-print-pdf-part-1 * 'main' of github.com:elastic/kibana: (59 commits) [Cloud Posture] Enabled findings group by feature (elastic#131780) [EBT] Fix `userId` generation (elastic#131701) [RAM] Add shareable rule tag filter (elastic#130710) Optimize package installation performance, phase 2 (elastic#131627) [Screenshotting] instrument for benchmark tests using new EventLogger class (elastic#130356) [Connector] Adding internal route for requesting ad-hoc ServiceNow access token (elastic#131171) [ci] bump kibana-buildkite-library (elastic#131754) [Synthetics] UI clean up (elastic#131598) [RsponseOps] Fix flaky rules list test (elastic#131567) [Cases] Add severity field to create case (elastic#131626) [Discover] Monospace font in Document Explorer (elastic#131513) Sessions tab improvements (elastic#131583) Add cloud icon "ess-icon" at the end of the config keys in "alerting" documentation (elastic#131735) [DOCS] Updates deprecation text for legacy APIs (elastic#131741) [ci] break out skip patterns so they can change without triggering CI (elastic#131726) Adjust search session management page font size (elastic#131291) [Unified search] Fix uptime css problem (elastic#131730) [Actionable Observability] Link to filtered rules page (elastic#131629) Add openAPI specifications for cases endpoint (elastic#131275) Display rule API key owner to users who can manage API keys (elastic#131662) ... # Conflicts: # x-pack/plugins/screenshotting/server/formats/pdf/index.ts # x-pack/plugins/screenshotting/server/screenshots/observable.ts

* rule tag filter * lower case test subj * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Tag filter and aggregation APIs * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Fix and add more tests * Fix test * Fix test and add new function test * Addressed comments * Lint * Create new load tags function * bump bundle size Co-authored-by: kibanamachine <[email protected]>

JiaweiWu added 2 commits April 20, 2022 08:58

rule tag filter

561b99b

lower case test subj

1c4b4b3

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

28a8ba6

…-fix'

kibanamachine and others added 3 commits April 25, 2022 23:20

Merge branch 'main' into issue-130158-tag-filter

17374b3

Tag filter and aggregation APIs

d4efe95

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

85ec236

…-fix'

Fix and add more tests

04a5f8f

kibanamachine and others added 4 commits April 26, 2022 14:48

Merge branch 'main' into issue-130158-tag-filter

93c1358

Fix test

3ca21d7

Fix merge conflicts

df698fc

Fix test and add new function test

f0b2615

JiaweiWu marked this pull request as ready for review April 28, 2022 00:31

JiaweiWu requested a review from a team as a code owner April 28, 2022 00:31

JiaweiWu requested a review from mgiota April 28, 2022 00:31

JiaweiWu changed the title ~~[RAM] Add shareable rule tags filter~~ [RAM] Add shareable rule tag filter Apr 28, 2022

mgiota reviewed Apr 28, 2022

View reviewed changes

mgiota self-requested a review April 28, 2022 19:48

mgiota approved these changes Apr 28, 2022

View reviewed changes

This was referenced Apr 29, 2022

[o11y rules] Consume shareable filter per tags #127013

Closed

[Actionable Observability] consume tags filter #131220

Merged

gmmorris reviewed Apr 29, 2022

View reviewed changes

chrisronline reviewed Apr 29, 2022

View reviewed changes

Addressed comments

197b792

Merge branch 'main' into issue-130158-tag-filter

74fd8ab

JiaweiWu commented May 3, 2022

View reviewed changes

JiaweiWu requested review from chrisronline and gmmorris May 4, 2022 17:13

JiaweiWu added 3 commits May 5, 2022 11:52

Fix merge conflicts

0a6d7e1

Lint

7e2d6df

Create new load tags function

c35c626

chrisronline approved these changes May 6, 2022

View reviewed changes

Fix merge conflicts

5428990

kibanamachine and others added 2 commits May 6, 2022 12:17

Merge branch 'main' into issue-130158-tag-filter

64364c8

bump bundle size

869545a

JiaweiWu requested a review from a team as a code owner May 6, 2022 17:37

spalger suggested changes May 6, 2022

View reviewed changes

packages/kbn-optimizer/limits.yml Show resolved Hide resolved

spalger approved these changes May 6, 2022

View reviewed changes

Merge branch 'main' into issue-130158-tag-filter

010bec3

JiaweiWu merged commit 19298ee into elastic:main May 6, 2022

[RAM] Add shareable rule tag filter #130710

[RAM] Add shareable rule tag filter #130710

Conversation

JiaweiWu commented Apr 20, 2022 • edited Loading

Summary

mgiota commented Apr 20, 2022 • edited Loading

JiaweiWu commented Apr 26, 2022

JiaweiWu commented Apr 26, 2022

JiaweiWu commented Apr 26, 2022

elasticmachine commented Apr 28, 2022

mgiota commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiota left a comment • edited Loading

Choose a reason for hiding this comment

mgiota commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgiota Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaweiWu commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented May 6, 2022

💔 Build Failed

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

chrisronline left a comment

Choose a reason for hiding this comment

JiaweiWu commented May 6, 2022

JiaweiWu commented May 6, 2022

JiaweiWu commented May 6, 2022

kibana-ci commented May 6, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

JiaweiWu commented Apr 20, 2022 •

edited

Loading

mgiota commented Apr 20, 2022 •

edited

Loading

mgiota left a comment •

edited

Loading

mgiota Apr 29, 2022 •

edited

Loading