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

Add loading count source for http requests #59245

Merged
merged 2 commits into from
Mar 4, 2020
Merged

Add loading count source for http requests #59245

merged 2 commits into from
Mar 4, 2020

Conversation

joshdover
Copy link
Contributor

Summary

Fixes #59222

Adds an observable to the internal Fetch class that gets registered with the core.http.addLoadingCount$ API so that any pending HTTP requests make the global loading bar appear.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 3, 2020
@joshdover joshdover requested a review from a team as a code owner March 3, 2020 23:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet
pgayvallet approved these changes Mar 4, 2020
View reviewed changes
src/core/public/http/fetch.test.ts Outdated Show resolved Hide resolved
src/core/public/http/http_service.ts
Comment on lines 46 to +48
const fetchService = new Fetch({ basePath, kibanaVersion });
const loadingCount = this.loadingCount.setup({ fatalErrors });
loadingCount.addLoadingCountSource(fetchService.getRequestCount$());
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: could add a test to ensure this is properly called.

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 added a test, but to avoid some really complex mocking I just asserted that addLoadingCountSource is called with an Observable instance, not specifically the one from fetchService.getRequestCount$. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, was under water last week on the SO management, did not see the notif. SGTM.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@joshdover joshdover merged commit 3ec71c3 into elastic:master Mar 4, 2020
@joshdover joshdover deleted the np/loading-count branch March 4, 2020 19:56
@joshdover joshdover mentioned this pull request Mar 4, 2020
Merged
joshdover added a commit to joshdover/kibana that referenced this pull request Mar 4, 2020
@joshdover
Add loading count source for http requests (elastic#59245)
3798272
@rudolf
Copy link
Contributor

rudolf commented Mar 5, 2020

To ease the platform migration I think it's useful that we match the legacy behaviour. But I think we should add the ability to opt-out of increasing the loading count. Users don't need to be made aware of all in-progress http requests. Showing a loading bar when we're sending usage metrics to the server or polling for updates from the pulse newsfeed would probably be distracting or confusing to users.

jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 5, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into console/featu…
ce71db9 
…re/files-and-filetree

* 'master' of github.com:elastic/kibana: (254 commits)
  Convert discover_page to ts, remove redundunt methods (elastic#59312)
  [Fix for Vis Editor] Revert setting time field to empty string when it's undefined (elastic#58873)
  Delete legacy search endpoint (elastic#59341)
  [Uptime] Improve duration chart (elastic#58404)
  [Snapshot & Restore] NP migration (elastic#59109)
  [ML] Add support for date_nanos time field in anomaly job wizard (elastic#59017)
  Revert "Makes alerting and actions optional properties for interface RequestH… (elastic#59264)"
  Change remote_clusters ID to remoteClusters (elastic#59246)
  Makes alerting and actions optional properties for interface RequestH… (elastic#59264)
  Clean up date histogram agg type. (elastic#58805)
  [ML] Management: fix license unsubscribe (elastic#59365)
  Remove documentation for server.cors settings (elastic#59096)
  Edit alert flyout (elastic#58964)
  [SIEM] Fix rule delete/duplicate actions (elastic#59306)
  move mouse to close obstructing tooltip (elastic#59214)
  Reset page after deleting (elastic#59310)
  Make sure phrases input filter triggers autosuggestons (elastic#59299)
  Add loading count source for http requests (elastic#59245)
  Revert "[ML] Transforms: Deprecate custom KibanaContext. (elastic#59133)"
  Expose metrics service to public API (elastic#59294)
  ...

# Conflicts:
#	src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 5, 2020
joshdover added a commit that referenced this pull request Mar 5, 2020
@joshdover
[7.x] Add loading count source for http requests (#59245) (#59359)
0a6858c
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 5, 2020
@joshdover
Copy link
Contributor Author

@rudolf That's a good point. We should probably opt out any requests that are using the asSystemRequest option, but also add another option. I will open an issue.

@joshdover joshdover mentioned this pull request Mar 5, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees
No one assigned
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate core.http.fetch with core.http.loadingCount
5 participants
@joshdover @elasticmachine @kibanamachine @rudolf @pgayvallet