[Uptime] replace fetch with kibana http #59881

shahzad31 · 2020-03-11T10:39:26Z

Summary

Fixes #56894
This PR will replace fetch usage with kibana http.

elasticmachine · 2020-03-11T10:39:28Z

Pinging @elastic/uptime (Team:uptime)

andrewvc

Definitely an improvement! Left some notes.

andrewvc · 2020-03-13T18:41:46Z

x-pack/legacy/plugins/uptime/public/uptime_app.tsx

    );
  }, [canSave, renderGlobalHelpControls, setBadge]);

+  apiService.http = core.http;


Per our discussion, can we just store the whole of core here. I think that's generally more useful.

have exposed a core via kibana_service.ts https://github.com/elastic/kibana/pull/59881/files#diff-9da56dd30eec66800d99e2aa39faf33e

x-pack/legacy/plugins/uptime/public/state/effects/fetch_effect.ts

x-pack/legacy/plugins/uptime/public/state/reducers/overview_filters.ts

andrewvc · 2020-03-13T19:13:57Z

x-pack/legacy/plugins/uptime/public/state/api/overview_filters.ts

  const decoded = OverviewFiltersType.decode(responseData);

-  ThrowReporter.report(decoded);
+  PathReporter.report(decoded);


Does this do anything? IIRC Throw reporter issues a throw, while PathReporter returns a value like what's shown here: https://github.com/gcanti/io-ts#error-reporters . I don't think we should be making this change, which is unrelated, in this PR.

ThrowReporter is deprecated.

Good to know. That said, I think we'll need throw new Error(PathReporter.report(decoded)) the code as-is just generates a string and throws it away.

we don't need to throw it, because we are not catching it. We are only checking for errors now.

andrewvc · 2020-03-13T19:14:34Z

x-pack/legacy/plugins/uptime/public/state/api/snapshot.ts

+
  const decoded = SnapshotType.decode(responseData);
-  ThrowReporter.report(decoded);
+  PathReporter.report(decoded);


Same comment here, let's stick with the ThrowReporter. Let's not mix too many things into one PR unless there's an issue with separating them out.

we don't need to throw it, because we are not catching it. We are only checking for errors now.

andrewvc · 2020-03-13T19:19:09Z

x-pack/legacy/plugins/uptime/public/state/api/utils.ts

+
+  public async get(apiUrl: string, params: HttpFetchQuery) {
+    const response = await this.http!.get(apiUrl, { query: params });
+    if (response instanceof Error) {


Are we trying to make Kibana's fetch more like window.fetch where errors are thrown instead of returned?

have updated this.

kibanamachine · 2020-03-16T18:09:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6d9f642

History

💔 Build #33454 failed 570162b
💔 Build #33414 failed 94a6860
💚 Build #33187 succeeded 6ef0c5c
💔 Build #33069 failed 3136295

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

justinkambic

Left a comment regarding io-ts usage for api requests.

justinkambic · 2020-03-17T13:29:48Z

x-pack/legacy/plugins/uptime/public/state/api/utils.ts

+    return ApiService.instance;
+  }
+
+  public async get(apiUrl: string, params?: HttpFetchQuery, decodeType?: any) {


I think it would be better to require the decodeType parameter. We could then describe decodeType better than any. If we did something like:

import { Decoder } from 'io-ts'

Suggested change

public async get(apiUrl: string, params?: HttpFetchQuery, decodeType?: any) {

public async get<T>(apiUrl: string, params?: HttpFetchQuery, decoder: Decoder<unknown, T>) {

In this case, T will be the type we get when isRight is truthy, and unknown is the type we specify for the value we give to PathReporter (which is ok, because decode will return an object of type Either<t.Errors, T> anyway, so the reporter will be happy).

Partial, Type, Intersection, etc., all seem to extend the Decoder interface, so I think this would be a safe assumption for us to make here.

Then, if we define a complex type like:

const MyComplexType = t.intersection([ t.partial({ foo: t.string, }), t.type({ bar: t.number, }), ]);

..the usage of the function will remain the same, but our responses will be typed appropriately:

const apiResponse = apiService.get('/api/pathToComplexType', { foo: 'bar' }, MyComplexType);

And the resultant object will be typed like:

If we allow the decodeType parameter to be optional, we can only ever guarantee a return type of any; this is more-or-less what we've always done, but the idea of type safety at runtime and "automatic" typing for all API responses sounds very nice to me.

If we feel it's out of the scope of this PR, a tech debt follow-up issue would be ok by me too, if we are agreed this is something we'd want.

I created #60395 to track the addition of this. I think this will be a nice add when we can get to it.

andrewvc

LGTM

* master: [NP] Cutover ensureDefaultIndexPattern to kibana_utils (elastic#59895) Closes elastic#60265. Adds Beta badge to service map (elastic#60482) [Visualize] Duplicated query filters in es request (elastic#60106) [ML] Disable functional transform tests Fixes to service map single node banner (elastic#60072) [Uptime] replace fetch with kibana http (elastic#59881) Upgrade @types/node to match Node.js runtime (elastic#60368)

…nless * 'app/painless' of github.com:elastic/kibana: (64 commits) Fix filter scope in bool query (#60488) change index pattern id to be the same as index pattern title (#60436) [Endpoint] resolver v1 events (#59233) Branding fixes for dashboard, loader and space selector (#60073) skip flaky suite (#60535) [SIEM][Detection Engine] Fixes bug with timeline templates not working Fixed errors which are happening if switch between alert types (#60453) [EPM] Add mapping field types to index template generation v2 (#60266) [NP] Cutover ensureDefaultIndexPattern to kibana_utils (#59895) Closes #60265. Adds Beta badge to service map (#60482) [Visualize] Duplicated query filters in es request (#60106) [ML] Disable functional transform tests Fixes to service map single node banner (#60072) [Uptime] replace fetch with kibana http (#59881) Upgrade @types/node to match Node.js runtime (#60368) [License Management] NP migration (#60250) Fix create alert button from not showing in alerts list (#60444) [SIEM][Case] Update connector through flyout (#60307) add data-test-subj where possible on SO management table (#60226) Enforce `required` presence for value/key validation of `recordOf` and `mapOf`. (#60406) ...

* upstream/app/painless: (66 commits) Another i18n issue Fix i18n Fix filter scope in bool query (elastic#60488) change index pattern id to be the same as index pattern title (elastic#60436) [Endpoint] resolver v1 events (elastic#59233) Branding fixes for dashboard, loader and space selector (elastic#60073) skip flaky suite (elastic#60535) [SIEM][Detection Engine] Fixes bug with timeline templates not working Fixed errors which are happening if switch between alert types (elastic#60453) [EPM] Add mapping field types to index template generation v2 (elastic#60266) [NP] Cutover ensureDefaultIndexPattern to kibana_utils (elastic#59895) Closes elastic#60265. Adds Beta badge to service map (elastic#60482) [Visualize] Duplicated query filters in es request (elastic#60106) [ML] Disable functional transform tests Fixes to service map single node banner (elastic#60072) [Uptime] replace fetch with kibana http (elastic#59881) Upgrade @types/node to match Node.js runtime (elastic#60368) [License Management] NP migration (elastic#60250) Fix create alert button from not showing in alerts list (elastic#60444) [SIEM][Case] Update connector through flyout (elastic#60307) ...

…alerting/tls-warning * 'alerting/tls-warning' of github.com:gmmorris/kibana: (33 commits) [ML] Disable functional transform tests Fixes to service map single node banner (elastic#60072) [Uptime] replace fetch with kibana http (elastic#59881) Upgrade @types/node to match Node.js runtime (elastic#60368) [License Management] NP migration (elastic#60250) Fix create alert button from not showing in alerts list (elastic#60444) [SIEM][Case] Update connector through flyout (elastic#60307) add data-test-subj where possible on SO management table (elastic#60226) Enforce `required` presence for value/key validation of `recordOf` and `mapOf`. (elastic#60406) [ML] Re-enabling file upload telemetry (elastic#60418) [NP] Use local helper shortenDottedString for discover (elastic#60271) [Console] Fix for `_settings` and x-pack autocomplete (elastic#60246) Task/host enhancements (elastic#59671) [Search service] Asynchronous ES search strategy (elastic#53538) Index Action - Moved index params fields to connector config (elastic#60349) Edits UI text for ML nodes and job button (elastic#60184) Publish getIsNavDrawerLocked$ method on core chrome service. (elastic#60191) Disabled edit alert button on management ui for non registered UI alert types (elastic#60439) Revert "[Console] Fix bool filter autocompletions and refactor (elastic#60361)" [Console] Fix bool filter autocompletions and refactor (elastic#60361) ...

kibanamachine · 2020-03-19T13:48:44Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59881 or prevent reminders by adding the backport:skip label.

* use kibana http * unused import * fix type * update type * refactor * fix types * fix type * fix type

kibanamachine · 2020-03-20T13:58:44Z

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 · 2020-03-21T14:08:44Z

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.

Backports the following commits to 7.x: [Uptime] replace fetch with kibana http (#59881)

use kibana http

c4f0ca1

shahzad31 added v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 11, 2020

shahzad31 requested review from andrewvc and justinkambic March 11, 2020 10:39

shahzad31 self-assigned this Mar 11, 2020

shahzad31 marked this pull request as ready for review March 11, 2020 10:39

shahzad31 added 2 commits March 11, 2020 11:41

unused import

b1077a5

Merge branch 'master' into user-kibana-http

3136295

andrewvc requested changes Mar 13, 2020

View reviewed changes

shahzad31 added 9 commits March 13, 2020 20:27

fix type

5d31f8e

Merge branch 'master' into user-kibana-http

13d60e8

update type

6ef0c5c

refactor

36117e5

Merge branch 'master' into user-kibana-http

94a6860

fix types

0dc7e8a

Merge branch 'master' into user-kibana-http

7e6339b

fix type

570162b

fix type

6d9f642

justinkambic reviewed Mar 17, 2020

View reviewed changes

justinkambic mentioned this pull request Mar 17, 2020

[Uptime] Require type check object for API requests #60395

Closed

andrewvc approved these changes Mar 18, 2020

View reviewed changes

shahzad31 merged commit 95a42ed into elastic:master Mar 18, 2020

shahzad31 deleted the user-kibana-http branch March 18, 2020 13:43

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 19, 2020

shahzad31 mentioned this pull request Mar 20, 2020

[7.x] [Uptime] replace fetch with kibana http (#59881) #60729

Merged

shahzad31 added a commit to shahzad31/kibana that referenced this pull request Mar 20, 2020

[Uptime] replace fetch with kibana http (elastic#59881)

36aa491

* use kibana http * unused import * fix type * update type * refactor * fix types * fix type * fix type

shahzad31 mentioned this pull request Mar 20, 2020

[Uptime]Update fetch effect failed action handling #60742

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 22, 2020

andrewvc pushed a commit that referenced this pull request Mar 22, 2020

[Uptime] replace fetch with kibana http (#59881) (#60729)

35b98f6

Backports the following commits to 7.x: [Uptime] replace fetch with kibana http (#59881)

	public async get(apiUrl: string, params?: HttpFetchQuery, decodeType?: any) {
	public async get<T>(apiUrl: string, params?: HttpFetchQuery, decoder: Decoder<unknown, T>) {

[Uptime] replace fetch with kibana http #59881

[Uptime] replace fetch with kibana http #59881

Uh oh!

Conversation

shahzad31 commented Mar 11, 2020

Summary

Uh oh!

elasticmachine commented Mar 11, 2020

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 16, 2020

💚 Build Succeeded

History

Uh oh!

justinkambic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewvc left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 19, 2020

Uh oh!

kibanamachine commented Mar 20, 2020

Uh oh!

kibanamachine commented Mar 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants