[context view] Use _doc for tie-breaking instead of _uid #12096
Conversation
weltenwort
force-pushed
the
fix/context-uid-fielddata
branch
from
5231c19 to
022f971
Compare
weltenwort
changed the title
src/ui/ui_settings/defaults.js
Outdated
| }, | ||
| 'context:tieBreakerFields': { | ||
| value: ['_doc'], | ||
| description: 'A list of fields to use for tie-breaking between documents ' + |
There was a problem hiding this comment.
Could you add that it's a comma delimited list of fields? Even I had to stop for a moment to figure out how to enter multiple fields in the input.
| function getFirstSortableField(indexPattern, fieldNames) { | ||
| const sortableFields = fieldNames.filter((fieldName) => ( | ||
| META_FIELD_NAMES.includes(fieldName) | ||
| || (indexPattern.fields.byName[fieldName] || { sortable: false }).sortable |
There was a problem hiding this comment.
If none of the configured fields match a meta field or index pattern field, context will throw a fatal error (as a result of sending a bad request to ES). Same thing happens if the field exists, but is not sortable. We should handle this more elegantly so the user knows how to fix the problem.
There was a problem hiding this comment.
The fatal error being shown is a consequence of #12124, which I'm trying to fix as well right now. This has proven to be more difficult than expected, but as soon as I have worked around the error handling memory leaks in courier I will handle these more gracefully.
There was a problem hiding this comment.
In this case we're sending an invalid query to elasticsearch though, which I don't think we should do in the first place. The sort portion of the query looks like this:
"sort": [
{
"@timestamp": {
"order": "desc",
"unmapped_type": "boolean"
}
},
{
"undefined": {
"order": "asc",
"unmapped_type": "boolean"
}
}
],
Instead, what if we fail early if there are no valid tiebreaker fields and notify the user with a message saying they need to set a valid tiebreaker field?
There was a problem hiding this comment.
You're absolutely right... I had my head in the error handling code so deep I didn't see that avoiding the error is the obvious solution. 😊
Using fields with docvalues (like `_doc`) for tie-breaking yields significantly better performance than using `_uid`, which lacks docvalues at the moment. The downside is that sorting by `_doc` by default is not stable under all conditions, but better than no tie-breaking at all. The new setting `context:tieBreakingFields` enables the user to customize the list of fields Kibana attempts to use for tie-breaking. The first field from that list, that is sortable in the current index pattern, will be used. It defaults to `_doc`, which should change to `_seq_no` from version 6.0 on.
In addition to just showing a notification, errors that occur while loading documents from the database will be stored as part of the `loadingStatus` along with a reason code (if known). This is used to display more nuanced and helpful error messages to the user. The first such error message indicates a missing or invalid tiebreaker field required for sorting the context.
weltenwort
force-pushed
the
fix/context-uid-fielddata
branch
from
022f971 to
a92628e
Compare
|
@Bargs @lukasolson I think this is ready for another view. The prior review comments should have been addressed:
|
|
Looks great! |
Using fields with docvalues (like `_doc`) for tie-breaking yields significantly better performance than using `_uid`, which lacks docvalues at the moment. The downside is that sorting by `_doc` by default is not stable under all conditions, but better than no tie-breaking at all. The new setting `context:tieBreakingFields` enables the user to customize the list of fields Kibana attempts to use for tie-breaking. The first field from that list, that is sortable in the current index pattern, will be used. It defaults to `_doc`, which should change to `_seq_no` from version 6.0 on. In addition to just showing a notification, errors that occur while loading documents from the database will be stored as part of the `loadingStatus` along with a reason code (if known). This is used to display more nuanced and helpful error messages to the user. The first such error message indicates a missing or invalid tiebreaker field required for sorting the context.
Using fields with docvalues (like
_doc) for tie-breaking yieldssignificantly better performance than using
_uid, which lacksdocvalues at the moment. The downside is that sorting by
_docbydefault is not stable under all conditions, but better than no
tie-breaking at all.
The new setting
context:tieBreakingFieldsenables the user tocustomize the list of fields Kibana attempts to use for tie-breaking.
The first field from that list, that is sortable in the current index
pattern, will be used. It defaults to
_doc, which should change to_seq_nofrom version 6.0 on.Summary: To avoid filling up Elasticsearch's fielddata cache by
sorting on the _uid field in the context view, the field _doc is now
used as a tiebreaker by default. The field to be used can now be
configured using the context:tieBreakingFields advanced setting.
fixes #11925