Skip to content

Commit

Permalink
[discover] Less field list loading (elastic#147825)
Browse files Browse the repository at this point in the history
## Summary

Looking at why Discover was loading data view field lists were loading
more than needed, I found more reasons than actual field list loads
occurring but I think they're all deserving of improvement.

- Discover was loading the default data view to see if there was an
existing data view. We can rely on the `hasData` api instead.
- We were loading a data view and then refreshing the field list. If the
data view was being loaded fresh, its meant the field list was loaded
twice. Field list refresh has been integrated into the dataViews api.
- We were loading a data view, checking it if was adhoc, and tossing it
if it wasn't. Now we keep the reference.

Previously on page load, discover would make 3 calls to
`fields_for_wildcard`, now it makes two. It loads all the field with one
request and it makes an additional request where it applies the current
filter to find relevant fields.

Closes elastic#147744
  • Loading branch information
mattkime authored and crespocarlos committed Dec 23, 2022
1 parent 242188c commit 0a4a006
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 70 deletions.
8 changes: 8 additions & 0 deletions src/plugins/data_views/common/data_views/data_views.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ describe('IndexPatterns', () => {
SOClientGetDelay = 0;
});

test('force field refresh', async () => {
const id = '1';
await indexPatterns.get(id);
expect(apiClient.getFieldsForWildcard).toBeCalledTimes(1);
await indexPatterns.get(id, undefined, true);
expect(apiClient.getFieldsForWildcard).toBeCalledTimes(2);
});

test('does cache ad-hoc data views', async () => {
const id = '1';

Expand Down
26 changes: 20 additions & 6 deletions src/plugins/data_views/common/data_views/data_views.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export interface DataViewsServicePublicMethods {
* @param id - Id of the data view to get.
* @param displayErrors - If set false, API consumer is responsible for displaying and handling errors.
*/
get: (id: string, displayErrors?: boolean) => Promise<DataView>;
get: (id: string, displayErrors?: boolean, refreshFields?: boolean) => Promise<DataView>;
/**
* Get populated data view saved object cache.
*/
Expand All @@ -214,8 +214,9 @@ export interface DataViewsServicePublicMethods {
getDefaultId: () => Promise<string | null>;
/**
* Get default data view, if it doesn't exist, choose and save new default data view and return it.
* @param refreshFields - refresh field list when true
*/
getDefaultDataView: () => Promise<DataView | null>;
getDefaultDataView: (refreshFields?: boolean) => Promise<DataView | null>;
/**
* Get fields for data view
* @param dataView - Data view instance or spec
Expand Down Expand Up @@ -873,10 +874,22 @@ export class DataViewsService {
* Get an index pattern by id, cache optimized.
* @param id
* @param displayErrors - If set false, API consumer is responsible for displaying and handling errors.
* @param refreshFields - If set true, will fetch fields from the index pattern
*/
get = async (id: string, displayErrors: boolean = true): Promise<DataView> => {
get = async (
id: string,
displayErrors: boolean = true,
refreshFields = false
): Promise<DataView> => {
const dataViewFromCache = this.dataViewCache.get(id)?.then(async (dataView) => {
if (dataView && refreshFields) {
await this.refreshFields(dataView);
}
return dataView;
});

const indexPatternPromise =
this.dataViewCache.get(id) ||
dataViewFromCache ||
this.dataViewCache.set(id, this.getSavedObjectAndInit(id, displayErrors));

// don't cache failed requests
Expand Down Expand Up @@ -1131,9 +1144,10 @@ export class DataViewsService {
* another data view is selected as default and returned.
* If no possible data view found to become a default returns null.
*
* @param {boolean} refreshFields - if true, will refresh the fields of the default data view
* @returns default data view
*/
async getDefaultDataView(): Promise<DataView | null> {
async getDefaultDataView(refreshFields?: boolean): Promise<DataView | null> {
const patterns = await this.getIdsWithTitle();
let defaultId: string | undefined = await this.config.get('defaultIndex');
const exists = defaultId ? patterns.some((pattern) => pattern.id === defaultId) : false;
Expand All @@ -1154,7 +1168,7 @@ export class DataViewsService {
}

if (defaultId) {
return this.get(defaultId);
return this.get(defaultId, undefined, refreshFields);
} else {
return null;
}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/discover/public/__mocks__/data_views.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export function createDiscoverDataViewsMock() {
return Promise.reject('Invald');
}
},
getDefaultDataView: jest.fn(() => dataViewMock),
updateSavedObject: jest.fn(),
getIdsWithTitle: jest.fn(() => {
return Promise.resolve([dataViewMock, dataViewComplexMock, dataViewWithTimefieldMock]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,6 @@ export function DiscoverMainRoute(props: Props) {
return;
}

const defaultDataView = await data.dataViews.getDefaultDataView();

if (!defaultDataView) {
setShowNoDataPage(true);
return;
}

const { appStateContainer } = getState({ history, savedSearch: nextSavedSearch, services });
const { index, query } = appStateContainer.getState();
const ip = await loadDataView(
Expand All @@ -118,7 +111,6 @@ export function DiscoverMainRoute(props: Props) {
toastNotifications,
isTextBasedQuery
);
await data.dataViews.refreshFields(dataViewData);
setDataViewList(ipList);

return dataViewData;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { loadDataView, getFallbackDataViewId } from './resolve_data_view';
import { loadDataView } from './resolve_data_view';
import { dataViewsMock } from '../../../__mocks__/data_views';
import { dataViewMock } from '../../../__mocks__/data_view';
import { configMock } from '../../../__mocks__/config';
Expand All @@ -26,13 +26,4 @@ describe('Resolve data view tests', () => {
expect(result.stateValFound).toBe(false);
expect(result.stateVal).toBe(dataViewId);
});
test('getFallbackDataViewId with an empty dataViews array', async () => {
const result = await getFallbackDataViewId([], '');
expect(result).toBe('');
});
test('getFallbackDataViewId with an dataViews array', async () => {
const list = await dataViewsMock.getIdsWithTitle();
const result = await getFallbackDataViewId(list, '');
expect(result).toBe('the-data-view-id');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -34,46 +34,6 @@ interface DataViewData {
stateValFound: boolean;
}

export function findDataViewById(
dataViews: DataViewListItem[],
id: string
): DataViewListItem | undefined {
if (!Array.isArray(dataViews) || !id) {
return;
}
return dataViews.find((o) => o.id === id);
}

/**
* Checks if the given defaultIndex exists and returns
* the first available data view id if not
*/
export function getFallbackDataViewId(
dataViews: DataViewListItem[],
defaultIndex: string = ''
): string {
if (defaultIndex && findDataViewById(dataViews, defaultIndex)) {
return defaultIndex;
}
return dataViews && dataViews[0]?.id ? dataViews[0].id : '';
}

/**
* A given data view id is checked for existence and a fallback is provided if it doesn't exist
* The provided defaultIndex is usually configured in Advanced Settings, if it's also invalid
* the first entry of the given list of dataViews is used
*/
export function getDataViewId(
id: string = '',
dataViews: DataViewListItem[] = [],
defaultIndex: string = ''
): string {
if (!id || !findDataViewById(dataViews, id)) {
return getFallbackDataViewId(dataViews, defaultIndex);
}
return id;
}

/**
* Function to load the given data view by id, providing a fallback if it doesn't exist
*/
Expand Down Expand Up @@ -104,9 +64,10 @@ export async function loadDataView(
fetchId = dataViewSpec.id!;
}

let fetchedDataView: DataView | undefined;
// try to fetch adhoc data view first
try {
const fetchedDataView = fetchId ? await dataViews.get(fetchId) : undefined;
fetchedDataView = fetchId ? await dataViews.get(fetchId) : undefined;
if (fetchedDataView && !fetchedDataView.isPersisted()) {
return {
list: dataViewList || [],
Expand All @@ -122,12 +83,14 @@ export async function loadDataView(
} catch (e) {}

// fetch persisted data view
const actualId = getDataViewId(fetchId, dataViewList, config.get('defaultIndex'));
return {
list: dataViewList || [],
loaded: await dataViews.get(actualId),
loaded: fetchedDataView
? fetchedDataView
: // we can be certain that the data view exists due to an earlier hasData check
((await dataViews.getDefaultDataView()) as DataView),
stateVal: fetchId,
stateValFound: !!fetchId && actualId === fetchId,
stateValFound: !!fetchId && !!fetchedDataView,
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@ export function ChangeDataView({
isTextBasedLangSelected={isTextBasedLangSelected}
setPopoverIsOpen={setPopoverIsOpen}
onChangeDataView={async (newId) => {
const dataView = await data.dataViews.get(newId);
await data.dataViews.refreshFields(dataView);
// refreshing the field list
await dataViews.get(newId, undefined, true);
setSelectedDataViewId(newId);
setPopoverIsOpen(false);
if (isTextBasedLangSelected && !isTextLangTransitionModalDismissed) {
Expand Down

0 comments on commit 0a4a006

Please sign in to comment.