Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Search Sessions] Fix restoration URL conflicts with global time sync #87488

Merged
merged 9 commits into from
Jan 18, 2021
Merged

[Search Sessions] Fix restoration URL conflicts with global time sync #87488

merged 9 commits into from
Jan 18, 2021

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 6, 2021

Summary

Fixes #87149

See reproduction of the bug:

global_time_sync_720.mov

This is "an easy" fix where apps remove searchSessionId query param from the URL that is going to be used in chrome's nav links.

Long term we should explore fetching app state from SO instead of relying on state in the URL for restoring the search session.

This pr also fixes a bug in kbnUrlTracker where state from initial URL wasn't stored in session storage so it wouldn't appear in nav link.

How to test

This pr is hard to test without #81707 in real world.
Bug to validate that the fix works:

  1. Load discover app using a URL with appended fake &searchSessionId=fake
  2. Discover should loaded with an error
  3. Go to dashboard
  4. Check that chrome link to discover app has your URL but without searchSessionId part

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Dosant Dosant added Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes Feature:StateManagement v7.12.0 labels Jan 6, 2021
@Dosant Dosant marked this pull request as ready for review January 6, 2021 19:38
@Dosant Dosant requested a review from a team January 6, 2021 19:38
@Dosant Dosant requested review from a team as code owners January 6, 2021 19:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@Dosant Dosant requested a review from lizozom January 6, 2021 19:38
@Dosant
Copy link
Contributor Author

Dosant commented Jan 7, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Jan 11, 2021

@elasticmachine merge upstream

lizozom
lizozom approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search Session ID is being properly cleaned from the URL.
Added one question, other than that LGTM!

src/plugins/kibana_utils/public/state_management/url/kbn_url_tracker.ts
storageInstance.setItem(storageKey, activeUrl);
}

function onMountApp() {
unsubscribe();
const historyInstance = history || (getHistory && getHistory()) || createHashHistory();

// set mounted URL as active
if (shouldTrackUrlUpdate(historyInstance.location.hash)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this is related to the current PR, mind elaborating?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted that in the description as:

This pr also fixes a bug in kbnUrlTracker where state from initial URL wasn't stored in session storage so it wouldn't appear in nav link.

I noticed this bug and needed to fix it to make artificial testing flow working:

Bug to validate that the fix works:
Load discover app using a URL with appended fake &searchSessionId=fake
Discover should loaded with an error
Go to dashboard
Check that chrome link to discover app has your URL but without searchSessionId part

In real-world this bug apparently was such a minor thing that no one never noticed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to reproduce this bug without the fix:

  1. Take a kibana URL with some non-default state, open it.
  2. Change app using nav links
  3. Get back to the first app using the nav link.

Broken behavior: you will land into the default state
Expected (new) behaviour: you should restore your previous state

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this comment! 🙏

@Dosant
Copy link
Contributor Author

Dosant commented Jan 12, 2021

@elasticmachine merge upstream

ThomThomson
ThomThomson approved these changes Jan 12, 2021
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested locally, and was able to see the the discover link lost the fake searchSessionId from the URL.

@Dosant
Copy link
Contributor Author

Dosant commented Jan 14, 2021

@elasticmachine merge upstream

@Dosant Dosant added Feature:Dashboard Dashboard related features Feature:Discover Discover Application labels Jan 14, 2021
@Dosant Dosant requested a review from kertal January 14, 2021 18:08
@Dosant
Copy link
Contributor Author

Dosant commented Jan 15, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Jan 15, 2021

ACK, will review

kertal
kertal approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, code owner review, had troubles to start my checked out branch, so I couldn't test, but I think it's a local problem, not related to the PR:
Bildschirmfoto 2021-01-15 um 16 51 20

@Dosant
Copy link
Contributor Author

Dosant commented Jan 18, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Jan 18, 2021

@kertal, we renamed a feature and feature flag. Now it is:

xpack.data_enhanced.search.sessions.enabled: true

Sorry for confusion.

@kibanamachine
Copy link
Contributor

💚 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
discover 511.3KB 511.3KB +2.0B

Page load bundle

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

id before after diff
dashboard 333.8KB 334.2KB +321.0B
discover 82.1KB 82.4KB +226.0B
kibanaUtils 150.4KB 150.8KB +465.0B
total +1012.0B

History

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

@Dosant Dosant merged commit d55ff81 into elastic:master Jan 18, 2021
Dosant added a commit to Dosant/kibana that referenced this pull request Jan 18, 2021
@Dosant
[Search Sessions] Fix restoration URL conflicts with global time sync (…
0d324e9 
…elastic#87488)
@Dosant Dosant mentioned this pull request Jan 18, 2021
Merged
Dosant added a commit that referenced this pull request Jan 18, 2021
@Dosant
[Search Sessions] Fix restoration URL conflicts with global time sync (…
8a4a907 
…#87488) (#88600)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizozom lizozom lizozom approved these changes

@ThomThomson ThomThomson ThomThomson approved these changes

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
Feature:Dashboard Dashboard related features Feature:Discover Discover Application Feature:Search Querying infrastructure in Kibana Feature:StateManagement release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Search][Send to background] Restoration URL conflicts with global time sync with chrome's URLs
6 participants
@Dosant @elasticmachine @kertal @kibanamachine @lizozom @ThomThomson