Reporting: Fix _index and _id columns in CSV export #96097

tsullivan · 2021-04-01T20:56:15Z

Summary

Elasticsearch has a bugfix change in 7.13 that invalidates certain metadata fields from being allowed in the fields API of a search request. Previously these fields would be ignored. See elastic/elasticsearch#70575

Kibana CSV Reporting populates the list of fields in the searchSource object with the columns selected in Discover. Since users can select metadata fields in Discover, it opened up the CSV export code to have metadata fields to exist in the list of fields in the searchSource. It did not matter that these fields were ignored by Elasticsearch. The set of fields were later used when generating the CSV to get the order of the columns.

This PR addresses the issue that Elasticsearch now throws an error when invalid metadata fields are passed in the fields API of a search. JobParamsCSV, which is the job payload for CSV report, previously only featured a searchSource object. A new columns string array is added to the params in this PR, which provides CSV generation with the selection and ordering of the columns. The columns param can contain the ID any field that is selectable in Discover, including metadata fields.

This screenshot of a select area of the code diff illustrates the change to the job params, in some mock code:

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
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

tsullivan · 2021-04-05T17:02:04Z

@elasticmachine merge upstream

elasticmachine · 2021-04-05T23:05:07Z

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

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

src/plugins/discover/public/application/helpers/get_sharing_data.ts

dokmic

LGTM, great job! I left a few minor suggestions.

…generate_csv/generate_csv.ts Co-authored-by: Michael Dokolin <[email protected]>

tsullivan · 2021-04-07T18:41:52Z

@elasticmachine merge upstream

ppisljar · 2021-04-08T14:51:38Z

src/plugins/discover/public/application/helpers/get_sharing_data.ts

+import { getSortForSearchSource } from '../angular/doc_table';
+
+export interface ISharingData {
+  columns: string[];


lets make this optional

never mind i see its optional on the report type

ppisljar

code LGTM

tsullivan · 2021-04-09T01:39:32Z

@elasticmachine merge upstream

tsullivan · 2021-04-12T16:33:26Z

@elasticmachine merge upstream

kertal · 2021-04-12T17:09:31Z

src/plugins/discover/public/shared/index.ts

@@ -12,3 +12,5 @@
 export async function loadSharingDataHelpers() {
  return await import('../application/helpers/get_sharing_data');
 }
+
+export { ISharingData } from '../application/helpers/get_sharing_data';


just a quick look, it's late and my local Kibana refuses to bootstrap. could you check the build stats of this PR once it's finished, to be sure there is no significant change because of this? I can review it tomorrow for sure

Sure -- nothing actually imports this interface, but Typescript complained without the export

@kertal - I noticed that this PR added an extra 25K to the discover page load bundle, which is unexpected. I added fc16f85 and watching for how the bundle sizes changes in CI metrics.

fc16f85 brought the discover page load bundle size back down. Thanks for the tip @kertal!

even better, this PR saves a few bytes now!

kibanamachine · 2021-04-13T00:35:59Z

💚 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
`discover`	397.0KB	396.7KB	-386.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`	86.8KB	86.8KB	-25.0B

History

💚 Build #118985 succeeded a879667
💔 Build #118836 failed 537f44e
💚 Build #118484 succeeded 8679a09
💚 Build #118056 succeeded 0729616
💔 Build #118000 failed dbac397

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

kertal

Code LGTM, tested locally, CSV export with _index and _id works again 👍 , thx for taking care of the bundle size

* Reporting: Fix _index and _id columns in CSV export * optimize - cache _columns and run getColumns once per execution * Update x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts Co-authored-by: Michael Dokolin <[email protected]> * feedback * fix typescripts * fix plugin list test * fix plugin list * take away the export interface to test CI build stats Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Dokolin <[email protected]>

* Reporting: Fix _index and _id columns in CSV export * optimize - cache _columns and run getColumns once per execution * Update x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts Co-authored-by: Michael Dokolin <[email protected]> * feedback * fix typescripts * fix plugin list test * fix plugin list * take away the export interface to test CI build stats Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Dokolin <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Michael Dokolin <[email protected]>

tsullivan force-pushed the tests/reporting-unskip-5 branch 3 times, most recently from ec4630e to 51dd5da Compare April 2, 2021 19:47

tsullivan added 2 commits April 2, 2021 13:38

Reporting: Fix _index and _id columns in CSV export

701416d

optimize - cache _columns and run getColumns once per execution

7889a86

tsullivan force-pushed the tests/reporting-unskip-5 branch from 51dd5da to 7889a86 Compare April 2, 2021 20:51

Merge branch 'master' into tests/reporting-unskip-5

b3b5e0d

tsullivan mentioned this pull request Apr 5, 2021

Failing ES Promotion: dashboard Reporting Download CSV Field Formatters and Scripted Fields Download CSV export of a saved search panel #96000

Closed

tsullivan added the v8.0.0 label Apr 5, 2021

Merge branch 'master' into tests/reporting-unskip-5

c3bcf7b

tsullivan marked this pull request as ready for review April 5, 2021 23:04

tsullivan requested a review from a team April 5, 2021 23:04

tsullivan added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:AppServices labels Apr 5, 2021

dokmic reviewed Apr 7, 2021

View reviewed changes

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts Outdated Show resolved Hide resolved

dokmic reviewed Apr 7, 2021

View reviewed changes

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts Outdated Show resolved Hide resolved

dokmic reviewed Apr 7, 2021

View reviewed changes

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts Outdated Show resolved Hide resolved

dokmic reviewed Apr 7, 2021

View reviewed changes

src/plugins/discover/public/application/helpers/get_sharing_data.ts Outdated Show resolved Hide resolved

dokmic approved these changes Apr 7, 2021

View reviewed changes

Update x-pack/plugins/reporting/server/export_types/csv_searchsource/…

e38f8b6

…generate_csv/generate_csv.ts Co-authored-by: Michael Dokolin <[email protected]>

kibanamachine and others added 4 commits April 7, 2021 14:41

Merge branch 'master' into tests/reporting-unskip-5

1aab963

feedback

dbac397

Merge branch 'master' into tests/reporting-unskip-5

fdc22a2

fix typescripts

0729616

ppisljar reviewed Apr 8, 2021

View reviewed changes

ppisljar approved these changes Apr 8, 2021

View reviewed changes

Merge branch 'master' into tests/reporting-unskip-5

8679a09

Merge branch 'master' into tests/reporting-unskip-5

537f44e

kertal reviewed Apr 12, 2021

View reviewed changes

tsullivan added 3 commits April 12, 2021 13:02

fix plugin list test

0977035

Merge branch 'master' into tests/reporting-unskip-5

e972b9e

fix plugin list

a879667

This was referenced Apr 12, 2021

Reporting: Refactor functional tests with security roles checks #96856

Merged

[Reporting] Kibana Application Privileges for Reporting #94966

Merged

take away the export interface to test CI build stats

fc16f85

kertal approved these changes Apr 13, 2021

View reviewed changes

tsullivan merged commit f67f0e8 into elastic:master Apr 13, 2021

tsullivan deleted the tests/reporting-unskip-5 branch April 13, 2021 15:03

tsullivan mentioned this pull request Apr 13, 2021

[7.x] Reporting: Fix _index and _id columns in CSV export (#96097) #97009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reporting: Fix _index and _id columns in CSV export #96097

Reporting: Fix _index and _id columns in CSV export #96097

tsullivan commented Apr 1, 2021 •

edited

Loading

tsullivan commented Apr 5, 2021

elasticmachine commented Apr 5, 2021

dokmic left a comment

tsullivan commented Apr 7, 2021

ppisljar Apr 8, 2021

ppisljar Apr 8, 2021

ppisljar left a comment

tsullivan commented Apr 9, 2021

tsullivan commented Apr 12, 2021

kertal Apr 12, 2021

tsullivan Apr 12, 2021

tsullivan Apr 12, 2021

tsullivan Apr 13, 2021

kertal Apr 13, 2021

kibanamachine commented Apr 13, 2021

kertal left a comment

Reporting: Fix _index and _id columns in CSV export #96097

Reporting: Fix _index and _id columns in CSV export #96097

Conversation

tsullivan commented Apr 1, 2021 • edited Loading

Summary

Checklist

For maintainers

tsullivan commented Apr 5, 2021

elasticmachine commented Apr 5, 2021

dokmic left a comment

Choose a reason for hiding this comment

tsullivan commented Apr 7, 2021

ppisljar Apr 8, 2021

Choose a reason for hiding this comment

ppisljar Apr 8, 2021

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

tsullivan commented Apr 9, 2021

tsullivan commented Apr 12, 2021

kertal Apr 12, 2021

Choose a reason for hiding this comment

tsullivan Apr 12, 2021

Choose a reason for hiding this comment

tsullivan Apr 12, 2021

Choose a reason for hiding this comment

tsullivan Apr 13, 2021

Choose a reason for hiding this comment

kertal Apr 13, 2021

Choose a reason for hiding this comment

kibanamachine commented Apr 13, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

tsullivan commented Apr 1, 2021 •

edited

Loading