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

[Lens] CSV Export for Lens #83430

Merged
merged 29 commits into from
Nov 24, 2020
Merged

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 16, 2020

Summary

First implementation of the CSV export feature for Lens.

  • Implement a first draft of the datatableToCsv function
    • First implementation based on the inspector one
    • Should support multiple layers (naming logic)
    • Should support formatters from the Datatable field hints
    • Should live integrated as data plugin + download share plugin
    • Add an option for string export to make the function testable?
  • Hook to the onData$ callback and pass the activeData to the app, which in turns shows the button
    • Display a button to download the data (show/hide logic)
    • Make the button download the data
    • Hook to settings to define separators/other options

Note: the embeddable action will be implemented in a subsequent PR.

Fixes part of #74509

Screenshot 2020-11-16 at 15 49 13

download_csv_multiple

Open questions:

  • at the moment, in case of multiple layers, each csv file will have a postfix with an index value (i.e. filename-N.csv). In [Lens] CSV export #74509 the suggestion was to use the postfix -Layer N (=> filename-Layer N.csv), which is a valid option. Is the - Layer N postfix the final decision on that? ( Change in later on will be easy )

Possible enhancement discussion (out of scope for this PR, but worth discussing):

  • A possible improvement on this side, as proposed in the linked issue, would be to have layers with names to use as postfix.
  • Multiple csv download is quite tricky, perhaps a possible future enhancement could be to download a zip file rather than multiple files?
  • Define a UI to make the User choose between formatted or raw values?

Checklist

Delete any items that are not applicable to this PR.

@dej611
Copy link
Contributor Author

dej611 commented Nov 17, 2020

Made the Download CSV button stay on the top nav all the time with enable/disable styling: on bootstrap I saw it blinking with the previous behaviour, so I moved to this other option which is less "noisy".
cc @MichaelMarcialis

@dej611 dej611 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Nov 18, 2020
@dej611 dej611 marked this pull request as ready for review November 18, 2020 17:35
@dej611 dej611 requested a review from a team November 18, 2020 17:35
@dej611 dej611 requested a review from a team as a code owner November 18, 2020 17:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@dej611
Copy link
Contributor Author

dej611 commented Nov 19, 2020

@elasticmachine merge upstream

*
* @returns undefined (download) - Record\<string, string\> (only for testing)
*/
export function exportAsCSVs(
Copy link
Member

Choose a reason for hiding this comment

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

lets change this function so it doesn't download the file but just returns csv as a string, this way we can move the function to common and make it usable from server.

the download aspect of it should be moved to some other place (possibly share plugin) so that can be used for other things as well, as that download function would now just take filename and filecontent in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You proposing splitting this into two exportCSVs + download plugins?

Copy link
Member

Choose a reason for hiding this comment

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

yes, two functions as converting to csv can be common and downloading can be used for other things beside csv as well.

@dej611
Copy link
Contributor Author

dej611 commented Nov 23, 2020

@elasticmachine merge upstream

raw?: boolean;
}

function buildCSV(
Copy link
Member

Choose a reason for hiding this comment

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

lets rename this one to datatableToCSV and export it

*
* @returns A dictionary of files to download: the key is the filename and the value the CSV string
*/
export function exportAsCSVs(
Copy link
Member

Choose a reason for hiding this comment

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

i am wondering if this function should be kept inside lens for now. how often will we want to convert multiple datatables at once ? also i am wondering if the reference to filename here should be omited but rather just use the datatable name ? @lukeelmers whats your opinion ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mind that the DataTable type has no name property. The key:string passed for the record may be just internal ids (for instance in Lens it represents the layer id).

Copy link
Member

Choose a reason for hiding this comment

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

As we move down the path of creating these various utility functions for datatables, I think it's important to have some consistency in how you interact with them. For any datatable utility function, I'd expect an interface similar to the following:

type SomeFn = (table: Datatable, options?: Record<string, whatever>) => Promise<Something> | Something;

how often will we want to convert multiple datatables at once ?

For the sake of consistency, I would vote to make this function operate on a single table just like our other datatable helpers.

i am wondering if the reference to filename here should be omited but rather just use the datatable name

I think the concept of a filename is only here because the file download functionality was extracted from this function originally. IMHO it would make more sense to remove it and handle it when calling the download helper from the share plugin.

This would make the interface something like:

exportAsCsv(table: Datatable, options: CSVOptions) => string;

Then Lens would handle calling it multiple times (for multiple tables), assigning a filename, and passing the output to the download helper.

To me this feels like the best way to make these utilities as decoupled as possible, and keeps the lens-specific stuff in lens. (And of course we can always revisit down the road if some of the lens logic is needed for a bunch of other apps)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Firefox and works well for me. It would be super cool to have a functional test for that but I'm not sure whether that's possible with our current setup.

@@ -27,6 +28,7 @@ export interface Document {
state?: unknown;
};
filters: PersistableFilter[];
activeData?: TableInspectorAdapter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is a leftover

@@ -41,5 +41,6 @@ export {
import { SharePlugin } from './plugin';

export { KibanaURL } from './kibana_url';
export * from './lib/download_as';
Copy link
Member

Choose a reason for hiding this comment

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

nit: We prefer to explicitly export named items from the top-level public/index.ts (instead of using export *)... that way we avoid accidentally leaking internal items into the public contract later.

*
* @returns A dictionary of files to download: the key is the filename and the value the CSV string
*/
export function exportAsCSVs(
Copy link
Member

Choose a reason for hiding this comment

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

As we move down the path of creating these various utility functions for datatables, I think it's important to have some consistency in how you interact with them. For any datatable utility function, I'd expect an interface similar to the following:

type SomeFn = (table: Datatable, options?: Record<string, whatever>) => Promise<Something> | Something;

how often will we want to convert multiple datatables at once ?

For the sake of consistency, I would vote to make this function operate on a single table just like our other datatable helpers.

i am wondering if the reference to filename here should be omited but rather just use the datatable name

I think the concept of a filename is only here because the file download functionality was extracted from this function originally. IMHO it would make more sense to remove it and handle it when calling the download helper from the share plugin.

This would make the interface something like:

exportAsCsv(table: Datatable, options: CSVOptions) => string;

Then Lens would handle calling it multiple times (for multiple tables), assigning a filename, and passing the output to the download helper.

To me this feels like the best way to make these utilities as decoupled as possible, and keeps the lens-specific stuff in lens. (And of course we can always revisit down the road if some of the lens logic is needed for a bunch of other apps)

* under the License.
*/

export * from './export_csv';
Copy link
Member

Choose a reason for hiding this comment

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

If we make my proposed changes to exportAsCsv, then we could move this all to the common directory. (Maybe common/utils?) and export it from both public/index.ts and server/index.ts

* Exporters (CSV)
*/

export * from './exports';
Copy link
Member

Choose a reason for hiding this comment

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

Same comment re: using named exports from top-level public/index.ts and server/index.ts

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Thanks for the edits @dej611! Code changes here LGTM pending green build. Let's wait to make sure @ppisljar has one last look at it before it merges.

Copy link
Member

@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

@dej611
Copy link
Contributor Author

dej611 commented Nov 24, 2020

It would be super cool to have a functional test for that but I'm not sure whether that's possible with our current setup.

@flash1293 added basic functional test :)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 601 603 +2
share 39 47 +8
total +10

Async chunks

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

id before after diff
lens 935.0KB 936.6KB +1.6KB

Distributable file count

id before after diff
default 43058 43060 +2
oss 22583 22585 +2

Page load bundle

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

id before after diff
data 974.3KB 976.0KB +1.7KB
lens 52.4KB 52.6KB +232.0B
share 70.4KB 81.2KB +10.8KB
total +12.7KB

History

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

@dej611 dej611 merged commit 423888c into elastic:master Nov 24, 2020
@dej611 dej611 deleted the feature/lens/csv-export branch November 24, 2020 11:37
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 26, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 83430 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 83430 or prevent reminders by adding the backport:skip label.

dej611 added a commit to dej611/kibana that referenced this pull request Nov 30, 2020
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/data/server/server.api.md
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 30, 2020
dej611 added a commit that referenced this pull request Nov 30, 2020
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@flash1293 flash1293 mentioned this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants