[Search Source] Update search source to use minimal data view spec for serialization #169460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davismcphee
added
Feature:Search
davismcphee
force-pushed
the
search-source-minimal-spec
branch
2 times, most recently
from
f802017
to
8c97bde
Compare
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
davismcphee
force-pushed
the
search-source-minimal-spec
branch
from
8c97bde
to
f067163
Compare
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @davismcphee |
jughosta
approved these changes
Oct 26, 2023
drewdaemon
approved these changes
Oct 26, 2023
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 26, 2023
…r serialization (elastic#169460) ## Summary Currently we use the full data view spec whenever we serialize fields for a search source with an ad hoc data view, except when otherwise specified (currently only when serializing for alert URLs). As a result, whenever we persist a serialized search source containing an ad hoc data view within a saved object, we end up persisting the entire data view field list as well. For data views that target hundreds or thousands of fields, this can result in huge saved objects that exceed Kibana's max payload size, causing issues like the one seen in elastic#168573. Does it ever make sense to persist the full data view spec now that we have a minimal spec (strips all field list data except popularity counts, custom labels, etc.)? I don't think it does, so this PR updates search source to always use the minimal data view spec for ad hoc data views, limiting the impact of large field lists. Barring a good reason not to do this, I think we should make this the standard behaviour. Fixes elastic#168573. ### 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 - [x] [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: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit f0bf4d2)
kibanamachine
added a commit
that referenced
this pull request
Oct 26, 2023
…spec for serialization (#169460) (#169941) # Backport This will backport the following commits from `main` to `8.11`: - [[Search Source] Update search source to use minimal data view spec for serialization (#169460)](#169460) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis McPhee","email":"davis.mcphee@elastic.co"},"sourceCommit":{"committedDate":"2023-10-26T13:51:05Z","message":"[Search Source] Update search source to use minimal data view spec for serialization (#169460)\n\n## Summary\r\n\r\nCurrently we use the full data view spec whenever we serialize fields\r\nfor a search source with an ad hoc data view, except when otherwise\r\nspecified (currently only when serializing for alert URLs). As a result,\r\nwhenever we persist a serialized search source containing an ad hoc data\r\nview within a saved object, we end up persisting the entire data view\r\nfield list as well. For data views that target hundreds or thousands of\r\nfields, this can result in huge saved objects that exceed Kibana's max\r\npayload size, causing issues like the one seen in #168573.\r\n\r\nDoes it ever make sense to persist the full data view spec now that we\r\nhave a minimal spec (strips all field list data except popularity\r\ncounts, custom labels, etc.)? I don't think it does, so this PR updates\r\nsearch source to always use the minimal data view spec for ad hoc data\r\nviews, limiting the impact of large field lists. Barring a good reason\r\nnot to do this, I think we should make this the standard behaviour.\r\n\r\nFixes #168573.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [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- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"f0bf4d2e9536ed74fb3ea13d75f23c5b305b5817","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Search","release_note:fix","Team:DataDiscovery","v8.11.0","v8.12.0"],"number":169460,"url":"https://github.com/elastic/kibana/pull/169460","mergeCommit":{"message":"[Search Source] Update search source to use minimal data view spec for serialization (#169460)\n\n## Summary\r\n\r\nCurrently we use the full data view spec whenever we serialize fields\r\nfor a search source with an ad hoc data view, except when otherwise\r\nspecified (currently only when serializing for alert URLs). As a result,\r\nwhenever we persist a serialized search source containing an ad hoc data\r\nview within a saved object, we end up persisting the entire data view\r\nfield list as well. For data views that target hundreds or thousands of\r\nfields, this can result in huge saved objects that exceed Kibana's max\r\npayload size, causing issues like the one seen in #168573.\r\n\r\nDoes it ever make sense to persist the full data view spec now that we\r\nhave a minimal spec (strips all field list data except popularity\r\ncounts, custom labels, etc.)? I don't think it does, so this PR updates\r\nsearch source to always use the minimal data view spec for ad hoc data\r\nviews, limiting the impact of large field lists. Barring a good reason\r\nnot to do this, I think we should make this the standard behaviour.\r\n\r\nFixes #168573.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [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- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"f0bf4d2e9536ed74fb3ea13d75f23c5b305b5817"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/169460","number":169460,"mergeCommit":{"message":"[Search Source] Update search source to use minimal data view spec for serialization (#169460)\n\n## Summary\r\n\r\nCurrently we use the full data view spec whenever we serialize fields\r\nfor a search source with an ad hoc data view, except when otherwise\r\nspecified (currently only when serializing for alert URLs). As a result,\r\nwhenever we persist a serialized search source containing an ad hoc data\r\nview within a saved object, we end up persisting the entire data view\r\nfield list as well. For data views that target hundreds or thousands of\r\nfields, this can result in huge saved objects that exceed Kibana's max\r\npayload size, causing issues like the one seen in #168573.\r\n\r\nDoes it ever make sense to persist the full data view spec now that we\r\nhave a minimal spec (strips all field list data except popularity\r\ncounts, custom labels, etc.)? I don't think it does, so this PR updates\r\nsearch source to always use the minimal data view spec for ad hoc data\r\nviews, limiting the impact of large field lists. Barring a good reason\r\nnot to do this, I think we should make this the standard behaviour.\r\n\r\nFixes #168573.\r\n\r\n### Checklist\r\n\r\n- [ ] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [x] [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- [ ] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>","sha":"f0bf4d2e9536ed74fb3ea13d75f23c5b305b5817"}}]}] BACKPORT--> Co-authored-by: Davis McPhee <davis.mcphee@elastic.co>
bryce-b
pushed a commit
to bryce-b/kibana
that referenced
this pull request
Oct 30, 2023
…r serialization (elastic#169460) ## Summary Currently we use the full data view spec whenever we serialize fields for a search source with an ad hoc data view, except when otherwise specified (currently only when serializing for alert URLs). As a result, whenever we persist a serialized search source containing an ad hoc data view within a saved object, we end up persisting the entire data view field list as well. For data views that target hundreds or thousands of fields, this can result in huge saved objects that exceed Kibana's max payload size, causing issues like the one seen in elastic#168573. Does it ever make sense to persist the full data view spec now that we have a minimal spec (strips all field list data except popularity counts, custom labels, etc.)? I don't think it does, so this PR updates search source to always use the minimal data view spec for ad hoc data views, limiting the impact of large field lists. Barring a good reason not to do this, I think we should make this the standard behaviour. Fixes elastic#168573. ### 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 - [x] [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: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
awahab07
pushed a commit
to awahab07/kibana
that referenced
this pull request
Oct 31, 2023
…r serialization (elastic#169460) ## Summary Currently we use the full data view spec whenever we serialize fields for a search source with an ad hoc data view, except when otherwise specified (currently only when serializing for alert URLs). As a result, whenever we persist a serialized search source containing an ad hoc data view within a saved object, we end up persisting the entire data view field list as well. For data views that target hundreds or thousands of fields, this can result in huge saved objects that exceed Kibana's max payload size, causing issues like the one seen in elastic#168573. Does it ever make sense to persist the full data view spec now that we have a minimal spec (strips all field list data except popularity counts, custom labels, etc.)? I don't think it does, so this PR updates search source to always use the minimal data view spec for ad hoc data views, limiting the impact of large field lists. Barring a good reason not to do this, I think we should make this the standard behaviour. Fixes elastic#168573. ### 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 - [x] [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: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Currently we use the full data view spec whenever we serialize fields for a search source with an ad hoc data view, except when otherwise specified (currently only when serializing for alert URLs). As a result, whenever we persist a serialized search source containing an ad hoc data view within a saved object, we end up persisting the entire data view field list as well. For data views that target hundreds or thousands of fields, this can result in huge saved objects that exceed Kibana's max payload size, causing issues like the one seen in #168573.
Does it ever make sense to persist the full data view spec now that we have a minimal spec (strips all field list data except popularity counts, custom labels, etc.)? I don't think it does, so this PR updates search source to always use the minimal data view spec for ad hoc data views, limiting the impact of large field lists. Barring a good reason not to do this, I think we should make this the standard behaviour.
Fixes #168573.
Checklist
For maintainers