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

[OnWeek][Discover] Allow to fetch more documents on Discover page #157241

Closed
wants to merge 67 commits into from
Closed
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
4e80b82
[Discover] Improve date_nanos support for "search_after" pagination
jughosta May 9, 2023
c07d672
Merge branch 'main' into onweek-date-nanos-pagination
jughosta May 10, 2023
e4e1269
[Discover] Allow to fetch more documents on Discover page
jughosta May 11, 2023
3d26f8a
Merge remote-tracking branch 'origin/onweek-date-nanos-pagination' in…
jughosta May 11, 2023
d218e6e
[Discover] Fix when the footer should appear
jughosta May 12, 2023
3254b98
[Discover] Experiment with a tie breaker field
jughosta May 12, 2023
0c867cf
[Discover] Update other places
jughosta May 12, 2023
e868a12
[Discover] Comment out the tie breaker
jughosta May 12, 2023
bead1dc
[Discover] Fix lint issues
jughosta May 12, 2023
d2c26f9
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Jul 24, 2023
f4575d7
[Discover] Update styles
jughosta Jul 24, 2023
f9adfa1
[Discover] Handle error cases
jughosta Jul 24, 2023
e6a9453
[Discover] Fix types
jughosta Jul 24, 2023
46a7a13
[Discover] Fix default sorting order for locator
jughosta Jul 24, 2023
b11ed0a
[Discover] Add a comment
jughosta Jul 24, 2023
87f57d3
[Discover] Add a tooltip when refresh interval is on
jughosta Jul 25, 2023
200b978
[Discover] Update code style
jughosta Jul 25, 2023
0b951e9
Merge branch 'main' into onweek-date-nanos-pagination
jughosta Jul 25, 2023
d89ec6b
[Discover] Show error toasts
jughosta Jul 27, 2023
bc57946
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Jul 27, 2023
e10c82c
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jul 27, 2023
32653ea
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Aug 2, 2023
e38495a
[Discover] Fix after merging main
jughosta Aug 2, 2023
02124f9
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Aug 3, 2023
a75d3cd
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Aug 4, 2023
f7759a3
[Discover] Update comment
jughosta Aug 4, 2023
d50cd7e
[Discover] Remove redundant param
jughosta Aug 4, 2023
ebadf47
[Discover] Refactor params of getSortForSearchSource
jughosta Aug 4, 2023
8c5f2f2
[Discover] Refactor params of get_es_query_sort
jughosta Aug 4, 2023
08281ea
[Discover] Don't update histogram for "Load more" in the grid
jughosta Aug 4, 2023
13d5b0a
[Discover] Fix footer for Surrounding Documents and Dashboard pages
jughosta Aug 4, 2023
5da6f3f
[Discover] Add unit tests
jughosta Aug 4, 2023
c193c64
[Discover] Add tests for the footer
jughosta Aug 7, 2023
c73ef31
[Discover] Add functional tests
jughosta Aug 7, 2023
556b284
Merge branch 'main' into onweek-date-nanos-pagination
jughosta Aug 7, 2023
b2d4ba6
[Discover] Add functional tests for date nanos
jughosta Aug 7, 2023
e3555f7
Merge remote-tracking branch 'origin/onweek-date-nanos-pagination' in…
jughosta Aug 7, 2023
ef34510
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Aug 7, 2023
2b97c3d
[Discover] Extract the code
jughosta Aug 7, 2023
6be7240
Merge remote-tracking branch 'origin/onweek-date-nanos-pagination' in…
jughosta Aug 7, 2023
76f1cfe
Merge branch 'main' into onweek-date-nanos-pagination
jughosta Aug 8, 2023
e3c2d83
Merge branch 'main' into onweek-date-nanos-pagination
jughosta Aug 8, 2023
2a8fefa
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Aug 10, 2023
a57eafa
[Discover] Add a test for fetchMore
jughosta Aug 10, 2023
5d10203
[Discover] Changes to abort logic
jughosta Aug 10, 2023
a72bd08
Merge remote-tracking branch 'upstream/main' into onweek-date-nanos-p…
jughosta Aug 11, 2023
358c755
[Discover] Add `_doc` as a tie breaker
jughosta Aug 11, 2023
5413e5e
[Discover] Update tests
jughosta Aug 11, 2023
a2cb79f
[Discover] Fix for CSV
jughosta Aug 11, 2023
c8728d4
[Discover] Invert the parameter
jughosta Aug 11, 2023
8d63088
[Discover] Remove the comment
jughosta Aug 11, 2023
bb5bd8a
Merge branch 'main' into onweek-date-nanos-pagination
jughosta Aug 11, 2023
2fb0eab
[Security Solution] expandable flyout - expandable panel UI update (#…
PhilippeOberti Aug 11, 2023
2d301a3
[controls] fix Dashboard getting stuck at loading in Kibana when Cont…
nreese Aug 11, 2023
334fcb8
[Security Solution] Adds RBAC for Assistant (#163031)
spong Aug 11, 2023
25a19fd
[Security Solution] expandable flyout - inverse Visualizations and In…
PhilippeOberti Aug 11, 2023
77ea97e
Add timeWindow type changes to slo docs (#163367)
wandergeek Aug 11, 2023
a2b8a72
[data views] swap_references api improvements (#163225)
mattkime Aug 12, 2023
3d336b9
[Security Solution] [Detections] Fixes flakey exceptions read-only vi…
dhurley14 Aug 12, 2023
3f9bdac
[api-docs] 2023-08-12 Daily api_docs build (#163762)
kibanamachine Aug 12, 2023
f0dfe07
[Telemetry] Use header-based versioned APIs instead of path-based (#1…
afharo Aug 12, 2023
f780097
[api-docs] 2023-08-13 Daily api_docs build (#163773)
kibanamachine Aug 13, 2023
5a9f68f
[Observability AI Assistant] Update README.md (#163769)
dgieselaar Aug 13, 2023
a29238b
[api-docs] 2023-08-14 Daily api_docs build (#163775)
kibanamachine Aug 14, 2023
5cd37d2
[Fleet][Agent Policy][Agent Tamper Protection] UI / API guard agent t…
parkiino Aug 14, 2023
17448a0
[Fleet] Only enable secret storage once all fleet servers are above 8…
hop-dev Aug 14, 2023
5230557
[Discover] Send a performance metric for fetchMore
jughosta Aug 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/plugins/discover/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* Side Public License, v 1.
*/

export const MAX_LOADED_GRID_ROWS = 10000;
Copy link
Member

Choose a reason for hiding this comment

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

I checked about the impact for loading 10000 documents.
It's not surprising, the used memory of the Browser is increasing significantly when loading 10000 documents incrementally. But for my working machine, a MacBook Pro, 2021, 32 GB, this ain't an issue. For sure this can get much slower depending on the machine spec.

I've been using my iPhone SE 2, to test it. It is no a pleasure to use Discover with this machine, but when I reached the end of those 10000 documents being loaded ... the UI was snappy.

Give we also allow the configuration of a 10000 sample size, which also worked for me: LGTM

export const DEFAULT_ROWS_PER_PAGE = 100;
export const ROWS_PER_PAGE_OPTIONS = [10, 25, 50, DEFAULT_ROWS_PER_PAGE, 250, 500];
export enum VIEW_MODE {
Expand Down
144 changes: 144 additions & 0 deletions src/plugins/discover/common/utils/sorting/get_es_query_sort.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { SortDirection } from '@kbn/data-plugin/common';
import { createStubDataView } from '@kbn/data-views-plugin/common/data_view.stub';
import {
getEsQuerySort,
getESQuerySortForTieBreaker,
getESQuerySortForTimeField,
getTieBreakerFieldName,
} from './get_es_query_sort';
import { CONTEXT_TIE_BREAKER_FIELDS_SETTING } from '@kbn/discover-utils';
import { IUiSettingsClient } from '@kbn/core-ui-settings-browser';

const dataView = createStubDataView({
spec: {
id: 'logstash-*',
fields: {
test_field: {
name: 'test_field',
type: 'string',
esTypes: ['keyword'],
aggregatable: true,
searchable: true,
},
test_field_not_sortable: {
name: 'test_field_not_sortable',
type: 'string',
esTypes: ['keyword'],
aggregatable: false,
searchable: false,
},
},
title: 'logstash-*',
timeFieldName: '@timestamp',
},
});

describe('get_es_query_sort', function () {
test('getEsQuerySort should return sort params', function () {
expect(
getEsQuerySort({
sortDir: SortDirection.desc,
timeFieldName: 'testTimeField',
isTimeNanosBased: false,
tieBreakerFieldName: 'testTieBreakerField',
})
).toStrictEqual([
{ testTimeField: { format: 'strict_date_optional_time', order: 'desc' } },
{ testTieBreakerField: 'desc' },
]);

expect(
getEsQuerySort({
sortDir: SortDirection.asc,
timeFieldName: 'testTimeField',
isTimeNanosBased: true,
tieBreakerFieldName: 'testTieBreakerField',
})
).toStrictEqual([
{
testTimeField: {
format: 'strict_date_optional_time_nanos',
numeric_type: 'date_nanos',
order: 'asc',
},
},
{ testTieBreakerField: 'asc' },
]);
});

test('getESQuerySortForTimeField should return time field as sort param', function () {
expect(
getESQuerySortForTimeField({
sortDir: SortDirection.desc,
timeFieldName: 'testTimeField',
isTimeNanosBased: false,
})
).toStrictEqual({
testTimeField: {
format: 'strict_date_optional_time',
order: 'desc',
},
});

expect(
getESQuerySortForTimeField({
sortDir: SortDirection.asc,
timeFieldName: 'testTimeField',
isTimeNanosBased: true,
})
).toStrictEqual({
testTimeField: {
format: 'strict_date_optional_time_nanos',
numeric_type: 'date_nanos',
order: 'asc',
},
});
});

test('getESQuerySortForTieBreaker should return tie breaker as sort param', function () {
expect(
getESQuerySortForTieBreaker({
sortDir: SortDirection.desc,
tieBreakerFieldName: 'testTieBreaker',
})
).toStrictEqual({ testTieBreaker: 'desc' });
});

test('getTieBreakerFieldName should return a correct tie breaker', function () {
expect(
getTieBreakerFieldName(dataView, {
get: (key) => (key === CONTEXT_TIE_BREAKER_FIELDS_SETTING ? ['_doc'] : undefined),
} as IUiSettingsClient)
).toBe('_doc');

expect(
getTieBreakerFieldName(dataView, {
get: (key) =>
key === CONTEXT_TIE_BREAKER_FIELDS_SETTING
? ['test_field_not_sortable', '_doc']
: undefined,
} as IUiSettingsClient)
).toBe('_doc');

expect(
getTieBreakerFieldName(dataView, {
get: (key) =>
key === CONTEXT_TIE_BREAKER_FIELDS_SETTING ? ['test_field', '_doc'] : undefined,
} as IUiSettingsClient)
).toBe('test_field');

expect(
getTieBreakerFieldName(dataView, {
get: (key) => undefined,
} as IUiSettingsClient)
).toBeUndefined();
});
});
107 changes: 107 additions & 0 deletions src/plugins/discover/common/utils/sorting/get_es_query_sort.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import type { EsQuerySortValue, SortDirection } from '@kbn/data-plugin/public';
import type { DataView } from '@kbn/data-views-plugin/common';
import type { IUiSettingsClient } from '@kbn/core-ui-settings-browser';
import { CONTEXT_TIE_BREAKER_FIELDS_SETTING } from '@kbn/discover-utils';

/**
* Returns `EsQuerySort` which is used to sort records in the ES query
* https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html
* @param sortDir
* @param timeFieldName
* @param tieBreakerFieldName
* @param isTimeNanosBased
*/
export function getEsQuerySort({
sortDir,
timeFieldName,
tieBreakerFieldName,
isTimeNanosBased,
}: {
sortDir: SortDirection;
timeFieldName: string;
tieBreakerFieldName: string;
isTimeNanosBased: boolean;
}): [EsQuerySortValue, EsQuerySortValue] {
return [
getESQuerySortForTimeField({ sortDir, timeFieldName, isTimeNanosBased }),
getESQuerySortForTieBreaker({ sortDir, tieBreakerFieldName }),
];
}

/**
* Prepares "sort" structure for a time field for next ES request
* @param sortDir
* @param timeFieldName
* @param isTimeNanosBased
*/
export function getESQuerySortForTimeField({
sortDir,
timeFieldName,
isTimeNanosBased,
}: {
sortDir: SortDirection;
timeFieldName: string;
isTimeNanosBased: boolean;
}): EsQuerySortValue {
return {
[timeFieldName]: {
order: sortDir,
...(isTimeNanosBased
? {
format: 'strict_date_optional_time_nanos',
numeric_type: 'date_nanos',
}
: { format: 'strict_date_optional_time' }),
},
};
}

/**
* Prepares "sort" structure for a tie breaker for next ES request
* @param sortDir
* @param tieBreakerFieldName
*/
export function getESQuerySortForTieBreaker({
sortDir,
tieBreakerFieldName,
}: {
sortDir: SortDirection;
tieBreakerFieldName: string;
}): EsQuerySortValue {
return { [tieBreakerFieldName]: sortDir };
}

/**
* The default tie breaker for Discover
*/
export const DEFAULT_TIE_BREAKER_NAME = '_doc';

/**
* The list of field names that are allowed for sorting, but not included in
* data view fields.
*/
const META_FIELD_NAMES: string[] = ['_seq_no', '_doc', '_uid'];
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if _uid is safe to sort on? With _id, at least, it's often a problem that the lack of doc values leads to a heavy usage of the fielddata cache which inpacts the whole cluster going forward and often ends in an OOM error. There are several related issues in the ES repo such as elastic/elasticsearch#60778. Did we take that into account here?

The documentation recommends using PIT with _shard_doc, which probably is out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weltenwort Thanks for bring our attention to this! I have not introduced this array, just moved from context utils to common utils. For supporting legacy behaviour, I think we should keep _uid available for user's selection. We can also add _shard_doc to the list, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can only use _shard_doc within a PIT context.

Copy link
Member

Choose a reason for hiding this comment

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

The _shard_doc value is the combination of the shard index within the PIT and the Lucene’s internal doc ID, it is unique per document and constant within a PIT.

wouldn't _doc be sufficient, since we are also using it already in Discover:

return [{ _doc: defaultDirection } as EsQuerySortValue];

Copy link
Member

Choose a reason for hiding this comment

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

It would work, it might just lead to duplicate rows sometimes when shard merges occur. For the Logs UI that's a trade-off that we accepted.

Copy link
Member

Choose a reason for hiding this comment

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

I think this trade-off is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback on this! I agree that the tradeoff should be fine for us too.


/**
* Returns a field from the intersection of the set of sortable fields in the
* given data view and a given set of candidate field names.
*/
export function getTieBreakerFieldName(
dataView: DataView,
uiSettings: Pick<IUiSettingsClient, 'get'>
) {
const sortableFields = (uiSettings.get(CONTEXT_TIE_BREAKER_FIELDS_SETTING) || []).filter(
(fieldName: string) =>
META_FIELD_NAMES.includes(fieldName) ||
(dataView.fields.getByName(fieldName) || { sortable: false }).sortable
);
return sortableFields[0];
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,35 +6,92 @@
* Side Public License, v 1.
*/
import type { SortOrder } from '@kbn/saved-search-plugin/public';
import { SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils';
import { getSortForSearchSource } from './get_sort_for_search_source';
import { stubDataView, stubDataViewWithoutTimeField } from '@kbn/data-plugin/common/stubs';
import { coreMock } from '@kbn/core/public/mocks';

describe('getSortForSearchSource function', function () {
const core = coreMock.createStart();
const uiSettings = core.uiSettings;

const uiSettingWithAscSorting = coreMock.createStart().uiSettings;
jest
.spyOn(uiSettingWithAscSorting, 'get')
.mockImplementation((key) => (key === SORT_DEFAULT_ORDER_SETTING ? 'asc' : null));

test('should be a function', function () {
expect(typeof getSortForSearchSource === 'function').toBeTruthy();
});

test('should return an object to use for searchSource when columns are given', function () {
const cols = [['bytes', 'desc']] as SortOrder[];
expect(getSortForSearchSource(cols, stubDataView)).toEqual([{ bytes: 'desc' }]);
expect(getSortForSearchSource(cols, stubDataView, 'asc')).toEqual([{ bytes: 'desc' }]);
expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataView,
uiSettings,
includeTieBreaker: true,
})
).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]);

expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField)).toEqual([{ bytes: 'desc' }]);
expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, 'asc')).toEqual([
{ bytes: 'desc' },
]);
expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataView,
uiSettings: uiSettingWithAscSorting,
includeTieBreaker: true,
})
).toEqual([{ bytes: 'desc' }, { _doc: 'desc' }]);

expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataView,
uiSettings: uiSettingWithAscSorting,
})
).toEqual([{ bytes: 'desc' }]);

expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataViewWithoutTimeField,
uiSettings,
includeTieBreaker: true,
})
).toEqual([{ bytes: 'desc' }]);

expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataViewWithoutTimeField,
uiSettings: uiSettingWithAscSorting,
})
).toEqual([{ bytes: 'desc' }]);
});

test('should return an object to use for searchSource when no columns are given', function () {
const cols = [] as SortOrder[];
expect(getSortForSearchSource(cols, stubDataView)).toEqual([{ _doc: 'desc' }]);
expect(getSortForSearchSource(cols, stubDataView, 'asc')).toEqual([{ _doc: 'asc' }]);

expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField)).toEqual([
{ _score: 'desc' },
]);
expect(getSortForSearchSource(cols, stubDataViewWithoutTimeField, 'asc')).toEqual([
{ _score: 'asc' },
expect(getSortForSearchSource({ sort: cols, dataView: stubDataView, uiSettings })).toEqual([
{ _doc: 'desc' },
]);
expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataView,
uiSettings: uiSettingWithAscSorting,
})
).toEqual([{ _doc: 'asc' }]);

expect(
getSortForSearchSource({ sort: cols, dataView: stubDataViewWithoutTimeField, uiSettings })
).toEqual([{ _score: 'desc' }]);
expect(
getSortForSearchSource({
sort: cols,
dataView: stubDataViewWithoutTimeField,
uiSettings: uiSettingWithAscSorting,
})
).toEqual([{ _score: 'asc' }]);
});
});
Loading