[Reporting] add version to all export types job params #106137

tsullivan · 2021-07-19T17:35:06Z

Summary

This PR adds a version field to the BaseParams interface that is extended by job params of all export types. This field is to allow future compatibility of automated reports.

Closes #104508

Other changes:

Adds getDecoratedJobParams to the ReportingAPIClient. This method adds the common fields: version and browserTimezone to the job params so that the applications do not have to provide those fields in their intergration.
- This removes code that was repeated in a lot of places
Simplifies the API surface by not requiring the apps to provide browserTimezone. This was done as part of responding to review feedback: [Reporting] add version to all export types job params #106137 (review)
Simplifies internal Reporting UI code by only instantiating a single ReportingAPIClient for all the code
Cleaned up unused dependencies in the Reporting example app
Improved tests by removing some any and importing mock providers
Updated get_csv_panel_action.tsx to use the common API Client and access getDecoratedJobParams
Added FIXME comments where my editor hinted at deprecated dependencies
Adds unit tests for verifying POST URL is correct, and is automatically decorated with version and browserTimezone

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

elasticmachine · 2021-07-19T17:36:53Z

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

elasticmachine · 2021-07-19T17:36:53Z

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

ppisljar

i think we shouldn't store csv searchsource version, but just general kibana version at which report was generated. any report will run into this problem as we are not using saved objects (if we will that would be handled for us) and possibly we'll need to run migrations.

searchsource also needs to implement persistable state service interface so we will be able to call migrations on it.

this should be done before 8.0 to prepare for saved object ids change

tsullivan · 2021-07-20T17:37:25Z

@ppisljar thanks for the feedback. I have added commit de232ba to use the kibana version from packageInfo on the plugin init context.

ppisljar · 2021-07-21T03:57:47Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/create_job.ts

+      logger.debug(`Using SearchSource v${jobParams.version}`);
+    } else {
+      logger.warning(
+        `No version provided for SearchSource version. Assuming ${reporting.getKibanaVersion()}`


here we should not assume current version but the last version where reports didn't store the version (7.14)

Hi @ppisljar is it essential to drop the .0 from the version string? Currently I am sticking it on just as-is from context.env.packageInfo.version, which looks like 8.0.0 and 7.14.0, etc.

i think that's fine. better more than less :D

ppisljar · 2021-07-21T04:25:31Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/types.d.ts

@@ -11,7 +11,8 @@ import type { BaseParams, BasePayload } from '../../types';
 export type RawValue = string | object | null | undefined;

 interface BaseParamsCSV {
-  browserTimezone: string;
+  browserTimezone: string; // used for formatting timestamps
+  version: string; // version of the last supported release in case it needs migration for forward-compatibility


could we rather put this on Report options and store it for every report type (inside enqueueJob?) and then make sure its sent over from client to server inside reporting_api_client createReportJob ?

i think we can make this a general implementation for all report types and its going to be simpler.

ppisljar · 2021-07-29T05:07:29Z

x-pack/examples/reporting_example/public/application.tsx

@@ -14,7 +14,7 @@ import { ReportingExampleApp } from './components/app';
 export const renderApp = (
  coreStart: CoreStart,
  deps: Omit<StartDeps & SetupDeps, 'developerExamples'>,
-  { appBasePath, element }: AppMountParameters
+  { appBasePath, element }: AppMountParameters // FIXME: appBasePath is deprecated


worth opening an issue for this, adding a blocker label to it and a version in which the appBasePath will be removed

ppisljar · 2021-07-29T05:16:02Z

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

-  public getReportingJobPath(exportType: string, jobParams: JobParams) {
-    const params = stringify({ jobParams: rison.encode(jobParams) });
+  public getReportingJobPath(exportType: string, jobParams: BaseParams) {
+    const risonObject: RisonObject = jobParams as Record<string, any>;


why do we need this additional casting now?

This is pretty much moving the casting down from the local type which was removed: interface JobParams

ppisljar · 2021-07-29T05:17:46Z

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

+ * Job Params are defined by the application, and on the client side, are decorated by the Reporting service.
+ * These are required for handling additional formatting or state migrations
+ */
+export type DecoratedBaseParams = BaseParams & {


whats the reason for adding DecoratedBaseParams instead of just adding browserTimezone and version to BaseParams ?

This allowed the getJobParams functions of the various apps to be correctly typed in saying they return BaseParams.

Reporting has still has trouble spots in the code where apps can not really import the correct type that they want to, because those types are restricted to the reporting code, on the server side. It makes sense to focus on these issues later.

I've undone the addition of DecoratedBaseParams: 300ba45

ppisljar · 2021-07-29T05:18:14Z

x-pack/plugins/reporting/public/share_context_menu/index.ts

+ * 2.0.
+ */
+
+import * as Rx from 'rxjs';


was this moved from somewhere ?

These types were moved here from share_context_menu/register_csv_reporting.tsx and share_context_menu/register_pdf_png_reporting.tsx, to remove a lot of duplicated imports.

ppisljar · 2021-07-29T05:22:09Z

x-pack/plugins/reporting/server/export_types/csv_searchsource_immediate/execute_job.ts

    const job = {
      objectType: 'immediate-search',
+      version: reporting.getKibanaVersion(),


why do we need to provide the version here ? from my understanding:

version is provided from the client side (in the generate report url or when clicking the generate report in ui)

server checks if version is provided and if not fallbacks to 7.14.0

It was because the job object gets passed directly to GenerateCSV, since this is an "immediate download" export - not a queued job. which had the parameter typed as JobParamsCSV and therefore needed the version.

This can be simplified by adding Omit<> around the type in the parameter: 300ba45

tsullivan · 2021-07-29T06:27:06Z

@elasticmachine merge upstream

tsullivan · 2021-07-30T02:33:38Z

x-pack/plugins/reporting/public/plugin.ts

    const {
      home,
      management,
-      licensing: { license$ },
+      licensing: { license$ }, // FIXME: 'license$' is deprecated


ppisljar

code LGTM

tsullivan · 2021-08-02T14:38:42Z

@elasticmachine merge upstream

kibanamachine · 2021-08-02T17:00:34Z

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id	before	after	diff
`canvas`	1.5MB	1.5MB	-75.0B
`reporting`	68.4KB	68.3KB	-157.0B
total			-232.0B

Page load bundle

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

id	before	after	diff
`reporting`	64.7KB	65.0KB	+323.0B

Unknown metric groups

API count

id	before	after	diff
`reporting`	133	140	+7

API count missing comments

id	before	after	diff
`reporting`	132	139	+7

API count with any type

id	before	after	diff
`reporting`	1	0	-1

Non-exported public API item count

id	before	after	diff
`reporting`	14	15	+1

History

💚 Build #141829 succeeded dd06b40
💔 Build #141766 failed 1c96876
💔 Build #141752 failed a6e34b0
💔 Build #141548 failed f4710b7
💔 Build #141541 failed f753f76

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

* add version to csv params * fix ts * fix api tests * use kibana version from packageInfo * use kibana version from packageInfo * clean up ide warnings * utility to log and set a default params version * fix baseparams ts * update snapshot * check version in enqueue job * add temporary ts-ignore for canvas * clarify comment * fix hardcoded version in png_pdf_panel * clarify the UNVERSIONED_VERSION variable with a comment * fix canvas jest test * fix ts in example app * fix types * send version param to canvas util for job params * update jest snapshot * Update utils.test.ts * fix snapshot * remove browserTimezone and version from integration boilerplate * wip ensure version is always populated in job params inside of the service * wip2 * wip3 * wip4 * wip5 * wip6 * update note * update example plugin * wip7 * improve tests * fix dynamic job params * better testing * improve enqueue_job test * more tests * fix types * fix types * fix example ts * simplify props * fix test * --wip-- [skip ci] * consolidate baseparams back into one interface * fix rison encoding of apiClient param * clean up * reorganize imports * back out functional change * fix 400 error in download csv Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/share_context_menu/screen_capture_panel_content.tsx # x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts

#107448) * [Reporting] add version to all export types job params (#106137) * add version to csv params * fix ts * fix api tests * use kibana version from packageInfo * use kibana version from packageInfo * clean up ide warnings * utility to log and set a default params version * fix baseparams ts * update snapshot * check version in enqueue job * add temporary ts-ignore for canvas * clarify comment * fix hardcoded version in png_pdf_panel * clarify the UNVERSIONED_VERSION variable with a comment * fix canvas jest test * fix ts in example app * fix types * send version param to canvas util for job params * update jest snapshot * Update utils.test.ts * fix snapshot * remove browserTimezone and version from integration boilerplate * wip ensure version is always populated in job params inside of the service * wip2 * wip3 * wip4 * wip5 * wip6 * update note * update example plugin * wip7 * improve tests * fix dynamic job params * better testing * improve enqueue_job test * more tests * fix types * fix types * fix example ts * simplify props * fix test * --wip-- [skip ci] * consolidate baseparams back into one interface * fix rison encoding of apiClient param * clean up * reorganize imports * back out functional change * fix 400 error in download csv Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/share_context_menu/screen_capture_panel_content.tsx # x-pack/plugins/reporting/server/export_types/printable_pdf/create_job/index.ts * fix 7.x

* add version to csv params * fix ts * fix api tests * use kibana version from packageInfo * use kibana version from packageInfo * clean up ide warnings * utility to log and set a default params version * fix baseparams ts * update snapshot * check version in enqueue job * add temporary ts-ignore for canvas * clarify comment * fix hardcoded version in png_pdf_panel * clarify the UNVERSIONED_VERSION variable with a comment * fix canvas jest test * fix ts in example app * fix types * send version param to canvas util for job params * update jest snapshot * Update utils.test.ts * fix snapshot * remove browserTimezone and version from integration boilerplate * wip ensure version is always populated in job params inside of the service * wip2 * wip3 * wip4 * wip5 * wip6 * update note * update example plugin * wip7 * improve tests * fix dynamic job params * better testing * improve enqueue_job test * more tests * fix types * fix types * fix example ts * simplify props * fix test * --wip-- [skip ci] * consolidate baseparams back into one interface * fix rison encoding of apiClient param * clean up * reorganize imports * back out functional change * fix 400 error in download csv Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

add version to csv params

de232ba

tsullivan changed the title ~~add version to csv params~~ [Reporting] add version to csv_searchsource params Jul 19, 2021

tsullivan requested review from jloleysens, dokmic and a team July 19, 2021 17:36

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

tsullivan added the technical debt Improvement of the software architecture and operational architecture label Jul 19, 2021

fix ts

915fa9c

ppisljar requested changes Jul 20, 2021

View reviewed changes

tsullivan added 5 commits July 20, 2021 09:23

Merge branch 'master' into reporting/csv-searchsource-version-param

1482d03

fix api tests

141a174

use kibana version from packageInfo

dfa0e56

use kibana version from packageInfo

f80097e

clean up ide warnings

9b592f6

tsullivan requested a review from ppisljar July 20, 2021 19:46

ppisljar reviewed Jul 21, 2021

View reviewed changes

tsullivan added 6 commits July 21, 2021 10:24

Merge branch 'master' into reporting/csv-searchsource-version-param

b37a238

utility to log and set a default params version

02688b7

fix baseparams ts

793618c

update snapshot

96d0dff

check version in enqueue job

8ff2152

Merge branch 'master' into reporting/csv-searchsource-version-param

27f1181

tsullivan added 2 commits July 28, 2021 18:22

fix example ts

dcfda41

simplify props

8ca7cfc

ppisljar reviewed Jul 29, 2021

View reviewed changes

tsullivan added 3 commits July 28, 2021 22:44

fix test

0dca459

--wip-- [skip ci]

f753f76

consolidate baseparams back into one interface

300ba45

kibanamachine and others added 8 commits July 29, 2021 02:27

Merge branch 'master' into reporting/csv-searchsource-version-param

f4710b7

Merge branch 'master' into reporting/csv-searchsource-version-param

0c85dab

fix rison encoding of apiClient param

a6e34b0

clean up

87dc4ba

reorganize imports

d74cc9e

back out functional change

1c96876

Merge branch 'master' into reporting/csv-searchsource-version-param

47329a1

fix 400 error in download csv

dd06b40

tsullivan commented Jul 30, 2021

View reviewed changes

ppisljar approved these changes Aug 2, 2021

View reviewed changes

Merge branch 'master' into reporting/csv-searchsource-version-param

abdd0e6

tsullivan enabled auto-merge (squash) August 2, 2021 16:11

tsullivan merged commit 5e8b242 into elastic:master Aug 2, 2021

tsullivan mentioned this pull request Aug 2, 2021

[7.x] [Reporting] add version to all export types job params (#106137) #107448

Merged

tsullivan deleted the reporting/csv-searchsource-version-param branch August 2, 2021 19:37

tsullivan mentioned this pull request Oct 11, 2021

Move reporting job types to /common #114400

Merged

1 task

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] add version to all export types job params #106137

[Reporting] add version to all export types job params #106137

tsullivan commented Jul 19, 2021 •

edited

Loading

elasticmachine commented Jul 19, 2021

elasticmachine commented Jul 19, 2021

ppisljar left a comment

tsullivan commented Jul 20, 2021

ppisljar Jul 21, 2021

tsullivan Jul 21, 2021

ppisljar Jul 22, 2021

ppisljar Jul 21, 2021

ppisljar Jul 29, 2021

tsullivan Jul 30, 2021

ppisljar Jul 29, 2021

tsullivan Jul 29, 2021

ppisljar Jul 29, 2021

tsullivan Jul 29, 2021 •

edited

Loading

ppisljar Jul 29, 2021

tsullivan Jul 29, 2021 •

edited

Loading

ppisljar Jul 29, 2021

tsullivan Jul 29, 2021

tsullivan commented Jul 29, 2021

tsullivan Jul 30, 2021

ppisljar left a comment

tsullivan commented Aug 2, 2021

kibanamachine commented Aug 2, 2021

API count

API count missing comments

API count with any type

Non-exported public API item count

[Reporting] add version to all export types job params #106137

[Reporting] add version to all export types job params #106137

Conversation

tsullivan commented Jul 19, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Jul 19, 2021

elasticmachine commented Jul 19, 2021

ppisljar left a comment

Choose a reason for hiding this comment

tsullivan commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan commented Jul 29, 2021

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

tsullivan commented Aug 2, 2021

kibanamachine commented Aug 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

API count

API count missing comments

API count with any type

Non-exported public API item count

History

tsullivan commented Jul 19, 2021 •

edited

Loading

tsullivan Jul 29, 2021 •

edited

Loading

tsullivan Jul 29, 2021 •

edited

Loading