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

Sort server-side in SavedObject export #55128

Merged
merged 5 commits into from
Jan 28, 2020

Conversation

tylersmalley
Copy link
Contributor

Sorting/Aggregating by _id was removed in #52517

I am not seeing a reason yet as to why the sorting here is necessary as we never perform additional fetches of the data outside of exportSizeLimit which would require consistent sorting.

cc: @rudolf

@tylersmalley tylersmalley added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 16, 2020
@tylersmalley tylersmalley requested a review from rudolf January 16, 2020 21:57
@tylersmalley tylersmalley requested a review from a team as a code owner January 16, 2020 21:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

I've asked Mike Cote to take a look as well, but my guess is this was only added to produce stable exports.

@mikecote
Copy link
Contributor

The sort by _id was there to solve this issue #29747. Though for now since we're loading them all at the same time, we could just do so on the server side or drop the this feature overall.

@rudolf
Copy link
Contributor

rudolf commented Jan 17, 2020

If this feature was requested by users we should rather keep it by doing a server-side sort. We would eventually like to stream these results to support huge exports but there's no immediate timeline for implementing that.

Sorting/Aggregating by _id was removed in
elastic#52517

Retains sorting as requested by
elastic#29747

Signed-off-by: Tyler Smalley <[email protected]>
Signed-off-by: Tyler Smalley <[email protected]>
@tylersmalley
Copy link
Contributor Author

@elasticmachine merge upstream

@tylersmalley
Copy link
Contributor Author

I have update to sort server-side.

I think doing this with ES does make more sense, but we would need another field to sort-by. We need to either copy _id to something with doc_values enabled, or find another field to sort by. I think finding another field would make sense but would be a breaking chance since the order would change. We should consider having a created_at field which is automatically populated which we could then sort by.

@rudolf
Copy link
Contributor

rudolf commented Jan 26, 2020

We should consider having a created_at field which is automatically populated which we could then sort by.

This sounds like a good idea.

@tylersmalley tylersmalley changed the title Removes sorting by _id in SavedObject export Sort server-side in SavedObject export Jan 27, 2020
@tylersmalley
Copy link
Contributor Author

Discussed with @mikecote who recommended we just sort the ES response in fetchObjectsToExport where the ES sort was removed. sortObjects is sorting for priority of dependencies, which will take precedence but still result in consistent results.

@@ -399,6 +393,79 @@ describe('getSortedObjectsForExport()', () => {
).rejects.toThrowErrorMatchingInlineSnapshot(`"Can't export more than 1 objects"`);
});

test('sorts objects within type', async () => {
savedObjectsClient.find.mockResolvedValueOnce({
Copy link
Contributor Author

@tylersmalley tylersmalley Jan 27, 2020

Choose a reason for hiding this comment

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

There previously was only coverage when objects were provided, resulting in the bulk request - so this also has the benefit of including that path using find.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Discussed with @mikecote who recommended we just sort the ES response in fetchObjectsToExport where the ES sort was removed. sortObjects is sorting for priority of dependencies, which will take precedence but still result in consistent results.

Ah I missed that 👍

Signed-off-by: Tyler Smalley <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@tylersmalley tylersmalley merged commit ff37dd1 into elastic:master Jan 28, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 29, 2020
* master: (31 commits)
  [SIEM] Overview page feedback (elastic#56261)
  refactor (elastic#56131)
  [NP Cleanup] Remove ui/public/inspector (elastic#55677)
  [SIEM] [TIMELINE] Only add endpoint logo when on event.module === endgame (elastic#56263)
  Basic Functionality Alert List (elastic#55800)
  [SIEM] Fix filters on Hosts and Network page (elastic#56234)
  [SIEM] Adds ability to infer the newsfeed.enabled setting (elastic#56236)
  [SIEM][Detection Engine] critical blocker for updated rules
  [SIEM][Detection Engine] critical blocker, fixes ordering issue that causes rules to not run the first time
  [SIEM] Add link to endpoint app through reference.url (elastic#56211)
  [Metrics UI] Fixing title truncation in Metrics Explorer (elastic#55917)
  [SIEM] Put the notice for rules in comment block (elastic#56123)
  [SIEM][Detection Engine] critical blocker with the UI crashing
  Consistent timeouts for the Space onPostAuth interceptor tests (elastic#56158)
  Skip tests that depend on other skipped test
  [SIEM] [Detection Engine] Timestamps for rules (elastic#56197)
  Sort server-side in SavedObject export (elastic#55128)
  [Reporting] Document the 8.0 breaking changes (elastic#56187)
  Revert "[Monitoring] Change all configs to `monitoring.*`" (elastic#56214)
  add owners for es_archiver (elastic#56184)
  ...
@tylersmalley
Copy link
Contributor Author

@mikecote, do you want this backported to 7.x - or just leave it at 8.0?

@mikecote
Copy link
Contributor

@tylersmalley

@mikecote, do you want this backported to 7.x - or just leave it at 8.0?

I don't see why not, might as well backport this to 7.x.

tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Feb 10, 2020
tylersmalley pushed a commit that referenced this pull request Feb 10, 2020
Signed-off-by: Tyler Smalley <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@tylersmalley
Copy link
Contributor Author

👍 @mikecote, backported #57243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Saved Objects release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants