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] Client side session service #76889

Merged
merged 98 commits into from
Oct 16, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Sep 7, 2020

Summary

This PR introduces the frontend session management service as part of #61738, and integrates it into the discover, by initializing a session before fetching fresh data from the server.

This PR also uses the session service, to show the timeout error once per session, instead of using a debounce.

Dev Docs

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom added Feature:Search Querying infrastructure in Kibana Team:AppArch v7.10.0 v8.0.0 labels Sep 7, 2020
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

I think we need to have a discussion about whether or not we decide to make the sessionService stateful, or to expect to pass the sessionId everywhere. Right now we're doing a mix of both (e.g. in searchSource) and I think we should make a choice to do one or the other.

Also, I'm not totally sure I understand all of the changes to dashboard app controller. Maybe we can chat about those.

src/plugins/data/public/search/search_interceptor.ts Outdated Show resolved Hide resolved
@lizozom
Copy link
Contributor Author

lizozom commented Oct 13, 2020

@Dosant I agree about the impact of removing the debounce. It is one of the reasons for not trying to merge this into 7.10. I want to have a separate PR for the visualize and dashboard integrations, as they are not as simple as this one and require a significant amount of functional tests.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

I made a quick pass through this, works well, but I haven't tested it in depth. Overall, the changes make sense, no major comments there.

Some minor questions below.

@@ -53,4 +53,22 @@ describe('tapOnce', () => {
},
});
});

test('fires once even when there is an error', async (done) => {
Copy link
Contributor

@majagrubic majagrubic Oct 14, 2020

Choose a reason for hiding this comment

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

Great for adding a test case!

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Recent code change LGTM, didn't retest
Thanks for looking into clear() thing

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
data 568 576 +8

async chunks size

id before after diff
dashboard 221.4KB 221.3KB -122.0B
discover 439.4KB 439.5KB +90.0B
total -32.0B

distributable file count

id before after diff
default 48495 48497 +2
oss 29211 29213 +2

page load bundle size

id before after diff
data 1.1MB 1.1MB +10.5KB
dataEnhanced 34.6KB 34.6KB -24.0B
total +10.5KB

History

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

@lizozom lizozom merged commit a1831a6 into elastic:master Oct 16, 2020
lizozom added a commit to lizozom/kibana that referenced this pull request Oct 16, 2020
* Add a session service and use it in discover and dashboard

* check unefined

* Start session in visualize

* Fix tests

* docs

* OSS error alignemnt

* Adjust error messages in xpack

* Add getErrorMessage

* Use showError in vizualize
Add original error to expression exception

* Cleanup

* ts, doc and i18n fixes

* Fix jest tests

* Fix functional test

* functional test

* ts

* Update functional tests

* Add unit tests to interceptor and timeout error

* expose toasts test function

* doc

* typos

* lint

* Cleanup

* review 1

* Code review

* doc

* doc fix

* visualization type fix

* fix jest

* Fix xpack functional test

* fix xpack test

* code review

* Add tracking methods to session service

* remove chromium

* Fix ts and jest tests

* jest + docs

* ts fix

* siem test

* Use session service to show a timeout notification per session + more unit tests

* ts and docs

* Remove session service from search source (not needed)

* Code review

* ts

* Single active session in FE session service

* Cleanup

* Don't integrate with dashboard \ visualize
Add functional tests for session toast plugin

* Typescript

* ts

* Improve functional tests

* es

* simplify filter test

* wait until loadedw

* filter test

* delete crypto for now

* Select the correct index 🤦

* timerange

* Adjust functional test logic

* improved test format @Dosant

* Handle exceptions

* Don't close sessions automatically, warn instead

* jest

* Adjust functional test

* Remove unused code

* delete export

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
lizozom added a commit that referenced this pull request Oct 16, 2020
* Add a session service and use it in discover and dashboard

* check unefined

* Start session in visualize

* Fix tests

* docs

* OSS error alignemnt

* Adjust error messages in xpack

* Add getErrorMessage

* Use showError in vizualize
Add original error to expression exception

* Cleanup

* ts, doc and i18n fixes

* Fix jest tests

* Fix functional test

* functional test

* ts

* Update functional tests

* Add unit tests to interceptor and timeout error

* expose toasts test function

* doc

* typos

* lint

* Cleanup

* review 1

* Code review

* doc

* doc fix

* visualization type fix

* fix jest

* Fix xpack functional test

* fix xpack test

* code review

* Add tracking methods to session service

* remove chromium

* Fix ts and jest tests

* jest + docs

* ts fix

* siem test

* Use session service to show a timeout notification per session + more unit tests

* ts and docs

* Remove session service from search source (not needed)

* Code review

* ts

* Single active session in FE session service

* Cleanup

* Don't integrate with dashboard \ visualize
Add functional tests for session toast plugin

* Typescript

* ts

* Improve functional tests

* es

* simplify filter test

* wait until loadedw

* filter test

* delete crypto for now

* Select the correct index 🤦

* timerange

* Adjust functional test logic

* improved test format @Dosant

* Handle exceptions

* Don't close sessions automatically, warn instead

* jest

* Adjust functional test

* Remove unused code

* delete export

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
* master: (51 commits)
  [Discover] Unskip flaky test (elastic#80670)
  Fix security solution template label (elastic#80754)
  [Ingest]: ignore 404, check if there are transforms in results. (elastic#80721)
  Moving loader to logo in header, add a slight 250ms pause (elastic#78879)
  [Security Solution][Cases] Fix bug with case connectors (elastic#80642)
  Update known-plugins.asciidoc (elastic#75388)
  [Lens] Add median operation (elastic#79453)
  Fix navigateToApp logic when navigating to the current app. (elastic#80809)
  [Visualizations] Fix bad color mapping with multiple split series (elastic#80801)
  [ILM] Add esErrorHandler for the new es js client (elastic#80302)
  Fix codeowners (elastic#80826)
  skip flaky suite (elastic#79463)
  [Timelion] Remove kui usage (elastic#80287)
  [Ingest Manager] add skipIfNoDockerRegistry to package_install_complete test (elastic#80779)
  [Alerting UI] Disable "Save" button for Alerts with broken Connectors (elastic#80579)
  Allow the default space to be accessed via `/s/default` (elastic#77109)
  Add script to identify plugin dependencies for TS project references migration (elastic#80463)
  [Search] Client side session service (elastic#76889)
  feat: 🎸 add separator for different context menu groups (elastic#80498)
  Lazy load reporting (elastic#80492)
  ...
@Dosant Dosant mentioned this pull request Nov 18, 2020
38 tasks
@gchaps
Copy link
Contributor

gchaps commented Dec 3, 2020

@lizozom Please update the "Dev Docs" section in the summary to include content that we can pull for the API plugin changes doc.

@bhavyarm
Copy link
Contributor

@lizozom how do I test this PR please? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.