`saved_search`: Cleanup saved objects mappings #153129

TinaHeiligers · 2023-03-10T19:52:06Z

Summary

Related to #149102

Subset of the global cleanup attempt in #153070

Cleans up saved_search mappings

Unit or functional tests were updated or added to match the most common scenarios

TinaHeiligers · 2023-03-10T19:54:59Z

@elastic/kibana-data-discovery if CI goes green, we can use this PR to clean up the savedSearch mappings.
If not, it means there's a direct reference to the fields and you'll need to decide how to handle that.

TinaHeiligers · 2023-03-21T13:57:38Z

@elasticmachine merge upstream

TinaHeiligers · 2023-03-23T12:07:18Z

@elasticmachine merge upstream

davismcphee

Does commenting out the index: false mappings mean we lose the only server-side validation we have (since we're currently using the SO client directly) until we migrate to a typed API, and are we ok with this?

davismcphee · 2023-03-23T15:21:39Z

src/plugins/saved_search/server/saved_objects/search.ts

          properties: {
+            // may need code changes


I'm not really sure what this means -- what code changes may we need?

If your code references any of the fields that could potentially be removed from the mappings, you might need to refactor. So, for example, if you're using sort from savedObjectsAttributes directly, you'd need to refactor to use sort from _source.

TinaHeiligers · 2023-03-27T15:25:45Z

Does commenting out the index: false mappings mean we lose the only server-side validation we have (since we're currently using the SO client directly) until we migrate to a typed API, and are we ok with this?

yes, on losing server-side validation and "it's up to your team" on being ok with that. It doesn't really matter if we comment the mappings out or delete them, I wanted to raise awareness of the urgency of migrating to a typed API! Some teams have decided it's ok to keep fields when absolutely needed if they're ok with never being able to delete/remove them.

@davismcphee, let me know if you're ok with removing these commented-out field mappings (or a subset of them) and I can see the work through. Alternatively, I'm also ok with closing this as not-doing and leaving it up to you folks to handle.

davismcphee · 2023-03-27T18:30:18Z

@TinaHeiligers Thanks for the additional context and info!

yes, on losing server-side validation and "it's up to your team" on being ok with that. It doesn't really matter if we comment the mappings out or delete them, I wanted to raise awareness of the urgency of migrating to a typed API!

Totally understandable, we're currently working with the Shared UX team on migrating our SOs to the Content Management system as services become available, so it hopefully shouldn't be this way for much longer.

@davismcphee, let me know if you're ok with removing these commented-out field mappings (or a subset of them) and I can see the work through. Alternatively, I'm also ok with closing this as not-doing and leaving it up to you folks to handle.

I put an item on our agenda to discuss this in our team sync tomorrow and will follow up here with our decisions. Thanks again!

TinaHeiligers · 2023-03-29T06:15:05Z

I put an item on our agenda to discuss this in our team sync tomorrow and will follow up here with our decisions. Thanks again!

@davismcphee what was the outcome?

TinaHeiligers · 2023-03-29T18:21:20Z

@elasticmachine merge upstream

kibana-ci · 2023-03-29T19:24:45Z

💚 Build Succeeded

Buildkite Build
Commit: 50571e8

Metrics [docs]

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/saved-objects-service.html#_mappings

id	before	after	diff
`search`	24	15	-9

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💚 Build #114957 succeeded 994c3b4
💚 Build #114359 succeeded 3cc86d1
💔 Build #113789 failed 3df723d
💔 Build #113427 failed 536f3ba

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

davismcphee · 2023-03-29T22:30:27Z

@TinaHeiligers We think we may actually be able to remove more mappings than the ones currently commented out. I'll use this work as a base, but I can open a new PR to see how many of these we can get rid of. Thanks for getting this started for us!

TinaHeiligers · 2023-03-30T06:05:08Z

I'll use this work as a base, but I can open a new PR to see how many of these we can get rid of.

@davismcphee ok, I'll go ahead and close this PR then.

## Summary This PR cleans up saved search mappings, removing all fields that aren't being searched or aggregated on. Continuation of the work started in #153129. ### Checklist - [ ] ~Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)~ - [ ] ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [ ] ~[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~ - [ ] ~Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))~ - [ ] ~Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))~ - [ ] ~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~ - [ ] ~This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))~ - [ ] ~This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)~ ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>

TinaHeiligers added 2 commits March 10, 2023 10:41

Cannot remove query from saved oquery mappings

6f99367

Comments out unindexed fields

536f3ba

TinaHeiligers added Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.8.0 labels Mar 10, 2023

updates snapshot

3df723d

Merge branch 'main' into saved-search-cleanup-index-false-mappings

3cc86d1

TinaHeiligers marked this pull request as ready for review March 23, 2023 11:57

TinaHeiligers requested review from a team as code owners March 23, 2023 11:57

Merge branch 'main' into saved-search-cleanup-index-false-mappings

994c3b4

pgayvallet approved these changes Mar 23, 2023

View reviewed changes

TinaHeiligers enabled auto-merge (squash) March 23, 2023 14:42

davismcphee reviewed Mar 23, 2023

View reviewed changes

Merge branch 'main' into saved-search-cleanup-index-false-mappings

50571e8

TinaHeiligers closed this Mar 30, 2023

auto-merge was automatically disabled March 30, 2023 06:05
Pull request was closed

davismcphee mentioned this pull request Mar 31, 2023

[Discover] [Saved Search] Cleanup saved search mappings #154016

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`saved_search`: Cleanup saved objects mappings #153129

`saved_search`: Cleanup saved objects mappings #153129

TinaHeiligers commented Mar 10, 2023 •

edited

Loading

TinaHeiligers commented Mar 10, 2023

TinaHeiligers commented Mar 21, 2023

TinaHeiligers commented Mar 23, 2023

davismcphee left a comment

davismcphee Mar 23, 2023

TinaHeiligers Mar 27, 2023

TinaHeiligers commented Mar 27, 2023 •

edited

Loading

davismcphee commented Mar 27, 2023

TinaHeiligers commented Mar 29, 2023

TinaHeiligers commented Mar 29, 2023

kibana-ci commented Mar 29, 2023

ESLint disabled line counts

Total ESLint disabled count

davismcphee commented Mar 29, 2023

TinaHeiligers commented Mar 30, 2023

saved_search: Cleanup saved objects mappings #153129

saved_search: Cleanup saved objects mappings #153129

Conversation

TinaHeiligers commented Mar 10, 2023 • edited Loading

Summary

TinaHeiligers commented Mar 10, 2023

TinaHeiligers commented Mar 21, 2023

TinaHeiligers commented Mar 23, 2023

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Mar 23, 2023

Choose a reason for hiding this comment

TinaHeiligers Mar 27, 2023

Choose a reason for hiding this comment

TinaHeiligers commented Mar 27, 2023 • edited Loading

davismcphee commented Mar 27, 2023

TinaHeiligers commented Mar 29, 2023

TinaHeiligers commented Mar 29, 2023

kibana-ci commented Mar 29, 2023

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

ESLint disabled line counts

Total ESLint disabled count

History

davismcphee commented Mar 29, 2023

TinaHeiligers commented Mar 30, 2023

`saved_search`: Cleanup saved objects mappings #153129

`saved_search`: Cleanup saved objects mappings #153129

TinaHeiligers commented Mar 10, 2023 •

edited

Loading

TinaHeiligers commented Mar 27, 2023 •

edited

Loading