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

[Embeddable] Export CSV action for Lens embeddables in dashboard #83654

Merged
merged 70 commits into from
Dec 3, 2020

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 18, 2020

Summary

This PR is built on top of #83430 , adding a new Action to make the CSV export available also from the dashboard environment.

Note wait to #83430 to be merged before merging this.

Note2: one big change in this PR is the share plugin moved to the required list for the dashboard plugin. That is due to the new download method required in combination with the CSV export. I'm open to alternatives here ( cc @timroes , @ThomThomson )

Plan:

  • Create a basic export CSV action for Lens
    • Very basic label for the action
  • Wire up the action for Lens embeddables and make it work with the new export data plugin
    • Read global settings from uiSettings
    • Get field formatters
    • Check it still download multiple csvs
  • Move the plugin to a shared location rather than Lens
    • Moved to dashboard actions folder for now (some pending issue with that)
  • Make it work for other embeddables?
    • PoC for Visualize embeddables (make CSV download work)
    • Check Vega embeddables (disable the action for them)
    • Any other type of embeddable to test?
  • Testing
    • Add some basic action unit testing
      • Use another Embeddable with datatable support rather than CONTACT_CARD_EMBEDDABLE
    • Add basic functional tests

download_csv_action

Some assumptions:

  • currently the export CSV is downloading formatted values by default. Even in the Visualize embeddable
  • the plugin supports the raw value export as well, this is just not used
  • Adapters is a generic container as for now, so a little bit of duck typing is required to understand where's the datatable

Checklist

dej611 and others added 30 commits November 16, 2020 11:15
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.

Left two nits that should be addressed, but I don't think it needs another round of review from my side, LGTM.

I want to note however this is causing the context menu in "View" mode to be shown using a more link.
Before:
Screenshot 2020-12-01 at 11 46 53

After:
Screenshot 2020-12-01 at 11 43 59

Feedback from @elastic/kibana-presentation appreciated on this.

@@ -54,6 +56,7 @@ export type LensByValueInput = {
export type LensByReferenceInput = SavedObjectEmbeddableInput & EmbeddableInput;
export type LensEmbeddableInput = (LensByValueInput | LensByReferenceInput) & {
palette?: PaletteOutput;
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.

Is this a leftover? Seems like activeData is never actually passed in as embeddable input (and I don't see a good reason for that).

@@ -84,6 +87,7 @@ export class Embeddable
private subscription: Subscription;
private autoRefreshFetchSubscription: Subscription;
private isInitialized = false;
private activeData: TableInspectorAdapter | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be LensInspectorAdapters now

@dej611
Copy link
Contributor Author

dej611 commented Dec 1, 2020

@elasticmachine merge upstream

@ThomThomson
Copy link
Contributor

RE. the placement of this action & the 'More' section. The more item will be needed even in view mode soon because we will be adding a share action to panels, so no worries there.

As for the order, seems like Download as CSV belongs in the more section. Looking at the order variables, 'maximize panel' is 7, so an order of 5 could potentially work for the new download action. Will do a full review later today.

Copy link
Contributor

@ThomThomson ThomThomson 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

Tested locally via downloading CSVs from Lens embeddables. Works with both by value & by reference embeddables, and shows the correct data.

One small problem I noticed when downloading a CSV from an embeddable by value which doesn't have a title is that it downloads as csv.txt by default. Maybe a fallback title should be added, so in this case it would be downloaded as untitled.csv or something similar.

constructor(protected readonly params: Params) {}

public getIconType() {
return 'exportAction';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the 'download' icon would better suit this action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion here.
I think I've picked the same icon used in inspector, but I can change it if you have strong opinion here.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the code, but I have some comments that I'd like to get addressed before merging:

  • Unlike the "Inspect" action, this action appears to only be available in Edit mode. I would have expected that I could download CSVs any time I'm viewing a dashboard
  • Please make sure the title of this PR is suitable to be put in the release notes, describing the new feature in one sentence
  • I think this feature needs documentation, and we should be doing this as part of the same PR in most cases. Specifically, I would expect a new section in the existing docs called Explore dashboard data about how to download CSV data from Lens.

Edit: There are some other frequently asked questions that we get about CSV export, and maybe we can answer this in the docs:

  • Can the entire dashboard be exported as CSV?
  • What can or can't be exported?
  • What is the license level of this feature?
  • Is there a way to automatically export using the Kibana API?
  • What happens with very large CSVs?

@flash1293
Copy link
Contributor

flash1293 commented Dec 2, 2020

Unlike the "Inspect" action, this action appears to only be available in Edit mode. I would have expected that I could download CSVs any time I'm viewing a dashboard

Not sure why you assume this, @wylieconlon - I just checked the latest version and downloading csvs works just fine in view mode as well.
Screenshot 2020-12-02 at 09 37 27

About the docs in general - I totally agree we should document this and answer the questions you raised (thanks for providing these), but given we are close to feature freeze I'm wondering whether it's required in this case? Maybe @KOTungseth can take a pass over new Lens features after feature freeze and extend the documentation where it makes sense? Based on the timing I'm concerned about iterating over documentation as part of this PR (I'm expecting we wouldn't nail it the very first time) because we can add documentation outside of the regular release cycle, but we can't add the feature itself in the same way.

@dej611 dej611 changed the title [Embeddable] Export CSV actions for Datatables-ready embeddables [Embeddable] Export CSV action for Lens embeddables in dashboard Dec 2, 2020
@dej611
Copy link
Contributor Author

dej611 commented Dec 2, 2020

Edit: There are some other frequently asked questions that we get about CSV export, and maybe we can answer this in the docs:

  • Can the entire dashboard be exported as CSV?
  • What can or can't be exported?
  • What is the license level of this feature?
  • Is there a way to automatically export using the Kibana API?
  • What happens with very large CSVs?

While I see some overlap here, most of these questions are really out of scope for this PR. Maybe it could be noted as part of the #81780 plan (at least for the Lens part)

@dej611
Copy link
Contributor Author

dej611 commented Dec 2, 2020

One small problem I noticed when downloading a CSV from an embeddable by value which doesn't have a title is that it downloads as csv.txt by default. Maybe a fallback title should be added, so in this case it would be downloaded as untitled.csv or something similar.

Good catch! It used to work, but probably with some refactor it went lost. Just fixed! :)

@dej611
Copy link
Contributor Author

dej611 commented Dec 2, 2020

Pushed a first draft of the possible documentation for the action: I've mostly based it on other articles around it. Let me know if it can be improved somehow.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

LGTM! I was able to find the CSV export in both dashboard modes now- I may have been looking in the wrong place before. The new docs stub looks good.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 186 187 +1
embeddable 99 101 +2
total +3

Async chunks

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

id before after diff
lens 968.6KB 968.8KB +273.0B

Page load bundle

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

id before after diff
dashboard 322.2KB 327.8KB +5.6KB
embeddable 221.6KB 225.7KB +4.1KB
total +9.7KB

History

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

@dej611 dej611 merged commit 17d986e into elastic:master Dec 3, 2020
@dej611 dej611 deleted the feature/dashboard/export-csv-action branch December 3, 2020 09:46
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 3, 2020
* master: (40 commits)
  fix: 🐛 don't add separator befor group on no main items (elastic#83166)
  [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323)
  [APM] Add APM agent config options (elastic#84678)
  Fixed a11y issue on rollup jobs table selection (elastic#84567)
  [Discover] Refactor getContextUrl to separate file (elastic#84503)
  [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654)
  [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749)
  [Lens] Migrate legacy es client and remove total hits as int (elastic#84340)
  Improve logging pipeline in @kbn/legacy-logging (elastic#84629)
  Catch @hapi/podium errors (elastic#84575)
  [Discover] Unskip date histogram test (elastic#84727)
  Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791)
  [Enterprise Search] Fix schema errors button (elastic#84842)
  [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589)
  [Maps] Always initialize routes on server-startup (elastic#84806)
  [Fleet] EPM support to handle uploaded file paths (elastic#84708)
  [Snapshot Restore] Fix initial policy form state (elastic#83928)
  Upgrade Node.js to version 14 (elastic#83425)
  [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992)
  [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 3, 2020
* master: (236 commits)
  fix: 🐛 don't add separator befor group on no main items (elastic#83166)
  [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323)
  [APM] Add APM agent config options (elastic#84678)
  Fixed a11y issue on rollup jobs table selection (elastic#84567)
  [Discover] Refactor getContextUrl to separate file (elastic#84503)
  [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654)
  [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749)
  [Lens] Migrate legacy es client and remove total hits as int (elastic#84340)
  Improve logging pipeline in @kbn/legacy-logging (elastic#84629)
  Catch @hapi/podium errors (elastic#84575)
  [Discover] Unskip date histogram test (elastic#84727)
  Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791)
  [Enterprise Search] Fix schema errors button (elastic#84842)
  [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589)
  [Maps] Always initialize routes on server-startup (elastic#84806)
  [Fleet] EPM support to handle uploaded file paths (elastic#84708)
  [Snapshot Restore] Fix initial policy form state (elastic#83928)
  Upgrade Node.js to version 14 (elastic#83425)
  [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992)
  [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 3, 2020
…overy-action-group

* upstream/master: (48 commits)
  [Lens] accessibility screen reader issues (elastic#84395)
  [Logs UI] Fetch single log entries via a search strategy (elastic#81710)
  fix: 🐛 don't add separator befor group on no main items (elastic#83166)
  [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323)
  [APM] Add APM agent config options (elastic#84678)
  Fixed a11y issue on rollup jobs table selection (elastic#84567)
  [Discover] Refactor getContextUrl to separate file (elastic#84503)
  [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654)
  [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749)
  [Lens] Migrate legacy es client and remove total hits as int (elastic#84340)
  Improve logging pipeline in @kbn/legacy-logging (elastic#84629)
  Catch @hapi/podium errors (elastic#84575)
  [Discover] Unskip date histogram test (elastic#84727)
  Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791)
  [Enterprise Search] Fix schema errors button (elastic#84842)
  [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589)
  [Maps] Always initialize routes on server-startup (elastic#84806)
  [Fleet] EPM support to handle uploaded file paths (elastic#84708)
  [Snapshot Restore] Fix initial policy form state (elastic#83928)
  Upgrade Node.js to version 14 (elastic#83425)
  ...
dej611 added a commit that referenced this pull request Dec 3, 2020
#83654) (#84885)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame 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.

8 participants