[data.search.session] Use locators instead of URL generators by lukasolson · Pull Request #115681 · elastic/kibana

lukasolson · 2021-10-19T23:05:30Z

Summary

Resolves #85126.

Migrates the search session service from URL generators to locators.

To do:

Migration of saved object
Fix/update tests
Update docs

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests 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)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dosant · 2021-10-20T09:50:02Z

x-pack/plugins/data_enhanced/public/search/sessions_mgmt/lib/api.ts

  try {
-    url = await urls.getUrlGenerator(urlGeneratorId).createUrl(state);
+    const locator = locators.get(locatorId);
+    return locator?.getRedirectUrl(state);


I think we should leave it like this in this migration pr, but I think later we should use locator.navigate instead. I assume this will solve the potential problem of "too long URLs"

👍 I had this same thought... I just did this for now since it was simpler but is there any reason we shouldn't use navigate in this PR?

is there any reason we shouldn't use navigate in this PR?

I think mostly because this will require destination apps to be able to handle incoming state: now they only can extract the initial state from the URL, but they will have to extract it from location.state.
So as this will require more changes and more testing, I suggest we focus on migration and use an old URL based approach for now

Noticed this bug with redirect endpoint - #116313. I think it is fine to fix separately

Dosant · 2021-10-20T14:57:46Z

Could you please also update relevant docs: https://github.com/elastic/kibana/blob/master/dev_docs/tutorials/data/search.mdx

ThomThomson

Code only review - nice change! The more we use locators the better! Presentation team changes LGTM

lukasolson · 2021-10-25T21:56:42Z

@elasticmachine merge upstream

majagrubic

Discover changes approved!

Dosant

Tested search sessions in Discover/Dashboard. Didn't notice any issues.

I also tested an upgrade from 7.16 to 8.0. all seems good 👍
nice work

Dosant · 2021-10-26T15:18:19Z

x-pack/plugins/data_enhanced/server/saved_objects/search_session_migration.test.ts

  });
 });
+
+describe('7.14.0 -> 7.18.0', () => {


I am confused :)

Dosant · 2021-10-26T15:31:23Z

x-pack/plugins/data_enhanced/public/search/sessions_mgmt/lib/api.ts

  try {
-    url = await urls.getUrlGenerator(urlGeneratorId).createUrl(state);
+    const locator = locators.get(locatorId);
+    return locator?.getRedirectUrl(state);


Noticed this bug with redirect endpoint - #116313. I think it is fine to fix separately

…arch-session-locators

kibanamachine · 2021-10-26T18:45:17Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	143.3KB	143.3KB	-11.0B
`dataEnhanced`	46.6KB	46.6KB	-5.0B
`discover`	340.0KB	340.0KB	-6.0B
total			-22.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	468.3KB	468.3KB	-32.0B
`discover`	22.7KB	22.9KB	+193.0B
total			+161.0B

History

💚 Build #1626 succeeded b376c30
💔 Build #1591 failed e6eeb01
💔 Build #1158 failed dd1cde8
💔 Build #1157 failed 88cf1ca
💔 Build #1145 failed f6c2123
💔 Build #1141 failed 271e21c

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

cc @lukasolson

kibanamachine · 2021-10-26T19:30:37Z

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 115681

kibanamachine · 2021-10-28T19:33:22Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115681 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-10-29T19:43:22Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115681 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-11-01T19:49:48Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115681 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-11-02T19:59:49Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115681 or prevent reminders by adding the backport:skip label.

[data.search.session] Use locators instead of URL generators

e53e804

lukasolson added v8.0.0 Team:AppServicesSv release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Feature:Search Sessions labels Oct 19, 2021

lukasolson self-assigned this Oct 19, 2021

Dosant reviewed Oct 20, 2021

View reviewed changes

Dosant mentioned this pull request Oct 20, 2021

[Search Sessions] "Your search is still running" popup is too flaky #106074

Closed

lukasolson added 4 commits October 21, 2021 14:10

Merge branch 'master' into search-session-locators

4a7075f

Add migration

271e21c

Fix/update tests

f6c2123

Merge branch 'master' into search-session-locators

88cf1ca

lukasolson marked this pull request as ready for review October 21, 2021 22:54

lukasolson requested review from a team as code owners October 21, 2021 22:54

lukasolson changed the title ~~[skip-ci] [data.search.session] Use locators instead of URL generators~~ [data.search.session] Use locators instead of URL generators Oct 21, 2021

Fix types

dd1cde8

ThomThomson approved these changes Oct 25, 2021

View reviewed changes

lukasolson added 3 commits October 25, 2021 12:56

Fix test setup

201b7fa

Update docs

db0116f

Merge branch 'master' into search-session-locators

e6eeb01

lukasolson requested a review from Dosant October 25, 2021 20:17

Merge branch 'master' into search-session-locators

b376c30

majagrubic approved these changes Oct 26, 2021

View reviewed changes

Dosant mentioned this pull request Oct 26, 2021

[UrlService] Back button with redirect endpoint doesn't work #116313

Closed

Dosant approved these changes Oct 26, 2021

View reviewed changes

lukasolson added 2 commits October 26, 2021 09:21

Fix version :)

8de1a7a

Merge remote-tracking branch 'origin/search-session-locators' into se…

747845c

…arch-session-locators

lukasolson merged commit eb74b86 into elastic:master Oct 26, 2021

lukasolson removed the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 26, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2021

lukasolson added backport:skip This PR does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Nov 1, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 2, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 2, 2021

qn895 mentioned this pull request Nov 3, 2021

[ML] Fix data visualizer grid failing if one of the fields failed and not updating when refreshed #115644

Merged

9 tasks

This was referenced Jan 11, 2022

Delete courier:batchSearches from settings during 8.0 migration #122662

Closed

Add a warning about removed courier:batchSearches to 8.0 upgrade assistance #122663

Closed

afharo mentioned this pull request Jan 12, 2022

[Search Sessions] Links always redirect to the default space #122741

Closed

Conversation

lukasolson commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Risk Matrix

For maintainers

Uh oh!

Dosant Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

lukasolson Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Dosant Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Dosant Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

Dosant commented Oct 20, 2021

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

lukasolson commented Oct 25, 2021

Uh oh!

majagrubic left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dosant Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

lukasolson Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

Dosant Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 26, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

Uh oh!

kibanamachine commented Oct 26, 2021

💔 Backport failed

Uh oh!

kibanamachine commented Oct 28, 2021

Uh oh!

kibanamachine commented Oct 29, 2021

Uh oh!

kibanamachine commented Nov 1, 2021

Uh oh!

kibanamachine commented Nov 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lukasolson commented Oct 19, 2021 •

edited

Loading

Dosant left a comment •

edited

Loading