From b47b3dc5330ae1cf171bbdae2246ad6cc218f218 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Tue, 17 Sep 2024 11:33:04 -0400 Subject: [PATCH] [Synthetics] Improve loading state for metric items (#192930) ## Summary There are some cases where the latest addition to the overview grid causes metric items not to load for very small numbers of monitors. This PR aims to fix that issue, and to add a more robust loading state to the individual items to make it easier to track when there is a request in-flight. Additionally, it adds a type guard to the server code and an io-ts codec for verifying the safety of the code we use to generate our server responses. ## Testing Ensure that you see metric items load their duration graphs for small numbers (3 or less) of monitors, and that scrolling etc., continues working for larger ones. --------- Co-authored-by: Elastic Machine (cherry picked from commit 7a7888bfb8e5472d7c0656283bc1acff89ad9f5a) --- .../synthetics/common/types/overview.ts | 17 ++- .../overview/overview/metric_item.tsx | 37 ++--- .../metric_item/metric_item_extra.test.tsx | 40 ++++++ .../metric_item/metric_item_extra.tsx | 47 ++++--- .../overview/overview/overview_grid.tsx | 2 +- .../apps/synthetics/state/overview/actions.ts | 4 +- .../synthetics/state/overview/effects.test.ts | 28 ++-- .../apps/synthetics/state/overview/effects.ts | 33 +++-- .../apps/synthetics/state/overview/index.ts | 16 ++- .../overview_trends/overview_trends.test.ts | 126 ++++++++++++++++++ .../routes/overview_trends/overview_trends.ts | 65 +++++---- 11 files changed, 315 insertions(+), 100 deletions(-) create mode 100644 x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.test.tsx create mode 100644 x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.test.ts diff --git a/x-pack/plugins/observability_solution/synthetics/common/types/overview.ts b/x-pack/plugins/observability_solution/synthetics/common/types/overview.ts index 4982f0f408fcf..ad91fedeccdf1 100644 --- a/x-pack/plugins/observability_solution/synthetics/common/types/overview.ts +++ b/x-pack/plugins/observability_solution/synthetics/common/types/overview.ts @@ -22,11 +22,16 @@ export interface OverviewTrend { locationId: string; data: TrendDatum[]; count: number; - min: number; - max: number; - avg: number; - sum: number; - median: number; + min: number | null; + max: number | null; + avg: number | null; + sum: number | null; + median: number | null; } -export type TrendTable = Record; +export type TrendTable = Record; + +export interface GetTrendPayload { + trendStats: TrendTable; + batch: TrendRequest[]; +} diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item.tsx b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item.tsx index 57c50bc54a5bf..4f8a97c6b8915 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item.tsx +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item.tsx @@ -141,26 +141,27 @@ export const MetricItem = ({ { title: monitor.name, subtitle: locationName, - value: trendData?.median ?? 0, + value: trendData !== 'loading' ? trendData?.median ?? 0 : 0, trendShape: MetricTrendShape.Area, - trend: trendData?.data ?? [], - extra: trendData ? ( - - ) : ( -
- -
- ), + ) : trendData === 'loading' ? ( +
+ +
+ ) : undefined, valueFormatter: (d: number) => formatDuration(d), color: getColor(theme, monitor.isEnabled, status), body: , diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.test.tsx b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.test.tsx new file mode 100644 index 0000000000000..4270d2a1967c9 --- /dev/null +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.test.tsx @@ -0,0 +1,40 @@ +/* + * 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; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from 'react'; +import { render } from '../../../../../utils/testing/rtl_helpers'; +import { MetricItemExtra } from './metric_item_extra'; +import { fireEvent, waitFor } from '@testing-library/dom'; + +describe('', () => { + it('renders the tooltip when there is content', async () => { + const { getByText } = render( + + ); + expect(getByText('Duration')).toBeInTheDocument(); + fireEvent.mouseOver(getByText('Info')); + await waitFor(() => expect(getByText('Median duration of last 50 checks')).toBeInTheDocument()); + }); + + it('renders the empty tooltip when there is no content', async () => { + const { getByText } = render( + + ); + expect(getByText('Duration')).toBeInTheDocument(); + fireEvent.mouseOver(getByText('Info')); + await waitFor(() => expect(getByText('Metric data is not available')).toBeInTheDocument()); + }); +}); diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.tsx b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.tsx index 584124bf22747..2383c840dd088 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.tsx +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_extra.tsx @@ -14,10 +14,10 @@ export const MetricItemExtra = ({ stats, }: { stats: { - medianDuration: number; - avgDuration: number; - minDuration: number; - maxDuration: number; + medianDuration: number | null; + avgDuration: number | null; + minDuration: number | null; + maxDuration: number | null; }; }) => { const { avgDuration, minDuration, maxDuration } = stats; @@ -36,20 +36,31 @@ export const MetricItemExtra = ({ })} - + {avgDuration && minDuration && maxDuration ? ( + + ) : ( + + )} ); diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/overview_grid.tsx b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/overview_grid.tsx index 691556a94278e..ec453ba30c973 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/overview_grid.tsx +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/components/monitors_page/overview/overview/overview_grid.tsx @@ -93,7 +93,7 @@ export const OverviewGrid = memo(() => { ); useEffect(() => { - if (monitorsSortedByStatus.length && maxItem) { + if (monitorsSortedByStatus.length) { const batch: TrendRequest[] = []; const chunk = monitorsSortedByStatus.slice(0, (maxItem + 1) * ROW_COUNT); for (const item of chunk) { diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/actions.ts b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/actions.ts index b5098aaa7cbf6..d7970f720e803 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/actions.ts +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/actions.ts @@ -5,7 +5,7 @@ * 2.0. */ import { createAction } from '@reduxjs/toolkit'; -import { TrendRequest, TrendTable } from '../../../../../common/types'; +import { GetTrendPayload, TrendRequest, TrendTable } from '../../../../../common/types'; import { createAsyncAction } from '../utils/actions'; import type { @@ -39,6 +39,6 @@ export const refreshOverviewTrends = createAsyncAction( 'refreshOverviewTrendStats' ); -export const trendStatsBatch = createAsyncAction( +export const trendStatsBatch = createAsyncAction( 'batchTrendStats' ); diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.test.ts b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.test.ts index ceef406ebbd1b..4a44b2d56c11c 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.test.ts +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.test.ts @@ -7,7 +7,7 @@ import sagaHelper from 'redux-saga-testing'; import { call, put, select } from 'redux-saga/effects'; -import { TrendKey, TrendRequest, TrendTable } from '../../../../../common/types'; +import { GetTrendPayload, TrendKey, TrendRequest, TrendTable } from '../../../../../common/types'; import { TRENDS_CHUNK_SIZE, fetchTrendEffect, refreshTrends } from './effects'; import { trendStatsBatch } from './actions'; import { fetchOverviewTrendStats as trendsApi } from './api'; @@ -48,7 +48,9 @@ describe('overview effects', () => { }); it('sends trends stats success action', (putResult) => { - expect(putResult).toEqual(put(trendStatsBatch.success(firstChunkResponse))); + expect(putResult).toEqual( + put(trendStatsBatch.success({ trendStats: firstChunkResponse, batch: firstChunk })) + ); }); it('calls the api for the second chunk', (callResult) => { @@ -57,7 +59,9 @@ describe('overview effects', () => { }); it('sends trends stats success action', (putResult) => { - expect(putResult).toEqual(put(trendStatsBatch.success(secondChunkResponse))); + expect(putResult).toEqual( + put(trendStatsBatch.success({ trendStats: secondChunkResponse, batch: secondChunk })) + ); }); it('terminates', (result) => { @@ -111,6 +115,10 @@ describe('overview effects', () => { }, }; + const batch = [ + { configId: 'monitor1', locationId: 'location', schedule: '3' }, + { configId: 'monitor3', locationId: 'location', schedule: '3' }, + ]; const apiResponse: TrendTable = { monitor1: { configId: 'monitor1', @@ -142,6 +150,11 @@ describe('overview effects', () => { }, }; + const successPayload: GetTrendPayload = { + trendStats: apiResponse, + batch, + }; + it('selects the trends in the table', (selectResult) => { expect(selectResult).toEqual(select(selectOverviewTrends)); @@ -161,18 +174,13 @@ describe('overview effects', () => { }); it('calls the api for the first chunk', (callResult) => { - expect(callResult).toEqual( - call(trendsApi, [ - { configId: 'monitor1', locationId: 'location', schedule: '3' }, - { configId: 'monitor3', locationId: 'location', schedule: '3' }, - ]) - ); + expect(callResult).toEqual(call(trendsApi, batch)); return apiResponse; }); it('sends trends stats success action', (putResult) => { - expect(putResult).toEqual(put(trendStatsBatch.success(apiResponse))); + expect(putResult).toEqual(put(trendStatsBatch.success(successPayload))); }); }); }); diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.ts b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.ts index 930c729bde4f7..591527435afb8 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.ts +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/effects.ts @@ -6,7 +6,7 @@ */ import { debounce, call, takeLeading, takeEvery, put, select } from 'redux-saga/effects'; -import type { TrendTable } from '../../../../../common/types'; +import type { OverviewTrend, TrendTable } from '../../../../../common/types'; import { fetchEffectFactory } from '../utils/fetch_effect'; import { selectOverviewState, selectOverviewTrends } from './selectors'; import { @@ -41,11 +41,11 @@ export function* fetchTrendEffect( const chunk = action.payload.slice(Math.max(i - TRENDS_CHUNK_SIZE, 0), i); if (chunk.length > 0) { const trendStats = yield call(trendsApi, chunk); - yield put(trendStatsBatch.success(trendStats)); + yield put(trendStatsBatch.success({ trendStats, batch: chunk })); } } } catch (e: any) { - yield put(trendStatsBatch.fail(e)); + yield put(trendStatsBatch.fail(action.payload)); } } @@ -57,7 +57,6 @@ export function* refreshTrends(): Generator { const existingTrends: TrendTable = yield select(selectOverviewTrends); const overviewState: MonitorOverviewState = yield select(selectOverviewState); - let acc = {}; const keys = Object.keys(existingTrends); while (keys.length) { const chunk = keys @@ -65,25 +64,25 @@ export function* refreshTrends(): Generator { .filter( (key: string) => existingTrends[key] !== null && + existingTrends[key] !== 'loading' && overviewState.data.monitors.some( - ({ configId }) => configId === existingTrends[key]!.configId + ({ configId }) => configId === (existingTrends[key] as OverviewTrend)!.configId ) ) - .map((key: string) => ({ - configId: existingTrends[key]!.configId, - locationId: existingTrends[key]!.locationId, - schedule: overviewState.data.monitors.find( - ({ configId }) => configId === existingTrends[key]!.configId - )!.schedule, - })); + .map((key: string) => { + const trend = existingTrends[key] as OverviewTrend; + return { + configId: trend.configId, + locationId: trend.locationId, + schedule: overviewState.data.monitors.find(({ configId }) => configId === trend.configId)! + .schedule, + }; + }); if (chunk.length) { - const res = yield call(trendsApi, chunk); - acc = { ...acc, ...res }; + const trendStats = yield call(trendsApi, chunk); + yield put(trendStatsBatch.success({ trendStats, batch: chunk })); } } - if (Object.keys(acc).length) { - yield put(trendStatsBatch.success(acc)); - } } export function* refreshOverviewTrendStats() { diff --git a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/index.ts b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/index.ts index 74ad8bb7a2a57..fef85b1cbeb9d 100644 --- a/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/index.ts +++ b/x-pack/plugins/observability_solution/synthetics/public/apps/synthetics/state/overview/index.ts @@ -99,13 +99,25 @@ export const monitorOverviewReducer = createReducer(initialState, (builder) => { .addCase(trendStatsBatch.get, (state, action) => { for (const { configId, locationId } of action.payload) { if (!state.trendStats[configId + locationId]) { + state.trendStats[configId + locationId] = 'loading'; + } + } + }) + .addCase(trendStatsBatch.fail, (state, action) => { + for (const { configId, locationId } of action.payload) { + if (state.trendStats[configId + locationId] === 'loading') { state.trendStats[configId + locationId] = null; } } }) .addCase(trendStatsBatch.success, (state, action) => { - for (const key of Object.keys(action.payload)) { - state.trendStats[key] = action.payload[key]; + for (const key of Object.keys(action.payload.trendStats)) { + state.trendStats[key] = action.payload.trendStats[key]; + } + for (const { configId, locationId } of action.payload.batch) { + if (!action.payload.trendStats[configId + locationId]) { + state.trendStats[configId + locationId] = null; + } } }); }); diff --git a/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.test.ts b/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.test.ts new file mode 100644 index 0000000000000..bca95e35c7362 --- /dev/null +++ b/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.test.ts @@ -0,0 +1,126 @@ +/* + * 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; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import type { SyntheticsEsClient } from '../../lib'; +import { fetchTrends } from './overview_trends'; + +const mockEsClient: Partial = { + msearch: jest.fn(), +}; + +describe('fetchTrends', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should return correctly formatted trend data with valid input', async () => { + const configs = { + config1: { locations: ['location1'], interval: 10 }, + }; + + const stats = { avg: 15, min: 10, max: 20, sum: 30, count: 2 }; + + const mockResponse = { + responses: [ + { + aggregations: { + byId: { + buckets: [ + { + key: 'config1', + byLocation: { + buckets: [ + { + key: 'location1', + last50: { + buckets: [{ max: { value: 10 } }, { max: { value: 20 } }], + }, + stats, + median: { values: { '50.0': 18 } }, + }, + ], + }, + }, + ], + }, + }, + }, + ], + }; + + (mockEsClient.msearch as jest.Mock).mockResolvedValueOnce(mockResponse); + + const result = await fetchTrends(mockEsClient as SyntheticsEsClient, configs); + + const expectedOutput = { + config1location1: { + configId: 'config1', + locationId: 'location1', + data: [ + { x: 0, y: 10 }, + { x: 1, y: 20 }, + ], + ...stats, + median: 18, + }, + }; + + expect(result).toEqual(expectedOutput); + expect(mockEsClient.msearch).toHaveBeenCalledTimes(1); + }); + + it('should return an empty object when no responses are returned', async () => { + const configs = { + config1: { locations: ['location1'], interval: 10 }, + }; + + const mockResponse = { + responses: [], + }; + + (mockEsClient.msearch as jest.Mock).mockResolvedValueOnce(mockResponse); + + const result = await fetchTrends(mockEsClient as SyntheticsEsClient, configs); + + expect(result).toEqual({}); + expect(mockEsClient.msearch).toHaveBeenCalledTimes(1); + }); + + it('should handle missing aggregations and return an empty object', async () => { + const configs = { + config1: { locations: ['location1'], interval: 10 }, + }; + + const mockResponse = { + responses: [ + { + aggregations: null, + }, + ], + }; + + (mockEsClient.msearch as jest.Mock).mockResolvedValueOnce(mockResponse); + + const result = await fetchTrends(mockEsClient as SyntheticsEsClient, configs); + + expect(result).toEqual({}); + expect(mockEsClient.msearch).toHaveBeenCalledTimes(1); + }); + + it('should throw an error if msearch fails', async () => { + const configs = { + config1: { locations: ['location1'], interval: 10 }, + }; + + (mockEsClient.msearch as jest.Mock).mockRejectedValueOnce(new Error('Elasticsearch error')); + + await expect(fetchTrends(mockEsClient as SyntheticsEsClient, configs)).rejects.toThrow( + 'Elasticsearch error' + ); + expect(mockEsClient.msearch).toHaveBeenCalledTimes(1); + }); +}); diff --git a/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.ts b/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.ts index e3d7589160c02..d68fc71c59b11 100644 --- a/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.ts +++ b/x-pack/plugins/observability_solution/synthetics/server/routes/overview_trends/overview_trends.ts @@ -7,13 +7,50 @@ import { ObjectType, schema } from '@kbn/config-schema'; import { SYNTHETICS_API_URLS } from '../../../common/constants'; -import { TrendRequest, TrendTable } from '../../../common/types'; +import type { TrendRequest, TrendTable } from '../../../common/types'; import { getFetchTrendsQuery, TrendsQuery } from './fetch_trends'; import { SyntheticsRestApiRouteFactory } from '../types'; +import { SyntheticsEsClient } from '../../lib'; export const getIntervalForCheckCount = (schedule: string, numChecks = 50) => Number(schedule) * numChecks; +export async function fetchTrends( + esClient: SyntheticsEsClient, + configs: Record< + string, + { + locations: string[]; + interval: number; + } + > +): Promise { + const requests = Object.keys(configs).map( + (key) => getFetchTrendsQuery(key, configs[key].locations, configs[key].interval).body + ); + const results = await esClient.msearch(requests); + + return results.responses.reduce((table, res): TrendTable => { + res.aggregations?.byId.buckets.map(({ key, byLocation }) => { + const nTable: TrendTable = {}; + for (const location of byLocation.buckets) { + nTable[String(key) + String(location.key)] = { + configId: String(key), + locationId: String(location.key), + data: location.last50.buckets.map((durationBucket, x) => ({ + x, + y: durationBucket.max.value!, + })), + ...location.stats, + median: location.median.values['50.0']!, + }; + } + table = { ...table, ...nTable }; + }); + return table; + }, {}); +} + export const createOverviewTrendsRoute: SyntheticsRestApiRouteFactory = () => ({ method: 'POST', path: SYNTHETICS_API_URLS.OVERVIEW_TRENDS, @@ -45,30 +82,6 @@ export const createOverviewTrendsRoute: SyntheticsRestApiRouteFactory = () => ({ {} ); - const requests = Object.keys(configs).map( - (key) => getFetchTrendsQuery(key, configs[key].locations, configs[key].interval).body - ); - const results = await esClient.msearch(requests); - - let main = {}; - for (const res of results.responses) { - res.aggregations?.byId.buckets.map(({ key, byLocation }) => { - const ret: Record = {}; - for (const location of byLocation.buckets) { - ret[String(key) + String(location.key)] = { - configId: key, - locationId: location.key, - data: location.last50.buckets.map((durationBucket, x) => ({ - x, - y: durationBucket.max.value, - })), - ...location.stats, - median: location.median.values['50.0'], - }; - } - main = { ...main, ...ret }; - }); - } - return main; + return fetchTrends(esClient, configs); }, });