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

[Reporting/CSV] Deprecate Download CSV, add a config flag to enable #178159

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Mar 6, 2024

Closes #164104

Summary

Replace "Download CSV" with "Generate CSV report" to export a CSV file from saved search panel, deprecate "Download CSV", use a config flag for providing the deprecated feature.

This PR uses the xpack.reporting.csv.enablePanelActionDownload kibana.yml setting, which was previously unused, for choosing behavior of CSV export in a Dashboard saved search panel, and sets the default value to false. The options allow the user to download a CSV file without creating a report (deprecated, support will be removed in the future) or to generate a CSV report (default).

  1. Use the config as a flag to switch between implementations:
    • downloading a CSV file without a generated report
    • generating a CSV report
  2. Updated documentation
  3. Refactored / cleaned up tests
  4. Increased API test coverage in Serverless
  5. Better error handling in packages/kbn-reporting/public/reporting_api_client.ts

Checklist

Delete any items that are not applicable to this PR.

Release Note

Kibana CSV Reporting offered a feature allowing users to download a CSV file from a saved search panel in a dashboard, without having a report generated. This feature is now deprecated. Now, when users need to access saved search data from a dashboard panel as CSV, a normal report will be generated. To access the deprecated functionality, you can add xpack.reporting.csv.enablePanelActionDownload: true to kibana.yml, but this ability will be removed in a future version of Kibana.

Copy link
Contributor

github-actions bot commented Mar 6, 2024

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@tsullivan tsullivan changed the title [Reporting/CSV] Config toggle to enable Download CSV for saved search… [Reporting/CSV] Config toggle to enable Download CSV Mar 6, 2024
@tsullivan tsullivan self-assigned this Mar 6, 2024
@tsullivan tsullivan added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Mar 6, 2024
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan force-pushed the reporting/fully-deprecate-download-csv branch 2 times, most recently from ba0da5e to 87f1801 Compare March 6, 2024 22:33
@tsullivan
Copy link
Member Author

/ci

1 similar comment
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan force-pushed the reporting/fully-deprecate-download-csv branch 2 times, most recently from b18c4dd to c96b333 Compare March 7, 2024 22:40
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan force-pushed the reporting/fully-deprecate-download-csv branch from c96b333 to 40b960f Compare March 8, 2024 21:46
@tsullivan tsullivan force-pushed the reporting/fully-deprecate-download-csv branch from d117dbc to b7c6e10 Compare March 8, 2024 22:12
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan marked this pull request as ready for review March 8, 2024 22:28
@tsullivan tsullivan requested review from a team as code owners March 8, 2024 22:28
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan marked this pull request as ready for review March 11, 2024 19:33
@tsullivan
Copy link
Member Author

/ci

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Tested the new implemention. Looks good!

I'd suggest to look into the number of snapshot additions. Seems excessive, maybe we could consider asserting something more specific (e.g number of rows + inline snapshot of the first two line)

@tsullivan
Copy link
Member Author

I'd suggest to look into the number of snapshot additions. Seems excessive, maybe we could consider asserting something more specific (e.g number of rows + inline snapshot of the first two line)

The many snapshot changes are due to:

  1. Previously, the bulk of testing CSV export was run through the Download CSV endpoint. After this PR, the functional tests server runs with that endpoint disabled, so the tests have been moved to run through the Generate CSV Report endpoint.
  2. Some of the request params for generating the CSV report were updated, as the format of the request parameters have changed since they were originally captured for the test. One big change was the addition of a 2nd sort field in the request.
  3. I am in favor of asserting less for the sake of not having huge bulk of text to match against. I will go with your suggestion to assert something more specific, such as the number of rows and first 2 lines of output. I think this will help the code overall.

@Dosant
Copy link
Contributor

Dosant commented Mar 21, 2024

Thanks for snapshots clean up 👏

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
reporting 115 122 +7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/reporting-export-types-csv-common 22 21 -1

Async chunks

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

id before after diff
reporting 72.1KB 71.2KB -1014.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 43.2KB 47.8KB +4.5KB
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/reporting-public 1 4 +3

References to deprecated APIs

id before after diff
@kbn/reporting-export-types-csv 0 8 +8
reporting 21 34 +13
total +21

Total ESLint disabled count

id before after diff
@kbn/reporting-public 1 4 +3

Unreferenced deprecated APIs

id before after diff
@kbn/reporting-public 0 1 +1

History

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

cc @tsullivan

@tsullivan tsullivan merged commit 2ff5410 into elastic:main Mar 23, 2024
17 checks passed
@tsullivan tsullivan deleted the reporting/fully-deprecate-download-csv branch March 23, 2024 01:28
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Mar 23, 2024
pheyos added a commit that referenced this pull request Mar 25, 2024
## Summary

This PR fixes the serverless reporting API integration tests for MKI
runs.

### The problem that we saw

After #178159 was merged, we saw the `Reporting Generate CSV` API
integration tests fail with `401` responses when running against MKI
projects. It turned out that the tests were using hard-coded credentials
that are only working locally / in CI - real serverless projects run
with a different set of username and password.

### How this PR fixes it

Remove hard-coded credentials from
`x-pack/test_serverless/shared/services/svl_reporting.ts` and instead
read them from the test config, which has the proper entries for local
and MKI runs.

I've also preemptively adjusted
`x-pack/test_serverless/functional/services/svl_reporting.ts` and
`x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts`
in the same way even though they not run in MKI projects today. This
will hopefully avoid the same set of problems when the tests eventually
get re-enabled for MKI runs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Reporting] Offer CSV Report Generation for saved search panel and deprecate CSV Download
6 participants