From 237173dfbbc70be0f0f3fb6a9f7e1b3d9038ec4e Mon Sep 17 00:00:00 2001 From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Date: Thu, 6 Oct 2022 02:22:27 -0600 Subject: [PATCH] [Profiling] Auto-abort requests (#142750) (#142817) (cherry picked from commit c05190e51648268e00d28dba79ab860c2bf9b330) Co-authored-by: Dario Gieselaar --- .../components/flame_graphs_view/index.tsx | 64 ++++++++++--------- .../public/components/flamegraph.tsx | 2 - .../components/functions_view/index.tsx | 50 +++++++++------ .../components/stack_traces_view/index.tsx | 48 +++++++------- .../profiling/public/hooks/use_async.ts | 56 ++++++++++++++-- x-pack/plugins/profiling/public/plugin.tsx | 2 +- x-pack/plugins/profiling/public/services.ts | 41 ++++-------- .../profiling/server/routes/flamechart.ts | 11 +--- .../profiling/server/routes/functions.ts | 11 +--- .../plugins/profiling/server/routes/topn.ts | 12 +--- .../utils/handle_route_error_handler.ts | 41 ++++++++++++ 11 files changed, 204 insertions(+), 134 deletions(-) create mode 100644 x-pack/plugins/profiling/server/utils/handle_route_error_handler.ts diff --git a/x-pack/plugins/profiling/public/components/flame_graphs_view/index.tsx b/x-pack/plugins/profiling/public/components/flame_graphs_view/index.tsx index 6a8802599f6cc..dab912902ba46 100644 --- a/x-pack/plugins/profiling/public/components/flame_graphs_view/index.tsx +++ b/x-pack/plugins/profiling/public/components/flame_graphs_view/index.tsx @@ -43,35 +43,40 @@ export function FlameGraphsView({ children }: { children: React.ReactElement }) services: { fetchElasticFlamechart }, } = useProfilingDependencies(); - const state = useTimeRangeAsync(() => { - return Promise.all([ - fetchElasticFlamechart({ - timeFrom: new Date(timeRange.start).getTime() / 1000, - timeTo: new Date(timeRange.end).getTime() / 1000, - kuery, - }), - comparisonTimeRange.start && comparisonTimeRange.end - ? fetchElasticFlamechart({ - timeFrom: new Date(comparisonTimeRange.start).getTime() / 1000, - timeTo: new Date(comparisonTimeRange.end).getTime() / 1000, - kuery: comparisonKuery, - }) - : Promise.resolve(undefined), - ]).then(([primaryFlamegraph, comparisonFlamegraph]) => { - return { - primaryFlamegraph, - comparisonFlamegraph, - }; - }); - }, [ - timeRange.start, - timeRange.end, - kuery, - comparisonTimeRange.start, - comparisonTimeRange.end, - comparisonKuery, - fetchElasticFlamechart, - ]); + const state = useTimeRangeAsync( + ({ http }) => { + return Promise.all([ + fetchElasticFlamechart({ + http, + timeFrom: new Date(timeRange.start).getTime() / 1000, + timeTo: new Date(timeRange.end).getTime() / 1000, + kuery, + }), + comparisonTimeRange.start && comparisonTimeRange.end + ? fetchElasticFlamechart({ + http, + timeFrom: new Date(comparisonTimeRange.start).getTime() / 1000, + timeTo: new Date(comparisonTimeRange.end).getTime() / 1000, + kuery: comparisonKuery, + }) + : Promise.resolve(undefined), + ]).then(([primaryFlamegraph, comparisonFlamegraph]) => { + return { + primaryFlamegraph, + comparisonFlamegraph, + }; + }); + }, + [ + timeRange.start, + timeRange.end, + kuery, + comparisonTimeRange.start, + comparisonTimeRange.end, + comparisonKuery, + fetchElasticFlamechart, + ] + ); const { data } = state; @@ -173,7 +178,6 @@ export function FlameGraphsView({ children }: { children: React.ReactElement }) = ({ id, - height, comparisonMode, primaryFlamegraph, comparisonFlamegraph, diff --git a/x-pack/plugins/profiling/public/components/functions_view/index.tsx b/x-pack/plugins/profiling/public/components/functions_view/index.tsx index 94410f961c469..2c7b25754f3d9 100644 --- a/x-pack/plugins/profiling/public/components/functions_view/index.tsx +++ b/x-pack/plugins/profiling/public/components/functions_view/index.tsx @@ -42,28 +42,36 @@ export function FunctionsView({ children }: { children: React.ReactElement }) { services: { fetchTopNFunctions }, } = useProfilingDependencies(); - const state = useTimeRangeAsync(() => { - return fetchTopNFunctions({ - timeFrom: new Date(timeRange.start).getTime() / 1000, - timeTo: new Date(timeRange.end).getTime() / 1000, - startIndex: 0, - endIndex: 1000, - kuery, - }); - }, [timeRange.start, timeRange.end, kuery, fetchTopNFunctions]); + const state = useTimeRangeAsync( + ({ http }) => { + return fetchTopNFunctions({ + http, + timeFrom: new Date(timeRange.start).getTime() / 1000, + timeTo: new Date(timeRange.end).getTime() / 1000, + startIndex: 0, + endIndex: 1000, + kuery, + }); + }, + [timeRange.start, timeRange.end, kuery, fetchTopNFunctions] + ); - const comparisonState = useTimeRangeAsync(() => { - if (!comparisonTimeRange.start || !comparisonTimeRange.end) { - return undefined; - } - return fetchTopNFunctions({ - timeFrom: new Date(comparisonTimeRange.start).getTime() / 1000, - timeTo: new Date(comparisonTimeRange.end).getTime() / 1000, - startIndex: 0, - endIndex: 1000, - kuery: comparisonKuery, - }); - }, [comparisonTimeRange.start, comparisonTimeRange.end, comparisonKuery, fetchTopNFunctions]); + const comparisonState = useTimeRangeAsync( + ({ http }) => { + if (!comparisonTimeRange.start || !comparisonTimeRange.end) { + return undefined; + } + return fetchTopNFunctions({ + http, + timeFrom: new Date(comparisonTimeRange.start).getTime() / 1000, + timeTo: new Date(comparisonTimeRange.end).getTime() / 1000, + startIndex: 0, + endIndex: 1000, + kuery: comparisonKuery, + }); + }, + [comparisonTimeRange.start, comparisonTimeRange.end, comparisonKuery, fetchTopNFunctions] + ); const routePath = useProfilingRoutePath() as | '/functions' diff --git a/x-pack/plugins/profiling/public/components/stack_traces_view/index.tsx b/x-pack/plugins/profiling/public/components/stack_traces_view/index.tsx index 23bb4a7bd4b6f..19834410c6b7a 100644 --- a/x-pack/plugins/profiling/public/components/stack_traces_view/index.tsx +++ b/x-pack/plugins/profiling/public/components/stack_traces_view/index.tsx @@ -50,29 +50,33 @@ export function StackTracesView() { rangeTo, }); - const state = useTimeRangeAsync(() => { - if (!topNType) { - return Promise.resolve({ charts: [], metadata: {} }); - } - return fetchTopN({ - type: topNType, - timeFrom: new Date(timeRange.start).getTime() / 1000, - timeTo: new Date(timeRange.end).getTime() / 1000, - kuery, - }).then((response: TopNResponse) => { - const totalCount = response.TotalCount; - const samples = response.TopN; - const charts = groupSamplesByCategory({ - samples, - totalCount, - metadata: response.Metadata, - labels: response.Labels, + const state = useTimeRangeAsync( + ({ http }) => { + if (!topNType) { + return Promise.resolve({ charts: [], metadata: {} }); + } + return fetchTopN({ + http, + type: topNType, + timeFrom: new Date(timeRange.start).getTime() / 1000, + timeTo: new Date(timeRange.end).getTime() / 1000, + kuery, + }).then((response: TopNResponse) => { + const totalCount = response.TotalCount; + const samples = response.TopN; + const charts = groupSamplesByCategory({ + samples, + totalCount, + metadata: response.Metadata, + labels: response.Labels, + }); + return { + charts, + }; }); - return { - charts, - }; - }); - }, [topNType, timeRange.start, timeRange.end, fetchTopN, kuery]); + }, + [topNType, timeRange.start, timeRange.end, fetchTopN, kuery] + ); const [highlightedSubchart, setHighlightedSubchart] = useState( undefined diff --git a/x-pack/plugins/profiling/public/hooks/use_async.ts b/x-pack/plugins/profiling/public/hooks/use_async.ts index ea6da578c3879..7156b4e8bfffd 100644 --- a/x-pack/plugins/profiling/public/hooks/use_async.ts +++ b/x-pack/plugins/profiling/public/hooks/use_async.ts @@ -4,8 +4,10 @@ * 2.0; you may not use this file except in compliance with the Elastic License * 2.0. */ -import { HttpStart } from '@kbn/core-http-browser'; -import { useEffect, useState } from 'react'; +import { HttpFetchOptions, HttpHandler, HttpStart } from '@kbn/core-http-browser'; +import { AbortError } from '@kbn/kibana-utils-plugin/common'; +import { useEffect, useRef, useState } from 'react'; +import { Overwrite, ValuesType } from 'utility-types'; import { useProfilingDependencies } from '../components/contexts/profiling_dependencies/use_profiling_dependencies'; export enum AsyncStatus { @@ -20,8 +22,22 @@ export interface AsyncState { status: AsyncStatus; } +const HTTP_METHODS = ['fetch', 'get', 'post', 'put', 'delete', 'patch'] as const; + +type HttpMethod = ValuesType; + +type AutoAbortedHttpMethod = ( + path: string, + options: Omit +) => ReturnType; + +export type AutoAbortedHttpService = Overwrite< + HttpStart, + Record +>; + export type UseAsync = ( - fn: ({ http }: { http: HttpStart }) => Promise | undefined, + fn: ({ http }: { http: AutoAbortedHttpService }) => Promise | undefined, dependencies: any[] ) => AsyncState; @@ -37,8 +53,30 @@ export const useAsync: UseAsync = (fn, dependencies) => { const { data, error } = asyncState; + const controllerRef = useRef(new AbortController()); + useEffect(() => { - const returnValue = fn({ http }); + controllerRef.current.abort(); + + controllerRef.current = new AbortController(); + + const autoAbortedMethods = {} as Record; + + for (const key of HTTP_METHODS) { + autoAbortedMethods[key] = (path, options) => { + return http[key](path, { ...options, signal: controllerRef.current.signal }).catch( + (err) => { + if (err.name === 'AbortError') { + // return never-resolving promise + return new Promise(() => {}); + } + throw err; + } + ); + }; + } + + const returnValue = fn({ http: { ...http, ...autoAbortedMethods } }); if (returnValue === undefined) { setAsyncState({ @@ -63,13 +101,23 @@ export const useAsync: UseAsync = (fn, dependencies) => { }); returnValue.catch((nextError) => { + if (nextError instanceof AbortError) { + return; + } setAsyncState({ status: AsyncStatus.Settled, error: nextError, }); + throw nextError; }); // eslint-disable-next-line react-hooks/exhaustive-deps }, [http, ...dependencies]); + useEffect(() => { + return () => { + controllerRef.current.abort(); + }; + }, []); + return asyncState; }; diff --git a/x-pack/plugins/profiling/public/plugin.tsx b/x-pack/plugins/profiling/public/plugin.tsx index 080941729b014..ead023a079e29 100644 --- a/x-pack/plugins/profiling/public/plugin.tsx +++ b/x-pack/plugins/profiling/public/plugin.tsx @@ -90,7 +90,7 @@ export class ProfilingPlugin implements Plugin { unknown ]; - const profilingFetchServices = getServices(coreStart); + const profilingFetchServices = getServices(); const { renderApp } = await import('./app'); function pushKueryToSubject(location: Location) { diff --git a/x-pack/plugins/profiling/public/services.ts b/x-pack/plugins/profiling/public/services.ts index a1f345dc96a2c..4dcad550d0bbb 100644 --- a/x-pack/plugins/profiling/public/services.ts +++ b/x-pack/plugins/profiling/public/services.ts @@ -4,21 +4,23 @@ * 2.0; you may not use this file except in compliance with the Elastic License * 2.0. */ - -import { CoreStart, HttpFetchQuery } from '@kbn/core/public'; +import { HttpFetchQuery } from '@kbn/core/public'; import { getRoutePaths } from '../common'; import { BaseFlameGraph, createFlameGraph, ElasticFlameGraph } from '../common/flamegraph'; import { TopNFunctions } from '../common/functions'; import { TopNResponse } from '../common/topn'; +import { AutoAbortedHttpService } from './hooks/use_async'; export interface Services { fetchTopN: (params: { + http: AutoAbortedHttpService; type: string; timeFrom: number; timeTo: number; kuery: string; }) => Promise; fetchTopNFunctions: (params: { + http: AutoAbortedHttpService; timeFrom: number; timeTo: number; startIndex: number; @@ -26,42 +28,31 @@ export interface Services { kuery: string; }) => Promise; fetchElasticFlamechart: (params: { + http: AutoAbortedHttpService; timeFrom: number; timeTo: number; kuery: string; }) => Promise; } -export function getServices(core: CoreStart): Services { +export function getServices(): Services { const paths = getRoutePaths(); return { - fetchTopN: async ({ type, timeFrom, timeTo, kuery }) => { + fetchTopN: async ({ http, type, timeFrom, timeTo, kuery }) => { try { const query: HttpFetchQuery = { timeFrom, timeTo, kuery, }; - return await core.http.get(`${paths.TopN}/${type}`, { query }); + return await http.get(`${paths.TopN}/${type}`, { query }); } catch (e) { return e; } }, - fetchTopNFunctions: async ({ - timeFrom, - timeTo, - startIndex, - endIndex, - kuery, - }: { - timeFrom: number; - timeTo: number; - startIndex: number; - endIndex: number; - kuery: string; - }) => { + fetchTopNFunctions: async ({ http, timeFrom, timeTo, startIndex, endIndex, kuery }) => { try { const query: HttpFetchQuery = { timeFrom, @@ -70,28 +61,20 @@ export function getServices(core: CoreStart): Services { endIndex, kuery, }; - return await core.http.get(paths.TopNFunctions, { query }); + return await http.get(paths.TopNFunctions, { query }); } catch (e) { return e; } }, - fetchElasticFlamechart: async ({ - timeFrom, - timeTo, - kuery, - }: { - timeFrom: number; - timeTo: number; - kuery: string; - }) => { + fetchElasticFlamechart: async ({ http, timeFrom, timeTo, kuery }) => { try { const query: HttpFetchQuery = { timeFrom, timeTo, kuery, }; - const baseFlamegraph: BaseFlameGraph = await core.http.get(paths.Flamechart, { query }); + const baseFlamegraph = (await http.get(paths.Flamechart, { query })) as BaseFlameGraph; return createFlameGraph(baseFlamegraph); } catch (e) { return e; diff --git a/x-pack/plugins/profiling/server/routes/flamechart.ts b/x-pack/plugins/profiling/server/routes/flamechart.ts index 772d505817484..49f0415049ad7 100644 --- a/x-pack/plugins/profiling/server/routes/flamechart.ts +++ b/x-pack/plugins/profiling/server/routes/flamechart.ts @@ -9,6 +9,7 @@ import { schema } from '@kbn/config-schema'; import { RouteRegisterParameters } from '.'; import { getRoutePaths } from '../../common'; import { createCalleeTree } from '../../common/callee'; +import { handleRouteHandlerError } from '../utils/handle_route_error_handler'; import { createBaseFlameGraph } from '../../common/flamegraph'; import { createProfilingEsClient } from '../utils/create_profiling_es_client'; import { withProfilingSpan } from '../utils/with_profiling_span'; @@ -71,14 +72,8 @@ export function registerFlameChartSearchRoute({ router, logger }: RouteRegisterP logger.info('returning payload response to client'); return response.ok({ body: flamegraph }); - } catch (e) { - logger.error(e); - return response.customError({ - statusCode: e.statusCode ?? 500, - body: { - message: e.message, - }, - }); + } catch (error) { + return handleRouteHandlerError({ error, logger, response }); } } ); diff --git a/x-pack/plugins/profiling/server/routes/functions.ts b/x-pack/plugins/profiling/server/routes/functions.ts index adbbc59003376..9d3eed222b908 100644 --- a/x-pack/plugins/profiling/server/routes/functions.ts +++ b/x-pack/plugins/profiling/server/routes/functions.ts @@ -9,6 +9,7 @@ import { schema, TypeOf } from '@kbn/config-schema'; import { RouteRegisterParameters } from '.'; import { getRoutePaths } from '../../common'; import { createTopNFunctions } from '../../common/functions'; +import { handleRouteHandlerError } from '../utils/handle_route_error_handler'; import { createProfilingEsClient } from '../utils/create_profiling_es_client'; import { withProfilingSpan } from '../utils/with_profiling_span'; import { getClient } from './compat'; @@ -72,14 +73,8 @@ export function registerTopNFunctionsSearchRoute({ router, logger }: RouteRegist return response.ok({ body: topNFunctions, }); - } catch (e) { - logger.error(e); - return response.customError({ - statusCode: e.statusCode ?? 500, - body: { - message: e.message, - }, - }); + } catch (error) { + return handleRouteHandlerError({ error, logger, response }); } } ); diff --git a/x-pack/plugins/profiling/server/routes/topn.ts b/x-pack/plugins/profiling/server/routes/topn.ts index 790f40049e167..a8a7efc01bb52 100644 --- a/x-pack/plugins/profiling/server/routes/topn.ts +++ b/x-pack/plugins/profiling/server/routes/topn.ts @@ -14,6 +14,7 @@ import { computeBucketWidthFromTimeRangeAndBucketCount } from '../../common/hist import { groupStackFrameMetadataByStackTrace, StackTraceID } from '../../common/profiling'; import { getFieldNameForTopNType, TopNType } from '../../common/stack_traces'; import { createTopNSamples, getTopNAggregationRequest, TopNResponse } from '../../common/topn'; +import { handleRouteHandlerError } from '../utils/handle_route_error_handler'; import { ProfilingRequestHandlerContext } from '../types'; import { createProfilingEsClient, ProfilingESClient } from '../utils/create_profiling_es_client'; import { withProfilingSpan } from '../utils/with_profiling_span'; @@ -189,15 +190,8 @@ export function queryTopNCommon( kuery, }), }); - } catch (e) { - logger.error(e); - - return response.customError({ - statusCode: e.statusCode ?? 500, - body: { - message: 'Profiling TopN request failed: ' + e.message + '; full error ' + e.toString(), - }, - }); + } catch (error) { + return handleRouteHandlerError({ error, logger, response }); } } ); diff --git a/x-pack/plugins/profiling/server/utils/handle_route_error_handler.ts b/x-pack/plugins/profiling/server/utils/handle_route_error_handler.ts new file mode 100644 index 0000000000000..782beeeae15cb --- /dev/null +++ b/x-pack/plugins/profiling/server/utils/handle_route_error_handler.ts @@ -0,0 +1,41 @@ +/* + * 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 { KibanaResponseFactory } from '@kbn/core-http-server'; +import { Logger } from '@kbn/logging'; +import { WrappedElasticsearchClientError } from '@kbn/observability-plugin/server'; +import { errors } from '@elastic/elasticsearch'; + +export function handleRouteHandlerError({ + error, + logger, + response, +}: { + error: any; + response: KibanaResponseFactory; + logger: Logger; +}) { + if ( + error instanceof WrappedElasticsearchClientError && + error.originalError instanceof errors.RequestAbortedError + ) { + return response.custom({ + statusCode: 499, + body: { + message: 'Client closed request', + }, + }); + } + logger.error(error); + + return response.customError({ + statusCode: error.statusCode ?? 500, + body: { + message: error.message, + }, + }); +}