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][Discover] Send to background integration improvements and fixes #87311

Merged
merged 9 commits into from
Jan 12, 2021

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 5, 2021

Summary

Closes #87148

This pr makes couple improves to send to background integration in Discover:

  1. Fixes discover URL generation in case there is a saved search. (Unfortunately to properly tests needs [Data/Search Sessions] Management UI #81707)
  2. Uses saved search's name as search session name if available. Otherwise fallbacks to Discover (Unfortunately to properly tests needs [Data/Search Sessions] Management UI #81707. Also can be tested by inspecting network request that saves a session)
  3. Moves discover session tests into separate test suite. This was done for dashboard tests in scope of [Send To Background UI] Isolate functional test for wip feature #84833 but we forgot to also move discover tests
  4. Add a check in ftr that there is an error in case incorrect search session id is used during restoration

Checklist

@Dosant Dosant added Feature:Search Querying infrastructure in Kibana Feature:Discover Discover Application v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 5, 2021
@Dosant Dosant changed the title Dev/search/discover restore fix [Search][Discover] Send to background integration improvements and fixes Jan 5, 2021
@Dosant Dosant marked this pull request as ready for review January 5, 2021 18:44
@Dosant Dosant requested a review from a team January 5, 2021 18:44
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant requested review from kertal and a team January 5, 2021 18:44
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.

Tested and LGTM, but please consider adding a timestamp to the session name.

@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

@kertal
Copy link
Member

kertal commented Jan 11, 2021

ACK, will review

@Dosant Dosant requested a review from kertal January 12, 2021 11:09
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, was testing linking back via the management UI when you're using a saved search the link is correct, but there seems to be an error restoring the session, I think this is unrelated to this PR, so I'm approving this, maybe you could check before merging. many thx for the refactoring and adding tests
Bildschirmfoto 2021-01-12 um 12 56 45

@Dosant
Copy link
Contributor Author

Dosant commented Jan 12, 2021

@kertal, could this be related to the relative time range?
Support for restoring session with relative time range is not in master yet: #84405

@kertal
Copy link
Member

kertal commented Jan 12, 2021

@kertal, could this be related to the relative time range?
Support for restoring session with relative time range is not in master yet: #84405

oh, yes, thank, works with absolute time ranges :)

@kertal kertal self-requested a review January 12, 2021 13:18
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, tested locally, works with absolute time ranges, finally approving this, not pretending to do so :)

@Dosant
Copy link
Contributor Author

Dosant commented Jan 12, 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
discover 502.0KB 502.3KB +312.0B

Page load bundle

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

id before after diff
discover 81.7KB 81.7KB +16.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: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.

[Bug][Discover][Search][Send to Background] Can't restore search session with saved search
5 participants