-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Search] Use session service on a dashboard #81297
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
Conversation
| it('delayed should load', async () => { | ||
| await PageObjects.common.navigateToApp('dashboard'); | ||
| await PageObjects.dashboard.gotoDashboardEditMode('Delayed 5s'); | ||
| await PageObjects.dashboard.loadSavedDashboard('Delayed 5s'); |
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.
@lukasolson, I doubt that gotoDashboardEditMode was intentional, so I changed it to loadSavedDashboard
loadSavedDashboard is faster because we don't have to wait for initial load and then for a 2nd load after switching to edit mode.
| $scope.model.query = queryStringManager.getQuery(); | ||
| dashboardStateManager.applyFilters($scope.model.query, $scope.model.filters); | ||
| if (dashboardContainer) { | ||
| dashboardContainer.updateInput({ |
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 is not needed. After removing all dashboard container updates are handled in single place right now.
Looking forward to this file being refactored.
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 in the middle of completely re-doing this file as part of deangularization. It will be a functional component - I may end up pinging you for some input at some point.
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.
@ThomThomson, that's great news 👍 Will be glad to chime in.
From perspective of this pr important bit is that we are calling dashboardContainer.updateInput in a single place and we assume this is when we want to start a new search session.
lukasolson
left a comment
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, verified that the searchSessionId seems to be set properly across visualizations that use SearchSource. It's updated properly when the filter/query/time range is updated and a new query is sent out.
| </EuiText> | ||
| )} | ||
|
|
||
| {this.state.request && this.state.request.searchSessionId && ( |
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.
Is this only for the functional tests, or do you think there is value in showing this parameter to the user?
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 think it would also be useful for debugging and support purposes
ThomThomson
left a comment
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.
Code & behaviours LGTM.
Tested locally in chrome by making changes in the dashboard app and ensuring that every embeddable uses the same sessionId, and that the sessionId changes correctly with every change.
|
@elasticmachine merge upstream |
| timeRange?: TimeRange; | ||
| query?: Query; | ||
| filters?: Filter[]; | ||
| searchSessionId?: string; |
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 is a major nit, but do we really want to have the name be so specific?
I think sessions can and will be used for more than search.
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 think sessionId is way too vague for dashboards / embeddable level, but OK on search level.
sessionId on dashboard could mean anything:
- user authentication session
- dashboard page session (created once when user loaded a dashboard)
- dashboard app session (to track drill-downs between dashboards without navigating away from a dashboard app)
- and the search / fetch session.
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.
Agreed, if this terminology is for using in solutions :)
lizozom
left a comment
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.
Looks good.
Added a couple of suggestions
| query: this.input.query, | ||
| filters: this.input.filters, | ||
| }, | ||
| searchSessionId: this.input.searchSessionId, |
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 we pass in the searchSessionId from the visualize app, would that enable sessions for that app too?
(Not in this PR)
| return searchSessionId; | ||
| }; | ||
|
|
||
| const panel1SessionId1 = await getSearchSessionIdByPanel('Sum of Bytes by Extension'); |
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.
Would it make sense to add tests to test/plugin_functional/test_suites/data_plugin/session.ts to make sure that sessions are being created once per load \ reload?
It was what uncovered the problematic behaviors in Dashboard in the first place.
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 will take a look 👍
but would like to do it in separate pr,
really want to unblock a bunch of follow up prs with merging this :(
|
@elasticmachine merge upstream |
flash1293
left a comment
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.
Lens and saved searches LGTM - it seems like the search session id is passed through correctly. It seems like Discover is fetching twice on changing the time range, but that happens on master as well. I will open a separate issue for this.
💔 Build Failed
Failed CI Steps
Test FailuresFirefox UI Functional Tests.test/functional/apps/dashboard/dashboard_save·js.dashboard app using legacy data dashboard save warns on duplicate name for new dashboardStandard OutStack TraceFirefox UI Functional Tests.test/functional/apps/dashboard/index·js.dashboard app using legacy data "after all" hook: unloadLogstash in "using legacy data"Standard OutStack TraceChrome UI Functional Tests.test/functional/apps/visualize/index·ts.visualize app "before all" hook in "visualize app"Standard OutStack Traceand 7 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |

Release note
Adds Search Sessions support to Kibana Dashboards
Summary
Follow up on #76889
Prev version of this pr with some comments: #81192
Part of #61738
Next step: restore searchSessionId from the URL: #81489
Some context for reviewers
This is part of client side background search. To save or restore a background search, searches will be grouped under a session. We implemented session creation on every search in discover in #76889. This pr creates a search session on every dashboard container state change and propagates
searchSessionIdto embeddable -> search infrastructure.Implementation notes:
Maps: TODO: probably better done separatelyAnything else?3. Added
sessionIddisplayed in inspector. Used it it in a functional test. I also think it would be useful for debug and support purposes.This is how it looks:
Functional test:
Added a test that checks:
How to test
searchSessionIdchanges on different state changes. You can use inspector.Open questions / edge cases to iterate over later
Next steps
searchSessionIdwith dashboardURL [Search][Discover] Restore searchSessionId from URL #81633Checklist
Delete any items that are not applicable to this PR.