From f6795f5c72ee6ffb8fb08f5ad86a45c5bbdd68cf Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Fri, 30 Apr 2021 14:50:16 +0300 Subject: [PATCH 1/7] [Data Table] Expensive queries are causing unnecessary load and delays on Elasticsearch Part of #93770 --- .../server/usage_collector/get_stats.test.ts | 10 ++-- .../server/usage_collector/get_stats.ts | 50 +++++++++++++++---- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts index 3f8f4289321b5..a04e3c1093d2e 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts @@ -42,14 +42,18 @@ const mockVisualizations = { describe('vis_type_table getStats', () => { const mockSoClient = ({ - find: jest.fn().mockResolvedValue(mockVisualizations), + createPointInTimeFinder: jest.fn().mockResolvedValue({ + find: function* asyncGenerator() { + yield mockVisualizations; + }, + }), } as unknown) as SavedObjectsClientContract; test('Returns stats from saved objects for table vis only', async () => { const result = await getStats(mockSoClient); - expect(mockSoClient.find).toHaveBeenCalledWith({ + expect(mockSoClient.createPointInTimeFinder).toHaveBeenCalledWith({ type: 'visualization', - perPage: 10000, + perPage: 1000, }); expect(result).toEqual({ total: 4, diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts index 6fdb555c86328..b5ac2b33201cb 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts @@ -6,13 +6,17 @@ * Side Public License, v 1. */ -import { ISavedObjectsRepository, SavedObjectsClientContract } from 'kibana/server'; import { - SavedVisState, - VisualizationSavedObjectAttributes, -} from 'src/plugins/visualizations/common'; + ISavedObjectsRepository, + SavedObjectsClientContract, + SavedObjectsFindResult, +} from 'kibana/server'; +import { SavedVisState } from 'src/plugins/visualizations/common'; import { TableVisParams, VIS_TYPE_TABLE } from '../../common'; +// elasticsearch index.max_result_window default value +const ES_MAX_RESULT_WINDOW_DEFAULT_VALUE = 1000; + export interface VisTypeTableUsage { /** * Total number of table type visualizations @@ -38,20 +42,48 @@ export interface VisTypeTableUsage { }; } +/** @internal **/ +type SavedTableVisState = SavedVisState; + /* * Parse the response data into telemetry payload */ export async function getStats( soClient: SavedObjectsClientContract | ISavedObjectsRepository ): Promise { - const visualizations = await soClient.find({ + const finder = await soClient.createPointInTimeFinder({ type: 'visualization', - perPage: 10000, + perPage: ES_MAX_RESULT_WINDOW_DEFAULT_VALUE, }); - const tableVisualizations = visualizations.saved_objects - .map>(({ attributes }) => JSON.parse(attributes.visState)) - .filter(({ type }) => type === VIS_TYPE_TABLE); + let tableVisualizations: SavedTableVisState[] = []; + + for await (const response of finder.find()) { + tableVisualizations = [ + ...tableVisualizations, + ...(response.saved_objects || []).reduce( + (acc: SavedTableVisState[], { attributes }: SavedObjectsFindResult) => { + if (attributes?.visState) { + try { + const visState: SavedVisState = JSON.parse(attributes.visState); + + if (visState.type === VIS_TYPE_TABLE) { + acc.push(visState as SavedTableVisState); + } + } catch { + // nothing to be here, "so" not valid + } + } + return acc; + }, + [] + ), + ]; + + if (!response.saved_objects.length) { + await finder.close(); + } + } const defaultStats = { total: tableVisualizations.length, From 2cecba0b14ddb821a61fe95afa6303c511df6963 Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Fri, 30 Apr 2021 16:34:32 +0300 Subject: [PATCH 2/7] remove extra cycles --- .../server/usage_collector/get_stats.ts | 70 ++++++++----------- 1 file changed, 30 insertions(+), 40 deletions(-) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts index b5ac2b33201cb..927b26b826ecc 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts @@ -12,7 +12,7 @@ import { SavedObjectsFindResult, } from 'kibana/server'; import { SavedVisState } from 'src/plugins/visualizations/common'; -import { TableVisParams, VIS_TYPE_TABLE } from '../../common'; +import { VIS_TYPE_TABLE } from '../../common'; // elasticsearch index.max_result_window default value const ES_MAX_RESULT_WINDOW_DEFAULT_VALUE = 1000; @@ -42,9 +42,6 @@ export interface VisTypeTableUsage { }; } -/** @internal **/ -type SavedTableVisState = SavedVisState; - /* * Parse the response data into telemetry payload */ @@ -56,37 +53,8 @@ export async function getStats( perPage: ES_MAX_RESULT_WINDOW_DEFAULT_VALUE, }); - let tableVisualizations: SavedTableVisState[] = []; - - for await (const response of finder.find()) { - tableVisualizations = [ - ...tableVisualizations, - ...(response.saved_objects || []).reduce( - (acc: SavedTableVisState[], { attributes }: SavedObjectsFindResult) => { - if (attributes?.visState) { - try { - const visState: SavedVisState = JSON.parse(attributes.visState); - - if (visState.type === VIS_TYPE_TABLE) { - acc.push(visState as SavedTableVisState); - } - } catch { - // nothing to be here, "so" not valid - } - } - return acc; - }, - [] - ), - ]; - - if (!response.saved_objects.length) { - await finder.close(); - } - } - - const defaultStats = { - total: tableVisualizations.length, + const stats: VisTypeTableUsage = { + total: 0, total_split: 0, split_columns: { total: 0, @@ -98,20 +66,42 @@ export async function getStats( }, }; - return tableVisualizations.reduce((acc, { aggs, params }) => { + const doTelemetry = ({ aggs, params }: SavedVisState) => { + stats.total += 1; + const hasSplitAgg = aggs.find((agg) => agg.schema === 'split'); if (hasSplitAgg) { - acc.total_split += 1; + stats.total_split += 1; const isSplitRow = params.row; const isSplitEnabled = hasSplitAgg.enabled; - const container = isSplitRow ? acc.split_rows : acc.split_columns; + const container = isSplitRow ? stats.split_rows : stats.split_columns; container.total += 1; container.enabled = isSplitEnabled ? container.enabled + 1 : container.enabled; } + }; + + for await (const response of finder.find()) { + (response.saved_objects || []).forEach(({ attributes }: SavedObjectsFindResult) => { + if (attributes?.visState) { + try { + const visState: SavedVisState = JSON.parse(attributes.visState); + + if (visState.type === VIS_TYPE_TABLE) { + doTelemetry(visState); + } + } catch { + // nothing to be here, "so" not valid + } + } + }); + + if (!response.saved_objects.length) { + await finder.close(); + } + } - return acc; - }, defaultStats); + return stats; } From 9da27c86679c8852a7ebccb872afd5de7e42cbb3 Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Fri, 30 Apr 2021 19:02:16 +0300 Subject: [PATCH 3/7] fix PR comments --- .../vis_type_table/server/usage_collector/get_stats.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts index 927b26b826ecc..aff8117974c63 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts @@ -14,9 +14,6 @@ import { import { SavedVisState } from 'src/plugins/visualizations/common'; import { VIS_TYPE_TABLE } from '../../common'; -// elasticsearch index.max_result_window default value -const ES_MAX_RESULT_WINDOW_DEFAULT_VALUE = 1000; - export interface VisTypeTableUsage { /** * Total number of table type visualizations @@ -50,7 +47,7 @@ export async function getStats( ): Promise { const finder = await soClient.createPointInTimeFinder({ type: 'visualization', - perPage: ES_MAX_RESULT_WINDOW_DEFAULT_VALUE, + perPage: 1000, }); const stats: VisTypeTableUsage = { @@ -98,7 +95,7 @@ export async function getStats( } }); - if (!response.saved_objects.length) { + if (!response.saved_objects.length || response.total === response.saved_objects.length) { await finder.close(); } } From c7f94a7c2a961b42269017537e4c93371a406388 Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Mon, 3 May 2021 10:50:40 +0300 Subject: [PATCH 4/7] fix finder.close --- .../vis_type_table/server/usage_collector/get_stats.test.ts | 1 + .../vis_type_table/server/usage_collector/get_stats.ts | 5 +---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts index a04e3c1093d2e..ae65321b892e1 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts @@ -43,6 +43,7 @@ const mockVisualizations = { describe('vis_type_table getStats', () => { const mockSoClient = ({ createPointInTimeFinder: jest.fn().mockResolvedValue({ + close: jest.fn(), find: function* asyncGenerator() { yield mockVisualizations; }, diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts index aff8117974c63..1e3c37f9130f1 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts @@ -94,11 +94,8 @@ export async function getStats( } } }); - - if (!response.saved_objects.length || response.total === response.saved_objects.length) { - await finder.close(); - } } + await finder.close(); return stats; } From 8712988fc714740897d741b82bae6d51d69646c5 Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Mon, 3 May 2021 12:39:05 +0300 Subject: [PATCH 5/7] code cleanup --- .../server/usage_collector/get_stats.test.ts | 4 +++- .../server/usage_collector/get_stats.ts | 11 ++++++----- .../register_usage_collector.test.ts | 15 +++++++-------- .../usage_collector/register_usage_collector.ts | 3 +-- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts index ae65321b892e1..f1dfd231fd29c 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts @@ -6,8 +6,8 @@ * Side Public License, v 1. */ -import { SavedObjectsClientContract } from 'kibana/server'; import { getStats } from './get_stats'; +import type { SavedObjectsClientContract } from '../../../../core/server'; const mockVisualizations = { saved_objects: [ @@ -52,10 +52,12 @@ describe('vis_type_table getStats', () => { test('Returns stats from saved objects for table vis only', async () => { const result = await getStats(mockSoClient); + expect(mockSoClient.createPointInTimeFinder).toHaveBeenCalledWith({ type: 'visualization', perPage: 1000, }); + expect(result).toEqual({ total: 4, total_split: 3, diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts index 1e3c37f9130f1..704e8d5f08763 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts @@ -6,13 +6,14 @@ * Side Public License, v 1. */ -import { +import { VIS_TYPE_TABLE } from '../../common'; + +import type { ISavedObjectsRepository, SavedObjectsClientContract, SavedObjectsFindResult, -} from 'kibana/server'; -import { SavedVisState } from 'src/plugins/visualizations/common'; -import { VIS_TYPE_TABLE } from '../../common'; +} from '../../../../core/server'; +import type { SavedVisState } from '../../../visualizations/common'; export interface VisTypeTableUsage { /** @@ -73,8 +74,8 @@ export async function getStats( const isSplitRow = params.row; const isSplitEnabled = hasSplitAgg.enabled; - const container = isSplitRow ? stats.split_rows : stats.split_columns; + container.total += 1; container.enabled = isSplitEnabled ? container.enabled + 1 : container.enabled; } diff --git a/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.test.ts b/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.test.ts index e045788897b61..d32435ac45406 100644 --- a/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.test.ts +++ b/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.test.ts @@ -6,20 +6,19 @@ * Side Public License, v 1. */ -jest.mock('./get_stats', () => ({ - getStats: jest.fn().mockResolvedValue({ somestat: 1 }), -})); - import { createUsageCollectionSetupMock, createCollectorFetchContextMock, -} from 'src/plugins/usage_collection/server/mocks'; - +} from '../../../usage_collection/server/mocks'; import { registerVisTypeTableUsageCollector } from './register_usage_collector'; import { getStats } from './get_stats'; +jest.mock('./get_stats', () => ({ + getStats: jest.fn().mockResolvedValue({ somestat: 1 }), +})); + describe('registerVisTypeTableUsageCollector', () => { - it('Usage collector configs fit the shape', () => { + test('Usage collector configs fit the shape', () => { const mockCollectorSet = createUsageCollectionSetupMock(); registerVisTypeTableUsageCollector(mockCollectorSet); expect(mockCollectorSet.makeUsageCollector).toBeCalledTimes(1); @@ -45,7 +44,7 @@ describe('registerVisTypeTableUsageCollector', () => { expect(usageCollectorConfig.isReady()).toBe(true); }); - it('Usage collector config.fetch calls getStats', async () => { + test('Usage collector config.fetch calls getStats', async () => { const mockCollectorSet = createUsageCollectionSetupMock(); registerVisTypeTableUsageCollector(mockCollectorSet); const usageCollector = mockCollectorSet.makeUsageCollector.mock.results[0].value; diff --git a/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.ts b/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.ts index d3d3204e0841c..74044c9ae70c0 100644 --- a/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.ts +++ b/src/plugins/vis_type_table/server/usage_collector/register_usage_collector.ts @@ -6,9 +6,8 @@ * Side Public License, v 1. */ -import { UsageCollectionSetup } from 'src/plugins/usage_collection/server'; - import { getStats, VisTypeTableUsage } from './get_stats'; +import type { UsageCollectionSetup } from '../../../usage_collection/server'; export function registerVisTypeTableUsageCollector(collectorSet: UsageCollectionSetup) { const collector = collectorSet.makeUsageCollector({ From 4a74c6afc094488edca7f0c4ed137e63e34e8051 Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Fri, 27 Aug 2021 16:54:23 +0300 Subject: [PATCH 6/7] add namespaces: ['*'], --- src/plugins/vis_type_table/server/usage_collector/get_stats.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts index 704e8d5f08763..ef948c2d7b70b 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.ts @@ -49,6 +49,7 @@ export async function getStats( const finder = await soClient.createPointInTimeFinder({ type: 'visualization', perPage: 1000, + namespaces: ['*'], }); const stats: VisTypeTableUsage = { From 40d276ba941cc6c545189399a42d616ae142b526 Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Mon, 30 Aug 2021 11:43:25 +0300 Subject: [PATCH 7/7] fix jest --- .../vis_type_table/server/usage_collector/get_stats.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts index f1dfd231fd29c..76f067e3a23d7 100644 --- a/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts +++ b/src/plugins/vis_type_table/server/usage_collector/get_stats.test.ts @@ -56,6 +56,7 @@ describe('vis_type_table getStats', () => { expect(mockSoClient.createPointInTimeFinder).toHaveBeenCalledWith({ type: 'visualization', perPage: 1000, + namespaces: ['*'], }); expect(result).toEqual({