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/Discover] include the document's entire set of fields #92730

Merged
merged 8 commits into from
Feb 25, 2021

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Feb 24, 2021

Summary

Fixes #92712

In 7.12, Discover may return an empty set as the list of fields selected, whereas in previous versions we would get an array with _source as a single element.

This PR revives the expected behavior when Discover triggers a CSV export and there are no columns selected for the search.

During development of this PR and testing Download CSV from a saved search Dashboard panel (different app):

  • A set of fields from the index pattern is not available on the server side. If no fields are selected, Download CSV would only include the auto-populated timeField column. Or if there is no timeField, it could possibly return an empty file as the CSV export
  • Meta fields (such as keyword sub-values of text fields) are not available when downloading the CSV from a dashboard panel.

The above issues will be fixed in 7.13 with #88303

New functional tests were added to verify the expected behavior in Discover. These tests were simply copied over from #88303

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@tsullivan tsullivan requested review from a team and wylieconlon February 24, 2021 22:42
@tsullivan tsullivan added v7.12.0 v7.13.0 v8.0.0 (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes Team:AppServices labels Feb 24, 2021
@elasticmachine
Copy link
Contributor

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

@@ -19,7 +19,10 @@ const getSharingDataFields = async (
timeFieldName: string,
hideTimeColumn: boolean
) => {
if (selectedFields.length === 1 && selectedFields[0] === '_source') {
if (
selectedFields.length === 0 ||
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the bug that was immediately noticed: only the date field is populated when exporting a new search (if no other fields are selected as columns)


logger.debug(`Execute job generating [${visType}] csv`);
logger.debug(`Execute job generating saved search CSV`);
Copy link
Member Author

Choose a reason for hiding this comment

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

logging cleanup: the visType variable was undefined

@@ -75,6 +77,14 @@ export const getGenerateCsvParams = async (
fields: indexPatternFields,
} = indexPatternSavedObject;

if (!indexPatternFields || indexPatternFields.length === 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

logging addition for the open issues with Download CSV from a dashboard panel (to be fixed in #88303)

const fromTime = 'Sep 19, 2015 @ 06:31:44.000';
const toTime = 'Sep 19, 2015 @ 18:01:44.000';
const fromTime = 'Apr 27, 2019 @ 23:56:51.374';
const toTime = 'Aug 23, 2019 @ 16:18:51.821';
Copy link
Member Author

Choose a reason for hiding this comment

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

These values have been wrong for a long time, but in this PR it matters because the content of the CSV is actually checked against a snapshot.

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

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 tested this change and found this fixes the issue and doesn't affect _source fetching. The new tests look safe to me.

@@ -69,14 +69,83 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
await PageObjects.reporting.setTimepickerInDataRange();
await PageObjects.discover.saveSearch('my search - with data - expectReportCanBeCreated');
await PageObjects.reporting.openCsvReportingPanel();
expect(await PageObjects.reporting.canReportBeCreated()).to.be(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, so we were previously testing that reporting is available, but not the contents of the reports.

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@wylieconlon
Copy link
Contributor

@tsullivan you have actual type errors in the code, it's not flaky

@kibanamachine
Copy link
Contributor

💚 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 401.0KB 401.0KB +27.0B

History

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

tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 25, 2021
…stic#92730)

* fix get_sharing_data to provide a list of fields if none are selected as columns in the saved search

* add logging for download CSV that fields must be selected as columns

* add more functional test coverage

* fix ts in test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 25, 2021
…stic#92730)

* fix get_sharing_data to provide a list of fields if none are selected as columns in the saved search

* add logging for download CSV that fields must be selected as columns

* add more functional test coverage

* fix ts in test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
tsullivan added a commit that referenced this pull request Feb 25, 2021
) (#92922)

* fix get_sharing_data to provide a list of fields if none are selected as columns in the saved search

* add logging for download CSV that fields must be selected as columns

* add more functional test coverage

* fix ts in test

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
tsullivan added a commit that referenced this pull request Feb 25, 2021
) (#92921)

* fix get_sharing_data to provide a list of fields if none are selected as columns in the saved search

* add logging for download CSV that fields must be selected as columns

* add more functional test coverage

* fix ts in test

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 26, 2021
…bana into task-manager/docs-monitoring

* 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: (40 commits)
  [Security Solution][Case][Bug] Improve case logging (elastic#91924)
  [Alerts][Doc] Added README documentation for alerts plugin status and framework health checks configuration options. (elastic#92761)
  Add warning for EQL and Threshold rules if exception list contains value list items (elastic#92914)
  [Security Solution][Case] Fix subcases bugs on detections and case view (elastic#91836)
  [APM] Always allow access to Profiling via URL (elastic#92889)
  [Vega] Allow image loading without CORS policy by changing the default to crossOrigin=null (elastic#91991)
  skip flaky suite (elastic#92114)
  [APM] Fix for default fields in correlations view (elastic#91868) (elastic#92090)
  chore(NA): bump bazelisk to v1.7.5 (elastic#92905)
  [Maps] fix selecting EMS basemap does not populate input (elastic#92711)
  API docs (elastic#92827)
  [kbn/test] add import/export support to KbnClient (elastic#92526)
  Test fix management scripted field filter functional test and unskip it  (elastic#92756)
  [App Search] Create Curation view/functionality (elastic#92560)
  [Reporting/Discover] include the document's entire set of fields (elastic#92730)
  [Fleet] Add new index to fleet for artifacts being served out of fleet-server (elastic#92860)
  [Alerts][Doc] Added README documentation for API key invalidation configuration options. (elastic#92757)
  [Discover][docs] Add search for relevance (elastic#90611)
  [Alerts][Docs] Extended README.md and the user docs with the licensing information. (elastic#92564)
  [7.12][Telemetry] Security telemetry allowlist fix. (elastic#92850)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] CSV report is missing the entire document when using fields API
4 participants