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

[Reporting] Clean up types for internal APIs needed for UI #105508

Merged
merged 13 commits into from
Jul 15, 2021

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jul 13, 2021

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.

For maintainers

@tsullivan tsullivan marked this pull request as draft July 13, 2021 21:36
@tsullivan 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 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 tsullivan marked this pull request as ready for review July 13, 2021 21:39
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@tsullivan tsullivan requested review from jloleysens and dokmic and removed request for jloleysens July 13, 2021 21:39
max_attempts: number;
started_at: string | undefined;
attempts: number;
status: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was all nearly duplicated from ReportSource

import { IlmPolicyLink } from './ilm_policy_link';

export interface Job {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hoisted this to a class defined in another file

@@ -325,28 +303,7 @@ class ReportListingUi extends Component<Props, State> {
this.setState(() => ({
isLoading: false,
total,
jobs: jobs.map(
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@tsullivan tsullivan force-pushed the reporting/job-apis-refactoring branch from 980ac90 to 5d548ed Compare July 13, 2021 21:55
jobId: string;
apiClient: ReportingAPIClient;
}
} & Pick<ListingProps, 'apiClient' | 'intl'>;
Copy link
Member Author

@tsullivan tsullivan Jul 13, 2021

Choose a reason for hiding this comment

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

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

const job = await createJob!(jobParams, context, request);

// 1. Add the report to ReportingStore to show as pending
const report = await store.addReport(
new Report({
jobtype: exportType.jobType,
created_by: user ? user.username : false,
max_attempts: config.get('capture', 'maxAttempts'), // NOTE: since max attempts is stored in the document, changing the capture.maxAttempts setting does not affect existing pending reports
Copy link
Member Author

Choose a reason for hiding this comment

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

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 tsullivan force-pushed the reporting/job-apis-refactoring branch 2 times, most recently from 6fc3653 to eefc828 Compare July 13, 2021 22:04
@@ -26,26 +22,13 @@ const apiResponseFields = [
'status',
];

// TODO: clean up the /list and /info endpoints to return ReportApiJSON interface data
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -135,9 +135,8 @@ export class ExecuteReportTask implements ReportingTask {

const m = moment();

// check if job has exceeded maxAttempts (stored in job params) and somehow hasn't been marked as failed yet
// NOTE: the max attempts value comes from the stored document, so changing the capture.maxAttempts config setting does not affect existing pending reports
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is no longer true as of this PR

@tsullivan tsullivan force-pushed the reporting/job-apis-refactoring branch from eefc828 to 3a85a26 Compare July 13, 2021 22:12
@@ -306,11 +306,14 @@ export class ExecuteReportTask implements ReportingTask {
}

if (!report) {
this.reporting.untrackReport(jobId);
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes #105155

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
x-pack/plugins/reporting/common/types.ts Outdated Show resolved Hide resolved
* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@tsullivan tsullivan Jul 14, 2021

Choose a reason for hiding this comment

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

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?

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`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

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

Left a few minor suggestions. LGTM, great job 👍

@tsullivan
Copy link
Member Author

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
Copy link
Contributor

💚 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

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

@tsullivan tsullivan merged commit 84af487 into elastic:master Jul 15, 2021
@tsullivan tsullivan deleted the reporting/job-apis-refactoring branch July 15, 2021 00:49
tsullivan added a commit to tsullivan/kibana that referenced this pull request Jul 15, 2021
…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>
tsullivan added a commit that referenced this pull request Jul 15, 2021
…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>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 15, 2021
…-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
@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0
Projects
None yet
6 participants