Skip to content

[Discover] Use tiebreaker sort in main discover view#32426

Closed
wylieconlon wants to merge 1 commit intoelastic:masterfrom
wylieconlon:sort-discover-tiebreaker
Closed

[Discover] Use tiebreaker sort in main discover view#32426
wylieconlon wants to merge 1 commit intoelastic:masterfrom
wylieconlon:sort-discover-tiebreaker

Conversation

@wylieconlon
Copy link
Copy Markdown
Contributor

Summary

When timestamps are equal, the tiebreaker field should be used to provide a consistent sort order. This field was being used only in the Context view, but not in the Discover view.

To reproduce this issue, go to Advanced Settings > Discover and set a tiebreaker field like offset,_doc.

Before:

screenshot 2019-03-04 15 58 55

After:

screenshot 2019-03-04 15 59 20

Closes #32221

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

@wylieconlon wylieconlon added release_note:enhancement Feature:Discover Discover Application Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Mar 4, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@wylieconlon
Copy link
Copy Markdown
Contributor Author

@chrisdavies Could you take a look at this when you get a chance?

Copy link
Copy Markdown
Contributor

@chrisdavies chrisdavies left a comment

Choose a reason for hiding this comment

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

LGTM. Just a review. Didn't pull down and test.

Copy link
Copy Markdown
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, when the conflicts are solved, this should be merged. tested locally with chrome

@kertal kertal self-requested a review June 18, 2019 13:26
Copy link
Copy Markdown
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

When the case with an empty tiebreaker is handled, conflicts are resolved, this should be merged

.setField('sort', [
getSort($state.sort, $scope.indexPattern),
{
[tiebreakerField]: defaultSortOrder,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No tiebreakerField (I've set the field to empty in Advanced Configuration) leads to the following sort {"undefined":{"order":"desc","unmapped_type":"boolean"}.

@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented Jul 31, 2019

This isn't a bug, it was intentional. The tiebreaker was needed in the context app because it uses search_after so we needed a guaranteed sort order. Discover has no such requirement so we don't use it there. Applying this setting in Discover doesn't really hurt anything, but it does make the sort code slightly more complex for no real benefit IMO. Also the setting is specifically called "context:tieBreakerFields", so if we're going to use it in Discover we should probably update the name of the setting and the description in the docs.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@benbuzbee
Copy link
Copy Markdown

benbuzbee commented Sep 27, 2019

+1 for this. It would offer a pretty reasonable workaround for elastic/beats/7559

Use case; Using discover to analyze logs uploaded by filebeat which have millisecond collisions. Saved searches are a workaround but an extra-click barrier and less sharable. When using filebeat on indices using log.offset as a secondary sort field by default (or tiebreaker) seems to make a lot of sense.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Discover Discover Application release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t//

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discover does not use tiebreaker sort order

6 participants