-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Saved Queries] Improve saved query management #170599
[Saved Queries] Improve saved query management #170599
Conversation
@davismcphee Thanks for working on improving this UI! Here are some design comments:
Here are the two alternatives I'm proposing: I think I lean towards option 1 because we usually do something like option 2 when users can select multiple items in a list and can apply bulk actions. |
@andreadelrio I like the option 1 👍 |
b967bee
to
6bcb417
Compare
@andreadelrio Thanks for the design review! I also prefer option 1, and am working on implementing the suggested changes 👍 |
/ci |
633b213
to
74410a2
Compare
/ci |
2 similar comments
/ci |
/ci |
.kbnSavedQueryManagement__text { | ||
padding: $euiSizeM $euiSizeM ($euiSizeM / 2) $euiSizeM; | ||
.euiSelectableListItem { | ||
border-bottom: 1px solid darken($euiColorLightestShade, 2%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need this line. If it's not supported natively from EUI I hesitate to add it. It creates a double border in the footer when there's only one page. I see we either remove it altogether or include it only when there's more than one page. What do you think @davismcphee ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the fewer overrides the better, so I'm in favour of removing it altogether. Dropped it in this commit: 17c60c3.
One other thing I overrode to try to better match the designs is removing the blue background on the focused list entry. In general I think this looks better, but now the only indicator of the focused field for keyboard users is the text underline. Do you think this is fine, or should I bring back the blue background/add another indicator of some sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davismcphee I say you bring back the blue background as the few overrides the better lifestyle recommends 😉.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! acf4cf0
cf6b635
to
d6ee700
Compare
/ci |
page: 0, | ||
perPage: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Minor performance optimization to no longer request documents when we only need the total
.
sortField: search ? undefined : 'titleKeyword', | ||
sortOrder: search ? undefined : 'asc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ If no search term is passed, we sort by title alphabetically. If a search term is passed, we use the default score based sorting.
@@ -178,6 +178,7 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void { | |||
} | |||
); | |||
|
|||
// TODO: We no longer support fetching all saved queries, so this route should be removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the /_all
endpoint to only load the first 100 saved queries which addresses the concern of loading too many into memory at once, but should we just fully remove the endpoint instead? It's an undocumented internal endpoint that has no remaining consumers in Kibana, but I'm not sure how we usually approach endpoint removals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be all for removing the endpoint altogether. Like you mentioned it's not documented as a public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up dropping the endpoint entirely here: e48acf9. Although now I'm wondering if this change is safe for ZDT upgrades. @elastic/kibana-core is it safe for us to remove this internal endpoint in the same release we stop using it, or could it be an issue for upgrades?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing an endpoint that isn't used internally should be ok if you're sure we don't have external services consuming the endpoint (fleet, agent, etc).
3rd party integrations and external consumers might be an issue but, as you mentioned, the endpoint is internal and isn't documented.
You could rather deprecate the endpoint with a short deprecation timeframe and see if there are any concerns/problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main concern with ZDT upgrades is a scenario where the UI uses an API, then when we roll an update to remove the API a user could encounter breakage if they still have cached the old UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core recommends the safest way forward for removing an internal endpoint is to (1) remove all usage, (2) wait for 2 (serverless) releases to allow for rollbacks, and then (3) remove the API. The approximate timeframe is 3-4 weeks, depending on when this commit lands.
@@ -91,11 +88,11 @@ const AddFilterPopoverComponent = React.memo(function AddFilterPopover({ | |||
panelPaddingSize="none" | |||
panelProps={{ | |||
'data-test-subj': 'addFilterPopover', | |||
css: popoverDragAndDropCss(euiTheme), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ EUI now supports this by default through the hasDragDrop
prop.
@@ -437,8 +445,7 @@ export function QueryBarMenuPanels({ | |||
{ | |||
id: 1, | |||
title: strings.getSaveCurrentFilterSetLabel(), | |||
disabled: !Boolean(showSaveQuery), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Removed because disabled
doesn't exist on EuiContextMenuPanelItemDescriptor
.
@@ -509,11 +516,10 @@ export function QueryBarMenuPanels({ | |||
}, | |||
{ | |||
id: 4, | |||
title: strings.getLoadCurrentFilterSetLabel(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Removed because title
doesn't exist on EuiContextMenuPanelItemDescriptor
.
/ci |
5ba975a
to
119ca6d
Compare
/ci |
// If the user tries to load the same saved query that is already loaded, | ||
// we will receive the same object reference which was previously frozen | ||
// by Redux Toolkit. `filterManager.setFilters` will then try to modify | ||
// the query's filters, which will throw an error. To avoid this, we need | ||
// to clone the filters before passing them to `filterManager.setFilters`. | ||
const savedQueryFilters = cloneDeep(newSavedQuery.attributes.filters || []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This bug was already present in main
, but was never caught in any functional tests until I added this check to test/functional/services/saved_query_management_component.ts
, which caused it to start triggering test failures: https://github.com/elastic/kibana/blob/119ca6d36d044f3ec0997a409fc119532ecc448e/test/functional/services/saved_query_management_component.ts#L90-L92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch!
/ci |
1 similar comment
/ci |
private hasFiltersOrQuery() { | ||
const hasFilters = Boolean(this.props.filters && this.props.filters.length > 0); | ||
const hasQuery = Boolean( | ||
this.state.query && isOfQueryType(this.state.query) && this.state.query.query | ||
); | ||
return hasFilters || hasQuery; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Dropped this because it was only being passed to SavedQueryManagementList
which wasn't using it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for all the work you've done to improve Saved queries UI and fix bugs in it!
The only remaining concern from my side is that React shows lots of warnings when opening "Load query" panel.
const SAVED_QUERY_PAGE_SIZE = 5; | ||
const SAVED_QUERY_SEARCH_DEBOUNCE = 500; | ||
|
||
export const SavedQueryManagementList = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get them every time I open "Load queries" panel. Can you try in Chrome?
cancelPendingListingRequest.current(); | ||
setSavedQueries( | ||
savedQueries.filter((currentSavedQuery) => currentSavedQuery.id !== savedQueryId) | ||
setTotalQueryCount((currentTotalQueryCount) => max([0, currentTotalQueryCount - 1])!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setTotalQueryCount((currentTotalQueryCount) => max([0, currentTotalQueryCount - 1])!); | |
setTotalQueryCount((currentTotalQueryCount) => Math.max(0, currentTotalQueryCount - 1)!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much coding is making me forgetful 🙂 The standard lib is always preferable, changed it here: 032a918.
)); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for improving tests too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Thanks for seeing this through! So much goodness inside.
I did find another case where searching for the exact title doesn't work - something like "what's up". It turns out the standard analyzer breaks this up into two tokens ["what's", "up"]
and not three (["what", "s", "up"]
). We could fix this by changing the regex a little bit, but I don't want to delay this PR and all its improvements with making everything work exactly as the default analyzers.
…, which is called within useMemo, and leads to nested hook calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did find another case where searching for the exact title doesn't work - something like "what's up".
Welp, good catch 😢 I think this could be worth addressing (maybe someone's possessive about their queries 😁), but maybe we can work together on a more robust regex as followup since we can safely backport it after FF as a bug fix... Then beg for DSL support as followup-followup.
const SAVED_QUERY_PAGE_SIZE = 5; | ||
const SAVED_QUERY_SEARCH_DEBOUNCE = 500; | ||
|
||
export const SavedQueryManagementList = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was trickier to address than anticipated due to the EUI pretty duration utils. It required a workaround to address, but I think it's better than all the errors and is at least consistent with what we do elsewhere in Kibana: 94e9361.
cancelPendingListingRequest.current(); | ||
setSavedQueries( | ||
savedQueries.filter((currentSavedQuery) => currentSavedQuery.id !== savedQueryId) | ||
setTotalQueryCount((currentTotalQueryCount) => max([0, currentTotalQueryCount - 1])!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much coding is making me forgetful 🙂 The standard lib is always preferable, changed it here: 032a918.
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great, thanx Davis. Very nice enhancement. I just have one comment, this 1 selected seems a bit weird. Can we remove it before merging? I saw that Andrea is also ok with it. I am approving as I assume you are going to address it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks, Davis!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change in x-pack/plugins/security_solution/public/common/lib/kibana/kibana_react.mock.ts
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes! Investigations changes 🚀 💯
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
Just an FYI that I've created a followup issue here to address some of the concerns that didn't make it into this PR: #176730. Please update or comment on the followup issue if there are any concerns I missed. |
## Summary This PR introduces a number of changes and improvements to saved query management: - Add server side pagination (5 queries per page) and search functionality to the "Load query" list, which improves UX and performance by no longer requesting all queries at once. - Redesign the "Load query" list to improve the UX and a11y, making it possible for keyboard users to effectively navigate the list and load/delete queries. - Add an "Active" badge to the "Load query" list to indicate which list entry represents the currently loaded query, and hoist the entry to the top of the first page for better visibility when no search term exists. - Deprecate the saved query `/_all` endpoint and update it to return only the first 100 queries instead of loading them all into memory at once. - Add a new `titleKeyword` field to the saved query SO, which allows sorting queries alphabetically by title when displaying them in the "Load query" list. - Improve the performance of the "has saved queries" check when Unified Search is mounted to no longer request actual queries, and instead just request the count. - Update the saved query duplicate title check to no longer rely on fetching all queries at once, and instead asynchronously check for duplicates by title on save. - Add server side duplicate title validation to the create and update saved query endpoints. - Various small fixes and cleanups throughout saved query management. https://github.com/elastic/kibana/assets/25592674/43328aea-0f7b-4b7a-a5fb-e33ed822f317 Resolves elastic#172044. Resolves elastic#176427. ## Testing To generate saved queries for testing, run the script below and replace `{KIBANA_REQUEST_COOKIE}` with the cookie header value from an API request of a Kibana user with an active session: ```shell for i in {1..100}; do curl 'http://localhost:5601/internal/saved_query/_create' \ -H 'Accept: */*' \ -H 'Accept-Language: en-US,en;q=0.9,az;q=0.8,es;q=0.7' \ -H 'Cache-Control: no-cache' \ -H 'Connection: keep-alive' \ -H 'Content-Type: application/json' \ -H 'Cookie: {KIBANA_REQUEST_COOKIE}' \ -H 'elastic-api-version: 1' \ -H 'kbn-build-number: 9007199254740991' \ -H 'kbn-version: 8.13.0' \ -H 'x-elastic-internal-origin: Kibana' \ --data-raw '{"title":"query '"$(echo $(($i - 1)) | tr '[0-9]' '[a-j]')"'","description":"","query":{"query":"bytes > 500","language":"kuery"},"filters":[]}' \ --compressed; done ``` ### Checklist - [x] 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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] 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) - [x] 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)) - [x] 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]> Co-authored-by: Stratoula Kalafateli <[email protected]>
## Summary This PR introduces a number of changes and improvements to saved query management: - Add server side pagination (5 queries per page) and search functionality to the "Load query" list, which improves UX and performance by no longer requesting all queries at once. - Redesign the "Load query" list to improve the UX and a11y, making it possible for keyboard users to effectively navigate the list and load/delete queries. - Add an "Active" badge to the "Load query" list to indicate which list entry represents the currently loaded query, and hoist the entry to the top of the first page for better visibility when no search term exists. - Deprecate the saved query `/_all` endpoint and update it to return only the first 100 queries instead of loading them all into memory at once. - Add a new `titleKeyword` field to the saved query SO, which allows sorting queries alphabetically by title when displaying them in the "Load query" list. - Improve the performance of the "has saved queries" check when Unified Search is mounted to no longer request actual queries, and instead just request the count. - Update the saved query duplicate title check to no longer rely on fetching all queries at once, and instead asynchronously check for duplicates by title on save. - Add server side duplicate title validation to the create and update saved query endpoints. - Various small fixes and cleanups throughout saved query management. https://github.com/elastic/kibana/assets/25592674/43328aea-0f7b-4b7a-a5fb-e33ed822f317 Resolves elastic#172044. Resolves elastic#176427. ## Testing To generate saved queries for testing, run the script below and replace `{KIBANA_REQUEST_COOKIE}` with the cookie header value from an API request of a Kibana user with an active session: ```shell for i in {1..100}; do curl 'http://localhost:5601/internal/saved_query/_create' \ -H 'Accept: */*' \ -H 'Accept-Language: en-US,en;q=0.9,az;q=0.8,es;q=0.7' \ -H 'Cache-Control: no-cache' \ -H 'Connection: keep-alive' \ -H 'Content-Type: application/json' \ -H 'Cookie: {KIBANA_REQUEST_COOKIE}' \ -H 'elastic-api-version: 1' \ -H 'kbn-build-number: 9007199254740991' \ -H 'kbn-version: 8.13.0' \ -H 'x-elastic-internal-origin: Kibana' \ --data-raw '{"title":"query '"$(echo $(($i - 1)) | tr '[0-9]' '[a-j]')"'","description":"","query":{"query":"bytes > 500","language":"kuery"},"filters":[]}' \ --compressed; done ``` ### Checklist - [x] 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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] 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) - [x] 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)) - [x] 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]> Co-authored-by: Stratoula Kalafateli <[email protected]>
Summary
This PR introduces a number of changes and improvements to saved query management:
/_all
endpoint and update it to return only the first 100 queries instead of loading them all into memory at once.titleKeyword
field to the saved query SO, which allows sorting queries alphabetically by title when displaying them in the "Load query" list.saved_query_management.mp4
Resolves #172044.
Resolves #176427.
Testing
To generate saved queries for testing, run the script below and replace
{KIBANA_REQUEST_COOKIE}
with the cookie header value from an API request of a Kibana user with an active session:Checklist
For maintainers