Skip to content

[Reporting-CSV Export] Re-write CSV Export using SearchSource#88303

Merged
tsullivan merged 62 commits intoelastic:masterfrom
tsullivan:reporting/searchsource-exporttype
Mar 16, 2021
Merged

[Reporting-CSV Export] Re-write CSV Export using SearchSource#88303
tsullivan merged 62 commits intoelastic:masterfrom
tsullivan:reporting/searchsource-exporttype

Conversation

@tsullivan
Copy link
Copy Markdown
Member

@tsullivan tsullivan commented Jan 14, 2021

Summary

Resolves #81079

This PR adds a new export type keyed by csv_searchsource which runs the query and data formatting through SearchSource methods.

The existing csv export type has already been labeled in the code as deprecated. This export type will have to remain throughout 7.x as it could be referenced in Watcher scripts in customer environments.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@tsullivan tsullivan force-pushed the reporting/searchsource-exporttype branch 6 times, most recently from 280d4b9 to 54c23fc Compare January 20, 2021 01:21
@tsullivan tsullivan force-pushed the reporting/searchsource-exporttype branch 7 times, most recently from 01eebe4 to 21b4cee Compare January 29, 2021 21:31
@tsullivan tsullivan added the zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Jan 29, 2021
@tsullivan tsullivan force-pushed the reporting/searchsource-exporttype branch 2 times, most recently from 56f7938 to 91c4372 Compare January 29, 2021 23:29
@tsullivan tsullivan changed the title reporting csv generation from searchSource (new export type) [Reporting-CSV Export] Re-write CSV Export using SearchSource Feb 1, 2021
@tsullivan tsullivan force-pushed the reporting/searchsource-exporttype branch 4 times, most recently from ab59e88 to 5d6550e Compare February 1, 2021 22:53
@tsullivan tsullivan force-pushed the reporting/searchsource-exporttype branch 2 times, most recently from e24a638 to 8777211 Compare February 2, 2021 00:41
@tsullivan tsullivan force-pushed the reporting/searchsource-exporttype branch from 8777211 to b13253d Compare February 2, 2021 17:23
@tsullivan
Copy link
Copy Markdown
Member Author

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

I had a quick look at this. When exporting a saved search with just the default column - Document the fields are not sorted. When exporting them from _source, they are sorted. I think we should make this consistent. I haven't had a look at the code yet, but one thing to note is that fields in the API response don't come sorted - they need to be sorted client-side.


const fields = fieldValues[fieldSource];
// Check if field name values are string[] and if the fields are user-defined
if (isPlainStringArray(fields)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we try to first parse fields from searchsource instead of always just using the ones in the table ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if its tabify dropping fields that don't exist in response (but do exist in searchsource) and discover is actually showing those fields, then we should probably update tabify.

Copy link
Copy Markdown
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
reporting 51 50 -1

Async chunks

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

id before after diff
discover 409.1KB 408.4KB -743.0B
reporting 34.6KB 34.6KB +1.0B
total -742.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 96.5KB 93.9KB -2.6KB

History

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

Copy link
Copy Markdown
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

I had a quick look at this, changes look good to me. I am not very familiar with the reporting code. Discover-related code LGTM 👍

@elasticmachine
Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment