[Reporting] Clean up types for internal APIs needed for UI by tsullivan · Pull Request #105508 · elastic/kibana

tsullivan · 2021-07-13T21:34:32Z

Summary

Needed for #102833

Closes #104791
Closes #105155
Closes #103360

Cleanups to achieve type consistency:

ReportSource['max_attempts'] is allowed to be undefined.
Note: The max attempts number is taken from configuration of the Kibana instance executing the job. This makes the max attempts number more consistent with other fields that only apply to jobs that have been claimed such as timeout and browser_type.
ReportApiJSON:
- Removed duplicated type definitions by using ReportSource as the source of truth
- payload.headers and output.content are omitted to avoid long strings of encrypted text in API responses
Added the Job class in reporting/public/lib/job.ts to handle implementation details of the fields of a report job for the UI.
- More will be added later to show warnings, errors, and job status in a more consistent way.
public/lib/reporting_api_client/reporting_api_client.ts
- Removed unnecessary type definitions
- Use more async/await for debuggability
No more implicit any
Removed some usage of _.get()

Changes to API responses:

/api/reporting/jobs/list
- Changed the format of the response data to ReportApiJSON[] format. Previously, the response data was unformatted from the original document in Elasticsearch (ReportSource format).

These are internal APIs and are not subject to backwards compatibility with older scripts.

This PR will not contain any user-facing changes as those will be isolated to #102833

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-07-13T21:39:12Z

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

elasticmachine · 2021-07-13T21:39:13Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

tsullivan · 2021-07-13T21:39:51Z

x-pack/plugins/reporting/common/types.ts

This was all nearly duplicated from ReportSource

tsullivan · 2021-07-13T21:52:47Z

x-pack/plugins/reporting/public/management/report_listing.tsx

Hoisted this to a class defined in another file

tsullivan · 2021-07-13T21:53:31Z

x-pack/plugins/reporting/public/management/report_listing.tsx

The map is no longer needed as apiClient.list returns data in a usable format

tsullivan · 2021-07-13T21:57:44Z

x-pack/plugins/reporting/public/management/report_info_button.tsx

This component is updated with a minor UI improvement to show a tooltip over the "info" icon. This makes it consistent with the "download" icon.

Fixes #103360

tsullivan · 2021-07-13T21:59:05Z

x-pack/plugins/reporting/server/lib/enqueue_job.ts

Max attempts is set on the report once it is claimed. The main reason to do this is to show the information in the "info" panel in the UI.

tsullivan · 2021-07-13T22:07:50Z

x-pack/test/reporting_api_integration/reporting_without_security/job_apis_csv.ts

This is done. The /list endpoint had the most inconsistencies.

tsullivan · 2021-07-13T22:12:16Z

x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

This comment is no longer true as of this PR

tsullivan · 2021-07-13T22:19:23Z

x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

          }

          if (!report) {
+            this.reporting.untrackReport(jobId);


Fixes #105155

jloleysens

Great job @tsullivan ! I did not run this locally, but all the changes make sense and I'm happy for this to be merged given green CI.

x-pack/plugins/reporting/common/types.ts

jloleysens · 2021-07-14T11:02:18Z

x-pack/plugins/reporting/public/lib/job.ts

+ * This class represents a report job for the UI
+ * It can be instantiated with ReportApiJSON: the response data format for the report job APIs
+ */
+export class Job {


Why are we using a class here? I think we could accomplish the same thing with just an interface and a function that maps one object to another (if needed). Unless we intend to associate behaviours with this in way that is in keeping with how we use classes elsewhere in Reporting.

Unless we intend to associate behaviours with this in way that is in keeping with how we use classes elsewhere in Reporting.

Yes, this is the case. In an earlier incarnation of the code, there are methods on this class for achieving consistency about warnings [1], and then further on, showing some info about deprecated export types. That got to be a lot of changes in the UI that just needed to be isolated to a separate PR.

[1] Why is the only warning shown in a table cell about when the max size was reached? Why don't we use the table to show that a CSV could contain formulas, but we do show it in the toast notification? Why doesn't the info side panel show "status" the same way it's shown in the table?

x-pack/plugins/reporting/public/lib/reporting_api_client/reporting_api_client.ts

jloleysens · 2021-07-14T11:09:07Z

x-pack/plugins/reporting/public/lib/reporting_api_client/reporting_api_client.ts

-  public verifyConfig = (): Promise<DiagnoseResponse> =>
-    this.http.post(`${API_BASE_URL}/diagnose/config`, {
+  public async verifyConfig() {
+    return await this.http.post(`${API_BASE_URL}/diagnose/config`, {


Just FWIW: these functions don't do anything with the call result so we don't need async await sugar. I think the return type gaurds have us covered.

It's kind of been preached to me to use this way instead of "desugared promises." It makes stack traces more readable, and there are some optimizations in V8 that went into making async and await native. https://v8.dev/blog/fast-async

x-pack/plugins/reporting/common/types.ts

x-pack/plugins/reporting/public/management/report_info_button.tsx

x-pack/plugins/reporting/server/routes/lib/jobs_query.ts

dokmic

Left a few minor suggestions. LGTM, great job 👍

Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com>

Co-authored-by: Michael Dokolin <dokmic@gmail.com>

tsullivan · 2021-07-14T21:42:43Z

Found some bugs in the info panel due to usage of _.get: 6c847d6

The next PR will have functional tests that cover the info panel in Reporting Management.

kibanamachine · 2021-07-14T23:51:14Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`reporting`	59	60	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`reporting`	69.9KB	70.1KB	+220.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`reporting`	18	14	-4

Page load bundle

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

id	before	after	diff
`reporting`	56.5KB	58.8KB	+2.3KB

History

💔 Build #138585 failed 93dadad
💔 Build #138578 failed 5265dc9
💚 Build #138559 succeeded abe2627
💔 Build #138320 failed 3a85a26

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

…05508) * [Reporting] Refactor types for API response data * fix api test * Update x-pack/plugins/reporting/common/types.ts Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com> * more verbose comments * set comments in interface type definition * Update x-pack/plugins/reporting/common/types.ts Co-authored-by: Michael Dokolin <dokmic@gmail.com> * more strict * Update x-pack/plugins/reporting/public/management/report_info_button.tsx Co-authored-by: Michael Dokolin <dokmic@gmail.com> * Update x-pack/plugins/reporting/public/management/report_info_button.tsx Co-authored-by: Michael Dokolin <dokmic@gmail.com> * fix suggestion * fix accidental declaration of interface methods as function properties * fix info panel Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com> Co-authored-by: Michael Dokolin <dokmic@gmail.com>

…105702) * [Reporting] Refactor types for API response data * fix api test * Update x-pack/plugins/reporting/common/types.ts Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com> * more verbose comments * set comments in interface type definition * Update x-pack/plugins/reporting/common/types.ts Co-authored-by: Michael Dokolin <dokmic@gmail.com> * more strict * Update x-pack/plugins/reporting/public/management/report_info_button.tsx Co-authored-by: Michael Dokolin <dokmic@gmail.com> * Update x-pack/plugins/reporting/public/management/report_info_button.tsx Co-authored-by: Michael Dokolin <dokmic@gmail.com> * fix suggestion * fix accidental declaration of interface methods as function properties * fix info panel Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com> Co-authored-by: Michael Dokolin <dokmic@gmail.com> Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com> Co-authored-by: Michael Dokolin <dokmic@gmail.com>

…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (75 commits) [Search Sessions] Don’t try to delete errored searches (elastic#105434) [Search Sessions] fix saved object can be created even if courier:batchSearches is enabled (elastic#105407) [Remote Clusters] Fixed remote clusters details flyout for long strings (elastic#105592) [ML] Functional tests - re-activate a11y tests (elastic#105198) [APM] Typed client-side routing (elastic#104274) [Canvas] Expression error (elastic#103048) [ML] Fixing job wizard with missing description (elastic#105574) [Security Solution][Alerts] - Add alerts subfeature UI (elastic#105505) Upgrade EUI to v35.0.0 (elastic#105127) [Reporting] Clean up types for internal APIs needed for UI (elastic#105508) skip flaky suite (elastic#105087) [Workplace Search] Fix Chrome issues with GitHub sources (elastic#105680) [Fleet] Add containerized fleet server instructions to Fleet README (elastic#105669) [ML] Add api integration test for analytics map endpoint (elastic#105531) Fixes cypress flake across two tests (elastic#105645) [Logs&Metrics UI] add owner properties to plugin manifest (elastic#105580) chore(NA): introduce preset for jest-integration tests on @kbn/test (elastic#105144) [Enterprise Search] Added Thumbnails to Search UI (elastic#104199) Translate App Search credentials list (elastic#105619) [APM] APM agent config created prior to Fleet migration is not injected into integration policy (elastic#105504) ... # Conflicts: # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/management/report_listing.test.tsx

tsullivan marked this pull request as draft July 13, 2021 21:36

tsullivan changed the title ~~[Reporting] Refactor types for API response data~~ [Reporting] Clean up types for internal APIs needed for UI Jul 13, 2021

tsullivan added Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0 labels Jul 13, 2021

tsullivan marked this pull request as ready for review July 13, 2021 21:39

tsullivan requested review from dokmic and jloleysens and removed request for jloleysens July 13, 2021 21:39

tsullivan commented Jul 13, 2021

View reviewed changes

tsullivan force-pushed the reporting/job-apis-refactoring branch from 980ac90 to 5d548ed Compare July 13, 2021 21:55

tsullivan commented Jul 13, 2021

View reviewed changes

tsullivan force-pushed the reporting/job-apis-refactoring branch 2 times, most recently from 6fc3653 to eefc828 Compare July 13, 2021 22:04

tsullivan commented Jul 13, 2021

View reviewed changes

[Reporting] Refactor types for API response data

3a85a26

tsullivan force-pushed the reporting/job-apis-refactoring branch from eefc828 to 3a85a26 Compare July 13, 2021 22:12

tsullivan commented Jul 13, 2021

View reviewed changes

jloleysens approved these changes Jul 14, 2021

View reviewed changes

dokmic reviewed Jul 14, 2021

View reviewed changes

x-pack/plugins/reporting/common/types.ts Outdated Show resolved Hide resolved

dokmic reviewed Jul 14, 2021

View reviewed changes

x-pack/plugins/reporting/public/management/report_info_button.tsx Outdated Show resolved Hide resolved

dokmic reviewed Jul 14, 2021

View reviewed changes

x-pack/plugins/reporting/public/management/report_info_button.tsx Outdated Show resolved Hide resolved

dokmic reviewed Jul 14, 2021

View reviewed changes

x-pack/plugins/reporting/server/routes/lib/jobs_query.ts Outdated Show resolved Hide resolved

dokmic approved these changes Jul 14, 2021

View reviewed changes

tsullivan and others added 12 commits July 14, 2021 10:40

Merge branch 'master' into reporting/job-apis-refactoring

325767d

fix api test

abe2627

Update x-pack/plugins/reporting/common/types.ts

25b5017

Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com>

more verbose comments

50d8b98

set comments in interface type definition

5cf4e22

Update x-pack/plugins/reporting/common/types.ts

b12d800

Co-authored-by: Michael Dokolin <dokmic@gmail.com>

more strict

5265dc9

Update x-pack/plugins/reporting/public/management/report_info_button.tsx

7d1b2a5

Co-authored-by: Michael Dokolin <dokmic@gmail.com>

Update x-pack/plugins/reporting/public/management/report_info_button.tsx

6ad12c4

Co-authored-by: Michael Dokolin <dokmic@gmail.com>

fix suggestion

296e137

fix accidental declaration of interface methods as function properties

93dadad

fix info panel

6c847d6

tsullivan merged commit 84af487 into elastic:master Jul 15, 2021

tsullivan deleted the reporting/job-apis-refactoring branch July 15, 2021 00:49

tsullivan mentioned this pull request Jul 15, 2021

[7.x] [Reporting] Clean up types for internal APIs needed for UI (#105508) #105702

Merged

This was referenced Jul 19, 2021

[Reporting] Error content of failed jobs is not available #106154

Closed

[Reporting] Reporting Job Params should not include timeout and max attempts. #76411

Closed

tsullivan mentioned this pull request Jul 27, 2021

Reporting: Generating report for large dashboard fails #106873

Closed

sophiec20 added the zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024

Conversation

tsullivan commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jul 13, 2021

Uh oh!

elasticmachine commented Jul 13, 2021

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!

tsullivan Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsullivan Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dokmic left a comment

Choose a reason for hiding this comment

Uh oh!

tsullivan commented Jul 14, 2021

Uh oh!

kibanamachine commented Jul 14, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Public APIs missing exports

Page load bundle

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tsullivan commented Jul 13, 2021 •

edited

Loading

tsullivan Jul 13, 2021 •

edited

Loading

tsullivan Jul 14, 2021 •

edited

Loading