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.

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

[Search Sessions] omit searchSessionId from the initialState, explicitly pause refreshInterval in restoreState #88650

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 19, 2021

Summary

Closes #88567 #88568

This pr closes two-issue around restoring a search session in Discover and Dashboard:

1. searchSessionId should be omitted in initialState

We save two states into a search session saved object:

  • restoreState
  • initialState
    These states are used when navigating from management to an App. By default restoreState is used to restore a session.
    We use initialState when we know there is no session to restore, for example, in case the search session is in the ERROR or EXPIRED state. initialState should not contain searchSessionId, otherwise search requests will error out.

How to test

When saving a session there should be no searchSessionId inside initialState in a request save request payload.

To test end-to-end needs #81707. In case the search is is in the ERROR or EXPIRED state, navigating to it should just restart a search.

2. refresh interval should be explicitly paused when generating restoreState for dashboard.

If there is no refreshInterval in the URL dashboard picks up whatever is saved in the dashboard SO.

@Dosant Dosant changed the title [Search Sessions] omit searchSessionId in initialState [Search Sessions] omit searchSessionId from the initialState Jan 19, 2021
@Dosant Dosant added Feature:Dashboard Dashboard related features Feature:Discover Discover Application Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 Team:AppServices labels Jan 19, 2021
@Dosant Dosant marked this pull request as ready for review January 19, 2021 11:04
@Dosant Dosant requested a review from a team January 19, 2021 11:04
@Dosant Dosant requested a review from a team as a code owner January 19, 2021 11:04
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant requested review from kertal and ThomThomson January 19, 2021 11:05
@Dosant Dosant changed the title [Search Sessions] omit searchSessionId from the initialState [Search Sessions] omit searchSessionId from the initialState, explicitly pause refreshInterval in restoreState Jan 20, 2021
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.

Kibana App owned code LGTM, tested locally in Chrome, searchSession at intialState is successfully omitted in Discover 👍

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.

Presentation team changes LGTM. Code only review, but will look into testing the full set of features with #81707.

@Dosant
Copy link
Contributor Author

Dosant commented Jan 25, 2021

@elasticmachine merge upstream

@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
dashboard 200.3KB 200.4KB +156.0B
discover 511.3KB 511.3KB +82.0B
total +238.0B

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Discover Discover Application Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SearchSessions] initialState should not contain the searchSessionId
5 participants