-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Restore the "csv by savedobject" endpoint for 7.17 #148030
[Reporting] Restore the "csv by savedobject" endpoint for 7.17 #148030
Conversation
08e6019
to
7b85537
Compare
b44804c
to
493f57d
Compare
493f57d
to
6250b2b
Compare
…owser (#148226) ## Summary This change allows developers on Mac to use the CSV generation APIs in 7.17. However, screenshot-type reports will continue to not be supported. In 7.17, there was no Kibana Reporting support for the Mac ARM processor. Now that many developers use that type of computer, they may be blocked from using any Reporting API at all, including CSV generation APIs. This change makes a correction to not block access to Reporting APIs when there is not registered headless browser. If a user attempts to generate a report in 7.17 on a server running an unsupported browser, they will receive an error message. Unblocks: #148030
6250b2b
to
58864f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
del
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a bunch of different request scenarios and everything worked well. great job.
A scenario I was thinking of and didn't test is if Kibana was upgraded from <7.3 where we had this api, then to <7.17 where we removed it, and then to this branch where we have it again. Have you tested? Do you anticipate any issues with Kibana bootstrap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for re-adding this functionality! Hopefully it can get some of our users out of any bind they may be in.
const searchSourceFields: ParsedSearchSourceJSON = parseSearchSourceJSON( | ||
savedSearch.attributes.kibanaSavedObjectMeta.searchSourceJSON | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think a bit ahead here. As I understand this team "owns" the "search source" object but not the "search" object?
When we have support for "down" and "on-read" migrations of saved objects we can consider pinning this object to a specific version of "search", but also a specific version of "searchsource" to ensure it is always in the shape we expect.
This is nothing to action on this PR, I'd be interested to hear what @pgayvallet or @rudolf think :)
I made an attempt to try this test, but I wasn't able to do so successfully. In 7.3-7.8, the API integration test was skipped, and the code seemed to be broken. I'm waiting for some information to find out if consumers that used this were using a fork of Kibana that had this fixed. When I find out, I plan to run this test case (after merging this PR) to make sure the upgrade is seamless for the consumers.
There are no new dependencies or changes to the build process in this PR. These changes were carefully done to ensure that nothing outside of the restored API can be affected. Let me know if that helps answer this question. @jloleysens @pgayvallet @rudolf
There are some tough choices that had to be made in this PR, since the feature is a combination of functionality from across different domains. In the 7.17 branch, those domains are not great at exposing the functionality that needs to be shared. As you see in the changes, that led to a compromise to copy code from other domains into I will draft a design document for a no-compromise implementation of this API. The design will propose having the having the appropriate domains share the functionality that is needed, and handle "on-read" migrations before sharing, which is what I think @jloleysens is suggesting. On the topic of domains, I would also add that code in |
I got a patch to test running Kibana with, and tested the API in 7.7. That testing led to the latest fix, which would have been a bug present with older saved search objects. |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
## Summary In 7.17.9, we're restoring a report generation API endpoint to create CSV reports based on saved searches. There has been one PR that serves as the first iteration: #148030 This resolves a missing capability from the first iteration. In the first iteration, POST bodies could allow an additional time range filter, which is merged with any saved filters or queries stored in the saved search object: ``` POST /api/reporting/v1/generate/csv/saved-object/search:${savedSearchId} { timerange: { min: '2015-09-20 10:23:36.052', max: '2015-09-20 10:25:55.744' } } ``` This PR is a second iteration. It allows additional "unsaved state" to be merged with the saved object at the time of report generation. ``` POST /api/reporting/v1/generate/csv/saved-object/search:${savedSearchId} state": { "query": { "multi_match": { "type": "best_fields", "query": "cognac", "lenient": true } }, "timerange": <same as before> ``` ``` POST /api/reporting/v1/generate/csv/saved-object/search:${savedSearchId} "state": { "query": [ { "multi_match": { "type": "best_fields", "query": "cognac", "lenient": true } }, { "bool": { "must_not": { "multi_match": { "type": "best_fields", "query": "Pyramidustries", "lenient": true } } } } ], timerange": <same as before> ``` ### Details In the details of #148030, it was stated: > Does not allow "raw state" to be merged with the Search object, as in the previous code (from 7.3-7.8). Otherwise, the API is compatible with the previous code. This PR pushes a bit back against that limitation. Now, requests to generate a report can accept a `state` field in the POST body. This field contains additional "unsaved state" that gets merged with the contents of the stored saved search object. However, the entire functionality of allowing `sort` and `docvalue_fields` keys in the request, is still not restored from the functionality that was implemented in 7.3-7.8. This limitation exists to minimize the complexity of the restored endpoint. Both of the non-restored keys are related to the sorting of documents. The sorting of documents is controlled by the saved search object only. The user can change the sort of the CSV after downloading the report, in a spreadsheet application or by programmatically working on the file. ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Summary
This restores an endpoint that was added in 7.3 in this PR, and was removed in 7.9 in this PR. The changes are re-done on top of 7.17, but still has a mostly-compatible with the one that existed in 7.3-7.8. This serves 3rd parties that relied on the earlier experimental code.
Supports:
LIMITATIONS:
main
in a way that uses a different API that is not compatible with this change.Testing
Since there is not a UI for this endpoint, there are a few options for testing:
node scripts/functional_tests.js \ --config x-pack/test/reporting_api_integration/reporting_and_security.config.ts \ --grep 'CSV Generation from Saved Search ID'