[Search][Dashboard] Restore searchSessionId from URL #81489

Dosant · 2020-10-22T14:54:57Z

Build on top of #81297
Part of #61738

Summary

Restore searchSessionId from URL
Add searchSessionId support to dashboard's URL generator

Additional info

Didn't do any additional handling to return to background search using back button. I spent some time trying to make it reliable, but I think to make it work in 100% of case it will require additional work and refactoring around dashboard state management. (Unless I missed something obvious) Created a ticket, please see explanation of the problem: [Search][State Management] Restoring background session and back button #82419

How to test

You can build a dashboard URL with fake session id:

${dashboardURL}&searchSessionId=__fake__

Open it.
And you should see __fake__ as session id in inspector.

^^ this is what a functional test does

Checklist

Delete any items that are not applicable to this PR.

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

lukasolson

Can we merge with master to avoid the noise of the previous PR? Thanks!

lukasolson · 2020-10-29T22:43:32Z

src/plugins/kibana_utils/public/history/get_query_params.ts

+import { parse, ParsedQuery } from 'query-string';
+import { Location } from 'history';
+
+export function getQueryParams(location: Location): ParsedQuery {


Looks very similar to the utility in src/plugins/es_ui_shared/public/url/extract_query_params.ts. Not sure the best approach here but it would be nice if we could combine these.

Thanks for point me to it!
I extracted my function from existing history helper (in kibana_utils) so technically I didn't introduce new code.
I decided to leave as is keeping dashboard -> kibana_utils dep and not introducing dashboard -> es_ui dep.

Probably ideally es_ui_shared/public/url/extract_query_params.ts should be removed and utility from kibana_utils should be used instead, but es_ui_shared is not dependent on kibana_utils today, so doesn't worth introducing dependency to maintain for these couple lines.

lukasolson · 2020-10-29T22:45:03Z

src/plugins/kibana_utils/public/history/get_query_params.ts

+import { parse, ParsedQuery } from 'query-string';
+import { Location } from 'history';
+
+export function getQueryParams(location: Location): ParsedQuery {


If we make this generic, can we avoid the cast as string above?

Suggested change

export function getQueryParams(location: Location): ParsedQuery {

export function getQueryParams<T>(location: Location): ParsedQuery<T> {

Looked into it,
ParsedQuery<T> is a weird loose type:

export interface ParsedQuery<T = string> { [key: string]: T | T[] | null; }

that attempt to cover all the query cases and doesn't cover undefined.
T in that case is just type of value in query object.

So I guess possible way to improve with typecasting for us is something like:

export function getQueryParams<T>(location: Location): T { return query as unknown as T }

But I think this is worth then current downcasting on consumer level, because this involves unknown

elasticmachine · 2020-11-02T13:21:40Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lukasolson

LGTM. Added a nit below and just wanted to confirm: Is this the first place we're using actual query parameters instead of stuffing things in the _a or _g (app/global state)? Are there any ramifications of this we need to be aware of?

Also wanted to note that I tested this and things seemed to function as expected (sessionId was properly set and displayed in the inspector and properly cleared when making a change that resulted in a re-fetch, which generated a new sessionId).

lukasolson · 2020-11-02T23:15:05Z

src/plugins/dashboard/public/application/dashboard_app_controller.tsx

@@ -120,6 +126,9 @@ interface UrlParamValues extends Omit<UrlParamsSelectedMap, UrlParams.SHOW_FILTE
  [UrlParams.HIDE_FILTER_BAR]: boolean;
 }

+const getSearchSessionIdFromURL = (history: History): string | undefined =>


Nit: This would probably make more sense inside src/plugins/dashboard/public/url_generator.ts. We could add a test for it there too.

I am keeping it separate because it depends on client side history and url_generator stuff shouldn't depend on it and should be moved to common eventually.

Dosant · 2020-11-03T11:40:02Z

@lukasolson,

Is this the first place we're using actual query parameters instead of stuffing things in the _a or _g (app/global state)? Are there any ramifications of this we need to be aware of?

No, this is not the first place. Dashboard has bunch of other query params options.

Reasons I thought we are better of with separate query param:

From dashboard perspective _a state is stored is stored in the saved object. We don't need to store sessionId in saved object.
Each change of state which is synced with _a is normally synced back to the URL. But we don't want newly created search ids synced back
It seemed that If we try to put sessionId into state management part of dashboard then becomes more complicated in the following way that a single change causes two separate state changes: Change filter -> trigger state change -> trigger new search -> again trigger state change to persist new sessionId

ThomThomson

Tested locally in chrome. Restoring a search session works well, and everything else still works as expected. Tests and code LGTM!

lizozom · 2020-11-03T16:43:01Z

src/plugins/dashboard/public/url_generator.ts

    );
+
+    if (state.searchSessionId) {
+      url = `${url}&${DashboardConstants.SEARCH_SESSION_ID}=${state.searchSessionId}`;


//nit
We don't have any function in our code base to serialize url params?

I think that would be
import { stringify } from 'query-string'; but I guess in this particular case using it would add more code then not using it

(can't benefit from it for _a & _g params here because their helpers already handling everything)

lizozom

LGTM
Added one nit

Dosant · 2020-11-03T17:43:43Z

@elasticmachine merge upstream

kibanamachine · 2020-11-03T19:31:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2d30c78

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`kibanaUtils`	189	190	+1

async chunks size

id	before	after	diff
`dashboard`	197.6KB	198.0KB	+486.0B

page load bundle size

id	before	after	diff
`dashboard`	311.4KB	311.5KB	+154.0B
`kibanaUtils`	147.8KB	148.0KB	+180.0B
total			+334.0B

History

💚 Build #85127 succeeded c7a851a
💚 Build #85108 succeeded 2fe0df8
💚 Build #83662 succeeded a5720f2
💔 Build #83625 failed 8275c3e
💚 Build #83371 succeeded 05a09b2

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

Dosant added Feature:Search Querying infrastructure in Kibana Feature:Dashboard Dashboard related features v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 22, 2020

Dosant requested review from ppisljar, lukasolson and lizozom October 22, 2020 14:55

This was referenced Oct 22, 2020

[Meta] Search Sessions Roadmap #61738

Closed

[Search] Use session service on a dashboard #81297

Merged

[Search] add "restore" to session service #81924

Merged

lukasolson reviewed Oct 29, 2020

View reviewed changes

restore session from URL on a dashboard

c7a851a

Dosant force-pushed the dev/search/3 branch from 8131769 to c7a851a Compare November 2, 2020 12:23

Dosant marked this pull request as ready for review November 2, 2020 13:21

Dosant requested a review from a team as a code owner November 2, 2020 13:21

lukasolson approved these changes Nov 2, 2020

View reviewed changes

Dosant requested a review from ThomThomson November 3, 2020 11:43

Dosant mentioned this pull request Nov 3, 2020

[Search][State Management] Restoring background session and back button #82419

Closed

ThomThomson approved these changes Nov 3, 2020

View reviewed changes

lizozom reviewed Nov 3, 2020

View reviewed changes

lizozom approved these changes Nov 3, 2020

View reviewed changes

Merge branch 'master' into dev/search/3

2d30c78

kibanamachine requested a review from a team as a code owner November 3, 2020 17:43

Dosant merged commit 2c61de1 into elastic:master Nov 4, 2020

Dosant mentioned this pull request Nov 4, 2020

[7.x] [Search][Dashboard] Restore searchSessionId from URL (#81489) #82577

Merged

Dosant added a commit to Dosant/kibana that referenced this pull request Nov 4, 2020

[Search][Dashboard] Restore searchSessionId from URL (elastic#81489)

a08a623

Dosant added a commit that referenced this pull request Nov 4, 2020

[Search][Dashboard] Restore searchSessionId from URL (#81489) (#82577)

fd9b442

Dosant mentioned this pull request Nov 9, 2020

[Search][Discover] Restore searchSessionId from URL #81633

Merged

1 task

Dosant mentioned this pull request Nov 18, 2020

[Search] Send to background #83640

Closed

38 tasks

Dosant mentioned this pull request Dec 3, 2020

[Lens] Encourage user to use smaller data set if searches take too long #84908

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search][Dashboard] Restore searchSessionId from URL #81489

[Search][Dashboard] Restore searchSessionId from URL #81489

Dosant commented Oct 22, 2020 •

edited

Loading

lukasolson left a comment

lukasolson Oct 29, 2020

Dosant Nov 2, 2020

lukasolson Oct 29, 2020

Dosant Nov 2, 2020

elasticmachine commented Nov 2, 2020

lukasolson left a comment •

edited

Loading

lukasolson Nov 2, 2020

Dosant Nov 3, 2020

Dosant commented Nov 3, 2020

ThomThomson left a comment

lizozom Nov 3, 2020

Dosant Nov 3, 2020 •

edited

Loading

lizozom left a comment

Dosant commented Nov 3, 2020

kibanamachine commented Nov 3, 2020

	export function getQueryParams(location: Location): ParsedQuery {
	export function getQueryParams<T>(location: Location): ParsedQuery<T> {

[Search][Dashboard] Restore searchSessionId from URL #81489

[Search][Dashboard] Restore searchSessionId from URL #81489

Conversation

Dosant commented Oct 22, 2020 • edited Loading

Summary

Additional info

How to test

Checklist

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Oct 29, 2020

Choose a reason for hiding this comment

Dosant Nov 2, 2020

Choose a reason for hiding this comment

lukasolson Oct 29, 2020

Choose a reason for hiding this comment

Dosant Nov 2, 2020

Choose a reason for hiding this comment

elasticmachine commented Nov 2, 2020

lukasolson left a comment • edited Loading

Choose a reason for hiding this comment

lukasolson Nov 2, 2020

Choose a reason for hiding this comment

Dosant Nov 3, 2020

Choose a reason for hiding this comment

Dosant commented Nov 3, 2020

ThomThomson left a comment

Choose a reason for hiding this comment

lizozom Nov 3, 2020

Choose a reason for hiding this comment

Dosant Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

Dosant commented Nov 3, 2020

kibanamachine commented Nov 3, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

Dosant commented Oct 22, 2020 •

edited

Loading

lukasolson left a comment •

edited

Loading

Dosant Nov 3, 2020 •

edited

Loading