[ResponseOps] Mapped/searchable params #126531

JiaweiWu · 2022-02-28T21:35:03Z

Summary

Addresses the issue: #124338

Complete implementation allowing for mapped/Searchable params. The change maps the values from params, which is a flattened field, to a mapped field called mapped_params. This lets us sort/filter/search on these values. To the public, these fields should be hidden, attempts to use mapped_params should not be allowed.

currently we are supporting:

params.risk_score -> mapped_params.risk_score
params.severity -> mapped_params.severity

In the background, mapped_params.severity is stored as 20-low, 40-medium, etc.... This enables us to sort on these fields.

Here's a screenshot that shows sorting by risk score, which is a params property, using mapped_params

Future additions to mapped_params will be done at an ad-hoc basis.

This change contains the following:

Create an util function to extract our mapped_params from the params attribute
Create an util function to replace our query parameters like sort_fields and search_fields and filter to use mapped_params instead of params field when we can
Integrate our util functions to do our migration around mapped params
Integrate our util functions around our I/O function like create/update/find
Make sure to hide our mapped_params from our public API

elasticmachine · 2022-02-28T21:35:05Z

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

JiaweiWu · 2022-02-28T21:49:11Z

...plugins/security_solution/public/detections/pages/detection_engine/rules/all/use_columns.tsx

@@ -196,15 +196,15 @@ export const useRulesColumns = ({ hasPermissions }: ColumnsProps): TableColumn[]
            {value}
          </EuiText>
        ),
-        sortable: !!isInMemorySorting,
+        sortable: true,


This and line 207 are only here for testing/demoing purposes. Will be removed once this PR is approved

JiaweiWu · 2022-03-01T18:18:42Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/find.ts

+      it('should sort by parameters', async () => {
+        const response = await supertest.get(
+          `${getUrlPrefix(Spaces.space1.id)}/${
+            describeType === 'public' ? 'api' : 'internal'
+          }/alerting/rules/_find?sort_field=params.severity&sort_order=asc`
+        );
+        expect(response.body.data[0].params.severity).to.equal('low');
+        expect(response.body.data[1].params.severity).to.equal('medium');
+        expect(response.body.data[2].params.severity).to.equal('high');


I'm not sure why but this test fails in CI, but I can't seem to get it to fail locally (always passes)

How are you running it locally? Using --grep?

correct, using the functional test runner and --grep="find".

Try running it a level up until you find the error, so instead of --grep="find", try --grep="Alerting"

Ah got it, makes sense, yea I'll try that

chrisronline · 2022-03-01T19:04:57Z

What do you think about adding a test in our migrations file? https://github.com/elastic/kibana/blob/main/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/migrations.ts

You'll need to add a new alert, or just modify an existing one in the archive, to contain the legacy data and ensure the migration occurred smoothly

JiaweiWu · 2022-03-01T19:06:42Z

Yep I can

chrisronline

This looks really really good! I had a couple of notes/comments so far

chrisronline · 2022-03-01T19:20:45Z

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

+  }, []);
+};
+
+export const getModifiedValue = (key: string, value: string) => {


Can we add a comment here explaining why we need to do this? It looks like we want to remap these values but I don't know why

I can explain, it is because in the params, we are saving the severity like critical, hight ... but in the mapped params, we are saving it like 80-critical, 60-high .... Therefore we can sort the severity field the right way.

Ah okay so to avoid needing to store the label and the value, we're going to put the value in the label so we can do the right sort. We should add a clarifying comment for future folks though

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

chrisronline · 2022-03-01T20:37:48Z

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

+    }
+
+    if (filterKueryNode) {
+      modifyFilterKueryNode({ astFilter: filterKueryNode });


Can you explain why this is necessary? Does the esKuery.fromKueryExpression not handle everything we need?

the reason is if your query has field which belong in the mapped_fields, we will convert this field to use the mapped_params like if you want to filter on risk_score the query will be something like alert.attributes.params.risk_score > 50 then we will convert it to alert.attributes.mapped_params.risk_score > 50

Ah okay so the filter kuery node functionality has no understanding of this mapped_params duality state situation so we need to explicitly modify the results to handle that - okay makes sense - lets add a clarifying comment for future folks

XavierM · 2022-03-02T17:35:30Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/find.ts

+        const response = await supertest.get(
+          `${getUrlPrefix(Spaces.space1.id)}/${
+            describeType === 'public' ? 'api' : 'internal'
+          }/alerting/rules/_find?search_fields=params.severity&search=40-medium`


I think the test should be like that and behind the scene we are doing the change

Suggested change

}/alerting/rules/_find?search_fields=params.severity&search=40-medium`

}/alerting/rules/_find?search_fields=params.severity&search=medium`

XavierM · 2022-03-02T17:36:07Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/find.ts

+        expect(response.body.total).to.equal(1);
+        expect(response.body.data[0].params.risk_score).to.eql(40);
+
+        if (describeType === 'public') {


Looks great in practice! (The separate of public and internal)

mdefazio · 2022-03-03T00:01:27Z

@JiaweiWu Is it possible to add in additional screenshots to the description of the PR? I thought I remember seeing this demoed outside of the security table with a specific search box. It'll help me provide some UI feedback if I can see it in all scenarios. Otherwise, if / when you're ready we can just do a quick zoom to go through it together.

JiaweiWu · 2022-03-03T00:39:27Z

@elasticmachine merge upstream

JiaweiWu · 2022-03-03T00:41:54Z

@mdefazio
Hey! Yea I created those search boxes pretty ad-hoc just to demo the searching and filtering since these are new searchable fields, so we currently don't have UI that performs this search. This PR is mostly the backend change to enable this, but yea, feel free to throw something in my calendar and I can give you more information.

XavierM · 2022-03-03T15:25:51Z

@elasticmachine merge upstream

chrisronline · 2022-03-03T21:56:33Z

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

-    if (options.searchFields) {
-      options.searchFields = getModifiedSearchFields(options.searchFields);
-    }
+    // Generate new modified search and search fields, translating certain params properties


XavierM · 2022-03-04T13:44:28Z

@elasticmachine merge upstream

XavierM

Well done!, exactly what we wanted. You are going to make some people happy!

chrisronline

LGTM!

kibana-ci · 2022-03-04T15:27:15Z

💚 Build Succeeded

Metrics [docs]

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`	293	298	+5

Async chunks

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

id	before	after	diff
`securitySolution`	4.7MB	4.7MB	-2.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`alert`	48	51	+3

Unknown metric groups

API count

id	before	after	diff
`alerting`	301	306	+5

History

💛 Build #27414 was flaky 904ddfc
💔 Build #27281 failed 575d0a8
💔 Build #27277 failed e66e75c
💔 Build #27275 failed da7cef9
💔 Build #26658 failed a6b82e5

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

JiaweiWu added 2 commits February 28, 2022 13:16

Mapped params implementation with unit/integration/migration tests

bc58b94

lint

a6b82e5

JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) ci:deploy-cloud v8.2.0 labels Feb 28, 2022

JiaweiWu requested review from chrisronline and XavierM February 28, 2022 21:35

JiaweiWu requested review from a team as code owners February 28, 2022 21:35

JiaweiWu changed the title ~~Mapped/searchable params~~ [ResponseOps] Mapped/searchable params Feb 28, 2022

JiaweiWu commented Feb 28, 2022

View reviewed changes

JiaweiWu commented Mar 1, 2022

View reviewed changes

chrisronline reviewed Mar 1, 2022

View reviewed changes

XavierM reviewed Mar 2, 2022

View reviewed changes

JiaweiWu added 2 commits March 2, 2022 16:31

Address comments

da7cef9

Revert demo files

5b0b6ab

Merge branch 'main' into issue-124338-mapped-params

e66e75c

Update tests after merge

575d0a8

Merge branch 'main' into issue-124338-mapped-params

904ddfc

JiaweiWu requested review from chrisronline and XavierM March 3, 2022 19:25

chrisronline reviewed Mar 3, 2022

View reviewed changes

Merge branch 'main' into issue-124338-mapped-params

6a0cb15

XavierM approved these changes Mar 4, 2022

View reviewed changes

chrisronline approved these changes Mar 4, 2022

View reviewed changes

joshdover mentioned this pull request Mar 4, 2022

[Fleet] Automatically quote vars that start with asterisks or ampersands #126919

Closed

1 task

JiaweiWu merged commit f7f8d6d into elastic:main Mar 4, 2022

banderror mentioned this pull request Mar 7, 2022

[Security Solution][Detections] Server-side filtering and sorting POC #126450

Closed

3 tasks

xcrzx mentioned this pull request Mar 16, 2022

Filtering rules by rule params doesn't work in RulesClient #127876

Closed

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps] Mapped/searchable params #126531

[ResponseOps] Mapped/searchable params #126531

JiaweiWu commented Feb 28, 2022

elasticmachine commented Feb 28, 2022

JiaweiWu Feb 28, 2022

JiaweiWu Mar 1, 2022

chrisronline Mar 1, 2022

JiaweiWu Mar 1, 2022

chrisronline Mar 1, 2022

JiaweiWu Mar 1, 2022

chrisronline commented Mar 1, 2022

JiaweiWu commented Mar 1, 2022

chrisronline left a comment

chrisronline Mar 1, 2022

XavierM Mar 2, 2022

chrisronline Mar 2, 2022

chrisronline Mar 1, 2022

XavierM Mar 2, 2022

chrisronline Mar 2, 2022

XavierM Mar 2, 2022

XavierM Mar 2, 2022

chrisronline Mar 2, 2022

mdefazio commented Mar 3, 2022

JiaweiWu commented Mar 3, 2022

JiaweiWu commented Mar 3, 2022

XavierM commented Mar 3, 2022

chrisronline Mar 3, 2022

XavierM commented Mar 4, 2022

XavierM left a comment

chrisronline left a comment

kibana-ci commented Mar 4, 2022

API count

	}/alerting/rules/_find?search_fields=params.severity&search=40-medium`
	}/alerting/rules/_find?search_fields=params.severity&search=medium`

[ResponseOps] Mapped/searchable params #126531

[ResponseOps] Mapped/searchable params #126531

Conversation

JiaweiWu commented Feb 28, 2022

Summary

elasticmachine commented Feb 28, 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

Choose a reason for hiding this comment

chrisronline commented Mar 1, 2022

JiaweiWu commented Mar 1, 2022

chrisronline left a 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

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

Choose a reason for hiding this comment

mdefazio commented Mar 3, 2022

JiaweiWu commented Mar 3, 2022

JiaweiWu commented Mar 3, 2022

XavierM commented Mar 3, 2022

Choose a reason for hiding this comment

XavierM commented Mar 4, 2022

XavierM left a comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 4, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Saved Objects .kibana field count

API count

History