From 01e3a9af69e35ddb4061c4a20e1e39c9802e1ce0 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Mon, 23 Aug 2021 18:33:15 +0200 Subject: [PATCH 01/12] fix chart state for empty results. --- .../distribution/index.tsx | 29 +++++++++---- .../transaction_details/trace_samples_tab.tsx | 28 +++++++++---- .../shared/charts/chart_container.tsx | 4 +- .../transaction_distribution_chart/index.tsx | 42 ++++++++++++++----- .../use_transaction_distribution_fetcher.ts | 6 +++ 5 files changed, 80 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx index 86bef9917daf6..a3751e1e5e27e 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import React, { useEffect } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; import { BrushEndListener, XYBrushArea } from '@elastic/charts'; import { EuiBadge, @@ -21,7 +21,10 @@ import { getDurationFormatter } from '../../../../../common/utils/formatters'; import { useUrlParams } from '../../../../context/url_params_context/use_url_params'; import { useApmPluginContext } from '../../../../context/apm_plugin/use_apm_plugin_context'; import { useTransactionDistributionFetcher } from '../../../../hooks/use_transaction_distribution_fetcher'; -import { TransactionDistributionChart } from '../../../shared/charts/transaction_distribution_chart'; +import { + OnHasData, + TransactionDistributionChart, +} from '../../../shared/charts/transaction_distribution_chart'; import { useUiTracker } from '../../../../../../observability/public'; import { useApmServiceContext } from '../../../../context/apm_service/use_apm_service_context'; import { useApmParams } from '../../../../hooks/use_apm_params'; @@ -46,10 +49,11 @@ export function getFormattedSelection(selection: Selection): string { }`; } -interface Props { +interface TransactionDistributionProps { markerCurrentTransaction?: number; onChartSelection: BrushEndListener; onClearSelection: () => void; + onHasData: OnHasData; selection?: Selection; } @@ -57,8 +61,9 @@ export function TransactionDistribution({ markerCurrentTransaction, onChartSelection, onClearSelection, + onHasData, selection, -}: Props) { +}: TransactionDistributionProps) { const { core: { notifications }, } = useApmPluginContext(); @@ -73,6 +78,16 @@ export function TransactionDistribution({ const { transactionName, start, end } = urlParams; + const [showSelection, setShowSelection] = useState(false); + + const onTransactionDistributionHasData: OnHasData = useCallback( + (hasData) => { + setShowSelection(hasData); + onHasData(hasData); + }, + [onHasData] + ); + const emptySelectionText = i18n.translate( 'xpack.apm.transactionDetails.emptySelectionText', { @@ -114,7 +129,6 @@ export function TransactionDistribution({ return () => { // cancel any running async partial request when unmounting the component - // we want this effect to execute exactly once after the component mounts cancelFetch(); }; // eslint-disable-next-line react-hooks/exhaustive-deps @@ -163,7 +177,7 @@ export function TransactionDistribution({ - {!selection && ( + {showSelection && !selection && ( )} - {selection && ( + {showSelection && selection && ( diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/trace_samples_tab.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/trace_samples_tab.tsx index 0421fcd055d6c..ea02cfea5a941 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/trace_samples_tab.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/trace_samples_tab.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import React from 'react'; +import React, { useState } from 'react'; import { EuiSpacer } from '@elastic/eui'; @@ -34,11 +34,17 @@ function TraceSamplesTab({ status: waterfallStatus, } = useWaterfallFetcher(); + const [ + transactionDistributionHasData, + setTransactionDistributionHasData, + ] = useState(false); + return ( <> - + {transactionDistributionHasData && ( + <> + - + + + )} ); } diff --git a/x-pack/plugins/apm/public/components/shared/charts/chart_container.tsx b/x-pack/plugins/apm/public/components/shared/charts/chart_container.tsx index 4098fc5e696db..695e62b3b7d78 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/chart_container.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/chart_container.tsx @@ -10,7 +10,7 @@ import { i18n } from '@kbn/i18n'; import React from 'react'; import { FETCH_STATUS } from '../../../hooks/use_fetcher'; -interface Props { +export interface ChartContainerProps { hasData: boolean; status: FETCH_STATUS; height: number; @@ -24,7 +24,7 @@ export function ChartContainer({ status, hasData, id, -}: Props) { +}: ChartContainerProps) { if (!hasData && status === FETCH_STATUS.LOADING) { return ; } diff --git a/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx index c511a708058d3..f3ba45d9eef1a 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import React, { useMemo } from 'react'; +import React, { useEffect, useMemo } from 'react'; import { AnnotationDomainType, AreaSeries, @@ -35,7 +35,14 @@ import { HistogramItem } from '../../../../../common/search_strategies/correlati import { FETCH_STATUS } from '../../../../hooks/use_fetcher'; import { useTheme } from '../../../../hooks/use_theme'; -import { ChartContainer } from '../chart_container'; +import { ChartContainer, ChartContainerProps } from '../chart_container'; + +export type TransactionDistributionChartLoadingState = Pick< + ChartContainerProps, + 'hasData' | 'status' +>; + +export type OnHasData = (hasData: boolean) => void; interface TransactionDistributionChartProps { field?: string; @@ -46,6 +53,7 @@ interface TransactionDistributionChartProps { markerPercentile: number; overallHistogram?: HistogramItem[]; onChartSelection?: BrushEndListener; + onHasData?: OnHasData; selection?: [number, number]; } @@ -103,6 +111,7 @@ export function TransactionDistributionChart({ markerPercentile, overallHistogram, onChartSelection, + onHasData, selection, }: TransactionDistributionChartProps) { const chartTheme = useChartTheme(); @@ -154,6 +163,24 @@ export function TransactionDistributionChart({ ] : undefined; + const chartLoadingState: TransactionDistributionChartLoadingState = useMemo( + () => ({ + hasData: + Array.isArray(patchedOverallHistogram) && + patchedOverallHistogram.length > 0, + status: Array.isArray(patchedOverallHistogram) + ? FETCH_STATUS.SUCCESS + : FETCH_STATUS.LOADING, + }), + [patchedOverallHistogram] + ); + + useEffect(() => { + if (typeof onHasData === 'function') { + onHasData(chartLoadingState.hasData); + } + }, [chartLoadingState, onHasData]); + return (
0 - } - status={ - Array.isArray(patchedOverallHistogram) - ? FETCH_STATUS.SUCCESS - : FETCH_STATUS.LOADING - } + hasData={chartLoadingState.hasData} + status={chartLoadingState.status} > Date: Tue, 24 Aug 2021 12:47:10 +0200 Subject: [PATCH 02/12] unit tests --- .../search_strategies/correlations/types.ts | 9 ++ .../distribution/index.test.ts | 22 --- .../distribution/index.test.tsx | 152 ++++++++++++++++++ .../distribution/index.tsx | 2 +- .../use_transaction_distribution_fetcher.ts | 31 ++-- .../correlations/search_strategy.ts | 19 ++- 6 files changed, 186 insertions(+), 49 deletions(-) delete mode 100644 x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.ts create mode 100644 x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx diff --git a/x-pack/plugins/apm/common/search_strategies/correlations/types.ts b/x-pack/plugins/apm/common/search_strategies/correlations/types.ts index 703106628f561..886c5fd6161d8 100644 --- a/x-pack/plugins/apm/common/search_strategies/correlations/types.ts +++ b/x-pack/plugins/apm/common/search_strategies/correlations/types.ts @@ -52,3 +52,12 @@ export interface AsyncSearchProviderProgress { loadedFieldValuePairs: number; loadedHistograms: number; } + +export interface SearchServiceRawResponse { + ccsWarning: boolean; + log: string[]; + overallHistogram?: HistogramItem[]; + percentileThresholdValue?: number; + took: number; + values: SearchServiceValue[]; +} diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.ts b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.ts deleted file mode 100644 index f541c16e655ab..0000000000000 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.ts +++ /dev/null @@ -1,22 +0,0 @@ -/* - * 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 { getFormattedSelection } from './index'; - -describe('transaction_details/distribution', () => { - describe('getFormattedSelection', () => { - it('displays only one unit if from and to share the same unit', () => { - expect(getFormattedSelection([10000, 100000])).toEqual('10 - 100 ms'); - }); - - it('displays two units when from and to have different units', () => { - expect(getFormattedSelection([100000, 1000000000])).toEqual( - '100 ms - 17 min' - ); - }); - }); -}); diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx new file mode 100644 index 0000000000000..393810b1248af --- /dev/null +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx @@ -0,0 +1,152 @@ +/* + * 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 { render, screen, waitFor } from '@testing-library/react'; +import { createMemoryHistory } from 'history'; +import React, { ReactNode } from 'react'; +import { of } from 'rxjs'; + +import { CoreStart } from 'kibana/public'; +import { merge } from 'lodash'; +import { dataPluginMock } from 'src/plugins/data/public/mocks'; +import type { IKibanaSearchResponse } from 'src/plugins/data/public'; +import { EuiThemeProvider } from 'src/plugins/kibana_react/common'; +import { createKibanaReactContext } from 'src/plugins/kibana_react/public'; +import type { SearchServiceRawResponse } from '../../../../../common/search_strategies/correlations/types'; +import { MockUrlParamsContextProvider } from '../../../../context/url_params_context/mock_url_params_context_provider'; +import { ApmPluginContextValue } from '../../../../context/apm_plugin/apm_plugin_context'; +import { + mockApmPluginContextValue, + MockApmPluginContextWrapper, +} from '../../../../context/apm_plugin/mock_apm_plugin_context'; +import { fromQuery } from '../../../shared/Links/url_helpers'; + +import { getFormattedSelection, TransactionDistribution } from './index'; + +function Wrapper({ + children, + dataSearchResponse, +}: { + children?: ReactNode; + dataSearchResponse: IKibanaSearchResponse; +}) { + const mockDataSearch = jest.fn(() => of(dataSearchResponse)); + + const dataPluginMockStart = dataPluginMock.createStartContract(); + const KibanaReactContext = createKibanaReactContext({ + data: { + ...dataPluginMockStart, + search: { + ...dataPluginMockStart.search, + search: mockDataSearch, + }, + }, + usageCollection: { reportUiCounter: () => {} }, + } as Partial); + + const httpGet = jest.fn(); + + const history = createMemoryHistory(); + jest.spyOn(history, 'push'); + jest.spyOn(history, 'replace'); + + history.replace({ + pathname: '/services/the-service-name/transactions/view', + search: fromQuery({ transactionName: 'the-transaction-name' }), + }); + + const mockPluginContext = (merge({}, mockApmPluginContextValue, { + core: { http: { get: httpGet } }, + }) as unknown) as ApmPluginContextValue; + + return ( + + + + + {children} + + + + + ); +} + +describe('transaction_details/distribution', () => { + describe('getFormattedSelection', () => { + it('displays only one unit if from and to share the same unit', () => { + expect(getFormattedSelection([10000, 100000])).toEqual('10 - 100 ms'); + }); + + it('displays two units when from and to have different units', () => { + expect(getFormattedSelection([100000, 1000000000])).toEqual( + '100 ms - 17 min' + ); + }); + }); + + describe('TransactionDistribution', () => { + it('shows loading indicator when the service is running and returned no results yet', async () => { + const onHasData = jest.fn(); + render( + + + + ); + + await waitFor(() => { + expect(screen.getByTestId('apmCorrelationsChart')).toBeInTheDocument(); + expect(screen.getByTestId('loading')).toBeInTheDocument(); + expect(onHasData).toHaveBeenLastCalledWith(false); + }); + }); + it("doesn't show loading indicator when the service isn't running", async () => { + const onHasData = jest.fn(); + render( + + + + ); + + await waitFor(() => { + expect(screen.getByTestId('apmCorrelationsChart')).toBeInTheDocument(); + expect(screen.queryByTestId('loading')).toBeNull(); // it doesn't exist + // TODO Get the chart rendered into a state to display the no data message + // expect(screen.getByText('No data to display')).toBeInTheDocument(); + expect(onHasData).toHaveBeenLastCalledWith(false); + }); + }); + }); +}); diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx index a3751e1e5e27e..327c2cc3a7f8c 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx @@ -72,7 +72,7 @@ export function TransactionDistribution({ const { query: { kuery, environment }, - } = useApmParams('/services/:serviceName'); + } = useApmParams('/services/:serviceName/transactions/view'); const { urlParams } = useUrlParams(); diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts index 00de254f7313e..4389877fda90e 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts @@ -14,31 +14,21 @@ import { isErrorResponse, } from '../../../../../src/plugins/data/public'; import type { - HistogramItem, SearchServiceParams, - SearchServiceValue, + SearchServiceRawResponse, } from '../../common/search_strategies/correlations/types'; import { useKibana } from '../../../../../src/plugins/kibana_react/public'; import { ApmPluginStartDeps } from '../plugin'; -interface RawResponse { - percentileThresholdValue?: number; - took: number; - values: SearchServiceValue[]; - overallHistogram: HistogramItem[]; - log: string[]; - ccsWarning: boolean; -} - interface TransactionDistributionFetcherState { error?: Error; isComplete: boolean; isRunning: boolean; loaded: number; - ccsWarning: RawResponse['ccsWarning']; - log: RawResponse['log']; - transactionDistribution?: RawResponse['overallHistogram']; - percentileThresholdValue?: RawResponse['percentileThresholdValue']; + ccsWarning: SearchServiceRawResponse['ccsWarning']; + log: SearchServiceRawResponse['log']; + transactionDistribution?: SearchServiceRawResponse['overallHistogram']; + percentileThresholdValue?: SearchServiceRawResponse['percentileThresholdValue']; timeTook?: number; total: number; } @@ -63,7 +53,9 @@ export function useTransactionDistributionFetcher() { const abortCtrl = useRef(new AbortController()); const searchSubscription$ = useRef(); - function setResponse(response: IKibanaSearchResponse) { + function setResponse( + response: IKibanaSearchResponse + ) { setFetchState((prevState) => ({ ...prevState, isRunning: response.isRunning || false, @@ -112,12 +104,15 @@ export function useTransactionDistributionFetcher() { // Submit the search request using the `data.search` service. searchSubscription$.current = data.search - .search>(req, { + .search< + IKibanaSearchRequest, + IKibanaSearchResponse + >(req, { strategy: 'apmCorrelationsSearchStrategy', abortSignal: abortCtrl.current.signal, }) .subscribe({ - next: (res: IKibanaSearchResponse) => { + next: (res: IKibanaSearchResponse) => { setResponse(res); if (isCompleteResponse(res)) { searchSubscription$.current?.unsubscribe(); diff --git a/x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.ts b/x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.ts index 3601f19ad7051..7f67147a75580 100644 --- a/x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.ts +++ b/x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.ts @@ -16,6 +16,7 @@ import { import type { SearchServiceParams, + SearchServiceRawResponse, SearchServiceValue, } from '../../../../common/search_strategies/correlations/types'; @@ -100,20 +101,22 @@ export const apmCorrelationsSearchStrategyProvider = ( const took = Date.now() - started; + const rawResponse: SearchServiceRawResponse = { + ccsWarning, + log, + took, + values, + percentileThresholdValue, + overallHistogram, + }; + return of({ id, loaded, total, isRunning, isPartial: isRunning, - rawResponse: { - ccsWarning, - log, - took, - values, - percentileThresholdValue, - overallHistogram, - }, + rawResponse, }); }, cancel: async (id, options, deps) => { From 6e21c1b14476bf7ecd1d98b0618a907dbae7ed58 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 24 Aug 2021 13:41:35 +0200 Subject: [PATCH 03/12] [ML] Fix empty state for latency correlations. --- .../latency_correlations.test.tsx | 131 ++++++++++++++++++ .../app/correlations/latency_correlations.tsx | 2 +- .../distribution/index.test.tsx | 3 +- .../use_transaction_distribution_fetcher.ts | 2 +- ...ransaction_latency_correlations_fetcher.ts | 39 +++--- 5 files changed, 154 insertions(+), 23 deletions(-) create mode 100644 x-pack/plugins/apm/public/components/app/correlations/latency_correlations.test.tsx diff --git a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.test.tsx b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.test.tsx new file mode 100644 index 0000000000000..b0da5b6d60d74 --- /dev/null +++ b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.test.tsx @@ -0,0 +1,131 @@ +/* + * 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 { render, screen, waitFor } from '@testing-library/react'; +import { createMemoryHistory } from 'history'; +import React, { ReactNode } from 'react'; +import { of } from 'rxjs'; + +import { __IntlProvider as IntlProvider } from '@kbn/i18n/react'; + +import { CoreStart } from 'kibana/public'; +import { merge } from 'lodash'; +import { dataPluginMock } from 'src/plugins/data/public/mocks'; +import type { IKibanaSearchResponse } from 'src/plugins/data/public'; +import { EuiThemeProvider } from 'src/plugins/kibana_react/common'; +import { createKibanaReactContext } from 'src/plugins/kibana_react/public'; +import type { SearchServiceRawResponse } from '../../../../common/search_strategies/correlations/types'; +import { MockUrlParamsContextProvider } from '../../../context/url_params_context/mock_url_params_context_provider'; +import { ApmPluginContextValue } from '../../../context/apm_plugin/apm_plugin_context'; +import { + mockApmPluginContextValue, + MockApmPluginContextWrapper, +} from '../../../context/apm_plugin/mock_apm_plugin_context'; +import { fromQuery } from '../../shared/Links/url_helpers'; + +import { LatencyCorrelations } from './latency_correlations'; + +function Wrapper({ + children, + dataSearchResponse, +}: { + children?: ReactNode; + dataSearchResponse: IKibanaSearchResponse; +}) { + const mockDataSearch = jest.fn(() => of(dataSearchResponse)); + + const dataPluginMockStart = dataPluginMock.createStartContract(); + const KibanaReactContext = createKibanaReactContext({ + data: { + ...dataPluginMockStart, + search: { + ...dataPluginMockStart.search, + search: mockDataSearch, + }, + }, + usageCollection: { reportUiCounter: () => {} }, + } as Partial); + + const httpGet = jest.fn(); + + const history = createMemoryHistory(); + jest.spyOn(history, 'push'); + jest.spyOn(history, 'replace'); + + history.replace({ + pathname: '/services/the-service-name/transactions/view', + search: fromQuery({ transactionName: 'the-transaction-name' }), + }); + + const mockPluginContext = (merge({}, mockApmPluginContextValue, { + core: { http: { get: httpGet } }, + }) as unknown) as ApmPluginContextValue; + + return ( + + + + + + {children} + + + + + + ); +} + +describe('correlations', () => { + describe('LatencyCorrelations', () => { + it('shows loading indicator when the service is running and returned no results yet', async () => { + render( + + + + ); + + await waitFor(() => { + expect(screen.getByTestId('apmCorrelationsChart')).toBeInTheDocument(); + expect(screen.getByTestId('loading')).toBeInTheDocument(); + }); + }); + + it("doesn't show loading indicator when the service isn't running", async () => { + render( + + + + ); + + await waitFor(() => { + expect(screen.getByTestId('apmCorrelationsChart')).toBeInTheDocument(); + expect(screen.queryByTestId('loading')).toBeNull(); // it doesn't exist + }); + }); + }); +}); diff --git a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx index ed47d49948fba..5d9a23af01ddc 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx @@ -60,7 +60,7 @@ export function LatencyCorrelations({ onFilter }: { onFilter: () => void }) { const { query: { kuery, environment }, - } = useApmParams('/services/:serviceName'); + } = useApmParams('/services/:serviceName/transactions/view'); const { urlParams } = useUrlParams(); diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx index 393810b1248af..5a9977b373c33 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.test.tsx @@ -123,6 +123,7 @@ describe('transaction_details/distribution', () => { expect(onHasData).toHaveBeenLastCalledWith(false); }); }); + it("doesn't show loading indicator when the service isn't running", async () => { const onHasData = jest.fn(); render( @@ -143,8 +144,6 @@ describe('transaction_details/distribution', () => { await waitFor(() => { expect(screen.getByTestId('apmCorrelationsChart')).toBeInTheDocument(); expect(screen.queryByTestId('loading')).toBeNull(); // it doesn't exist - // TODO Get the chart rendered into a state to display the no data message - // expect(screen.getByText('No data to display')).toBeInTheDocument(); expect(onHasData).toHaveBeenLastCalledWith(false); }); }); diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts index 4389877fda90e..f6d38d2837fbf 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts @@ -76,7 +76,7 @@ export function useTransactionDistributionFetcher() { } : {}), // if loading is done but didn't return any data for the overall histogram, - // set it to an empty array so the consuming chart component known loading is done. + // set it to an empty array so the consuming chart component knows loading is done. ...(!response.isRunning && response.rawResponse?.overallHistogram === undefined ? { transactionDistribution: [] } diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts index 49f2a279f4931..f643623459e93 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts @@ -14,32 +14,22 @@ import { isErrorResponse, } from '../../../../../src/plugins/data/public'; import type { - HistogramItem, SearchServiceParams, - SearchServiceValue, + SearchServiceRawResponse, } from '../../common/search_strategies/correlations/types'; import { useKibana } from '../../../../../src/plugins/kibana_react/public'; import { ApmPluginStartDeps } from '../plugin'; -interface RawResponse { - percentileThresholdValue?: number; - took: number; - values: SearchServiceValue[]; - overallHistogram: HistogramItem[]; - log: string[]; - ccsWarning: boolean; -} - interface TransactionLatencyCorrelationsFetcherState { error?: Error; isComplete: boolean; isRunning: boolean; loaded: number; - ccsWarning: RawResponse['ccsWarning']; - histograms: RawResponse['values']; - log: RawResponse['log']; - overallHistogram?: RawResponse['overallHistogram']; - percentileThresholdValue?: RawResponse['percentileThresholdValue']; + ccsWarning: SearchServiceRawResponse['ccsWarning']; + histograms: SearchServiceRawResponse['values']; + log: SearchServiceRawResponse['log']; + overallHistogram?: SearchServiceRawResponse['overallHistogram']; + percentileThresholdValue?: SearchServiceRawResponse['percentileThresholdValue']; timeTook?: number; total: number; } @@ -65,7 +55,9 @@ export const useTransactionLatencyCorrelationsFetcher = () => { const abortCtrl = useRef(new AbortController()); const searchSubscription$ = useRef(); - function setResponse(response: IKibanaSearchResponse) { + function setResponse( + response: IKibanaSearchResponse + ) { setFetchState((prevState) => ({ ...prevState, isRunning: response.isRunning || false, @@ -85,6 +77,12 @@ export const useTransactionLatencyCorrelationsFetcher = () => { response.rawResponse?.percentileThresholdValue, } : {}), + // if loading is done but didn't return any data for the overall histogram, + // set it to an empty array so the consuming chart component knows loading is done. + ...(!response.isRunning && + response.rawResponse?.overallHistogram === undefined + ? { overallHistogram: [] } + : {}), })); } @@ -108,12 +106,15 @@ export const useTransactionLatencyCorrelationsFetcher = () => { // Submit the search request using the `data.search` service. searchSubscription$.current = data.search - .search>(req, { + .search< + IKibanaSearchRequest, + IKibanaSearchResponse + >(req, { strategy: 'apmCorrelationsSearchStrategy', abortSignal: abortCtrl.current.signal, }) .subscribe({ - next: (res: IKibanaSearchResponse) => { + next: (res: IKibanaSearchResponse) => { setResponse(res); if (isCompleteResponse(res)) { searchSubscription$.current?.unsubscribe(); From ca86f769d73920a811c375004e699c027a11c141 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Tue, 24 Aug 2021 17:21:36 +0200 Subject: [PATCH 04/12] [ML] Fix tooltip text. --- .../app/correlations/failed_transactions_correlations.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx index 307dfc556672f..1e75bd2273848 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx @@ -146,7 +146,7 @@ export function FailedTransactionsCorrelations({ 'xpack.apm.correlations.failedTransactions.correlationsTable.failurePercentageDescription', { defaultMessage: - 'Percentage of time the term appear in failed transactions.', + 'Percentage of time the term appears in failed transactions.', } )} > @@ -179,7 +179,7 @@ export function FailedTransactionsCorrelations({ 'xpack.apm.correlations.failedTransactions.correlationsTable.successPercentageDescription', { defaultMessage: - 'Percentage of time the term appear in successful transactions.', + 'Percentage of time the term appears in successful transactions.', } )} > From cc7f8f1f855c2f12793b008a9150fdeb26e110a9 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Wed, 25 Aug 2021 10:46:38 +0200 Subject: [PATCH 05/12] [Ml] Trigger reload for log log histogram and analysis. --- .../app/correlations/failed_transactions_correlations.tsx | 2 +- .../public/components/app/correlations/latency_correlations.tsx | 2 +- .../components/app/transaction_details/distribution/index.tsx | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx index 1e75bd2273848..a80d208dfaf03 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx @@ -95,7 +95,7 @@ export function FailedTransactionsCorrelations({ const startFetchHandler = useCallback(() => { startFetch(searchServicePrams); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [environment, serviceName, kuery, start, end]); + }, [environment, serviceName, transactionType, kuery, start, end]); // start fetching on load // we want this effect to execute exactly once after the component mounts diff --git a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx index fe6868474b138..b54f2662c89a3 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx @@ -96,7 +96,7 @@ export function LatencyCorrelations({ onFilter }: { onFilter: () => void }) { percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, }); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [environment, serviceName, kuery, start, end]); + }, [environment, serviceName, transactionType, kuery, start, end]); // start fetching on load // we want this effect to execute exactly once after the component mounts diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx index 63b64242ce529..af523e23fcbed 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx @@ -135,7 +135,7 @@ export function TransactionDistribution({ cancelFetch(); }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [environment, serviceName, kuery, start, end]); + }, [environment, serviceName, transactionType, kuery, start, end]); useEffect(() => { if (isErrorMessage(error)) { From 137b14ee6130dc8600eac8a4bd3a4cdf4331e03f Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Wed, 25 Aug 2021 11:50:52 +0200 Subject: [PATCH 06/12] [ML] Fix paragraph spacing. --- .../public/components/app/correlations/empty_state_prompt.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/empty_state_prompt.tsx b/x-pack/plugins/apm/public/components/app/correlations/empty_state_prompt.tsx index 57e57a526baff..9b161fc1b9fa9 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/empty_state_prompt.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/empty_state_prompt.tsx @@ -33,8 +33,7 @@ export function CorrelationsEmptyStatePrompt() { id="xpack.apm.correlations.noCorrelationsTextLine1" defaultMessage="Correlations will only be identified if they have significant impact." /> -

-

+
Date: Wed, 25 Aug 2021 15:33:14 +0200 Subject: [PATCH 07/12] Revert "[ML] Fix tooltip text." This reverts commit ca86f769d73920a811c375004e699c027a11c141. --- .../app/correlations/failed_transactions_correlations.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx index 1e75bd2273848..307dfc556672f 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx @@ -146,7 +146,7 @@ export function FailedTransactionsCorrelations({ 'xpack.apm.correlations.failedTransactions.correlationsTable.failurePercentageDescription', { defaultMessage: - 'Percentage of time the term appears in failed transactions.', + 'Percentage of time the term appear in failed transactions.', } )} > @@ -179,7 +179,7 @@ export function FailedTransactionsCorrelations({ 'xpack.apm.correlations.failedTransactions.correlationsTable.successPercentageDescription', { defaultMessage: - 'Percentage of time the term appears in successful transactions.', + 'Percentage of time the term appear in successful transactions.', } )} > From 22e1b9c28059fbed60f04d58ea6ec278fe933f30 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 26 Aug 2021 08:27:09 +0200 Subject: [PATCH 08/12] [ML] Fix typo. --- .../app/correlations/failed_transactions_correlations.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx index 08412ec5d6a0a..69797837202b9 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx @@ -70,7 +70,7 @@ export function FailedTransactionsCorrelations({ const inspectEnabled = uiSettings.get(enableInspectEsQueries); - const searchServicePrams: SearchServiceParams = { + const searchServiceParams: SearchServiceParams = { environment, kuery, serviceName, @@ -93,7 +93,7 @@ export function FailedTransactionsCorrelations({ } = result; const startFetchHandler = useCallback(() => { - startFetch(searchServicePrams); + startFetch(searchServiceParams); // eslint-disable-next-line react-hooks/exhaustive-deps }, [environment, serviceName, transactionType, kuery, start, end]); From acc4caad1c21b1d909b2a74b8336c914d26fd52f Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 26 Aug 2021 08:31:21 +0200 Subject: [PATCH 09/12] [ML] Fix latency chart legend label. --- .../shared/charts/transaction_distribution_chart/index.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx index f3ba45d9eef1a..217a614e40862 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx @@ -302,7 +302,8 @@ export function TransactionDistributionChart({ fieldName !== undefined && fieldValue !== undefined && ( Date: Thu, 26 Aug 2021 09:05:03 +0200 Subject: [PATCH 10/12] [ML] Improve useEffects deps. --- .../failed_transactions_correlations.tsx | 40 ++++--- .../app/correlations/latency_correlations.tsx | 21 ++-- .../distribution/index.tsx | 27 ++++- ...ailed_transactions_correlations_fetcher.ts | 89 ++++++++------- .../use_transaction_distribution_fetcher.ts | 105 +++++++++--------- ...ransaction_latency_correlations_fetcher.ts | 105 +++++++++--------- 6 files changed, 209 insertions(+), 178 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx index 69797837202b9..999bbb555ea6a 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx @@ -41,7 +41,6 @@ import { CorrelationsLog } from './correlations_log'; import { CorrelationsEmptyStatePrompt } from './empty_state_prompt'; import { CrossClusterSearchCompatibilityWarning } from './cross_cluster_search_warning'; import { CorrelationsProgressControls } from './progress_controls'; -import type { SearchServiceParams } from '../../../../common/search_strategies/correlations/types'; import type { FailedTransactionsCorrelationValue } from '../../../../common/search_strategies/failure_correlations/types'; import { Summary } from '../../shared/Summary'; import { asPercent } from '../../../../common/utils/formatters'; @@ -70,16 +69,6 @@ export function FailedTransactionsCorrelations({ const inspectEnabled = uiSettings.get(enableInspectEsQueries); - const searchServiceParams: SearchServiceParams = { - environment, - kuery, - serviceName, - transactionName, - transactionType, - start, - end, - }; - const result = useFailedTransactionsCorrelationsFetcher(); const { @@ -93,12 +82,27 @@ export function FailedTransactionsCorrelations({ } = result; const startFetchHandler = useCallback(() => { - startFetch(searchServiceParams); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [environment, serviceName, transactionType, kuery, start, end]); + startFetch({ + environment, + kuery, + serviceName, + transactionName, + transactionType, + start, + end, + }); + }, [ + startFetch, + environment, + serviceName, + transactionName, + transactionType, + kuery, + start, + end, + ]); - // start fetching on load - // we want this effect to execute exactly once after the component mounts + // start fetching on load and on changing environment useEffect(() => { if (isRunning) { cancelFetch(); @@ -108,11 +112,11 @@ export function FailedTransactionsCorrelations({ return () => { // cancel any running async partial request when unmounting the component - // we want this effect to execute exactly once after the component mounts cancelFetch(); }; + // `isRunning` must not be part of the deps check // eslint-disable-next-line react-hooks/exhaustive-deps - }, [startFetchHandler]); + }, [cancelFetch, startFetchHandler]); const [ selectedSignificantTerm, diff --git a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx index b54f2662c89a3..672c63279acf0 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx @@ -95,11 +95,18 @@ export function LatencyCorrelations({ onFilter }: { onFilter: () => void }) { end, percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [environment, serviceName, transactionType, kuery, start, end]); - - // start fetching on load - // we want this effect to execute exactly once after the component mounts + }, [ + startFetch, + environment, + serviceName, + transactionName, + transactionType, + kuery, + start, + end, + ]); + + // start fetching on load and on changing environment useEffect(() => { if (isRunning) { cancelFetch(); @@ -109,11 +116,11 @@ export function LatencyCorrelations({ onFilter }: { onFilter: () => void }) { return () => { // cancel any running async partial request when unmounting the component - // we want this effect to execute exactly once after the component mounts cancelFetch(); }; + // `isRunning` must not be part of the deps check // eslint-disable-next-line react-hooks/exhaustive-deps - }, [startFetchHandler]); + }, [cancelFetch, startFetchHandler]); useEffect(() => { if (isErrorMessage(error)) { diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx index af523e23fcbed..30f074587a8de 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx @@ -114,11 +114,7 @@ export function TransactionDistribution({ transactionDistribution, } = useTransactionDistributionFetcher(); - useEffect(() => { - if (isRunning) { - cancelFetch(); - } - + const startFetchHandler = useCallback(() => { startFetch({ environment, kuery, @@ -129,13 +125,32 @@ export function TransactionDistribution({ end, percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, }); + }, [ + startFetch, + environment, + serviceName, + transactionName, + transactionType, + kuery, + start, + end, + ]); + + // start fetching on load and on changing environment + useEffect(() => { + if (isRunning) { + cancelFetch(); + } + + startFetchHandler(); return () => { // cancel any running async partial request when unmounting the component cancelFetch(); }; + // `isRunning` must not be part of the deps check // eslint-disable-next-line react-hooks/exhaustive-deps - }, [environment, serviceName, transactionType, kuery, start, end]); + }, [cancelFetch, startFetchHandler]); useEffect(() => { if (isErrorMessage(error)) { diff --git a/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts index add00968f0444..7bfba78671448 100644 --- a/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { useRef, useState } from 'react'; +import { useCallback, useRef, useState } from 'react'; import type { Subscription } from 'rxjs'; import { IKibanaSearchRequest, @@ -72,54 +72,57 @@ export const useFailedTransactionsCorrelationsFetcher = () => { })); } - const startFetch = (params: SearchServiceParams) => { - setFetchState((prevState) => ({ - ...prevState, - error: undefined, - isComplete: false, - })); - searchSubscription$.current?.unsubscribe(); - abortCtrl.current.abort(); - abortCtrl.current = new AbortController(); + const startFetch = useCallback( + (params: SearchServiceParams) => { + setFetchState((prevState) => ({ + ...prevState, + error: undefined, + isComplete: false, + })); + searchSubscription$.current?.unsubscribe(); + abortCtrl.current.abort(); + abortCtrl.current = new AbortController(); - const req = { params }; + const req = { params }; - // Submit the search request using the `data.search` service. - searchSubscription$.current = data.search - .search>(req, { - strategy: FAILED_TRANSACTIONS_CORRELATION_SEARCH_STRATEGY, - abortSignal: abortCtrl.current.signal, - }) - .subscribe({ - next: (res: IKibanaSearchResponse) => { - setResponse(res); - if (isCompleteResponse(res)) { - searchSubscription$.current?.unsubscribe(); + // Submit the search request using the `data.search` service. + searchSubscription$.current = data.search + .search>(req, { + strategy: FAILED_TRANSACTIONS_CORRELATION_SEARCH_STRATEGY, + abortSignal: abortCtrl.current.signal, + }) + .subscribe({ + next: (res: IKibanaSearchResponse) => { + setResponse(res); + if (isCompleteResponse(res)) { + searchSubscription$.current?.unsubscribe(); + setFetchState((prevState) => ({ + ...prevState, + isRunnning: false, + isComplete: true, + })); + } else if (isErrorResponse(res)) { + searchSubscription$.current?.unsubscribe(); + setFetchState((prevState) => ({ + ...prevState, + error: (res as unknown) as Error, + setIsRunning: false, + })); + } + }, + error: (error: Error) => { setFetchState((prevState) => ({ ...prevState, - isRunnning: false, - isComplete: true, - })); - } else if (isErrorResponse(res)) { - searchSubscription$.current?.unsubscribe(); - setFetchState((prevState) => ({ - ...prevState, - error: (res as unknown) as Error, + error, setIsRunning: false, })); - } - }, - error: (error: Error) => { - setFetchState((prevState) => ({ - ...prevState, - error, - setIsRunning: false, - })); - }, - }); - }; + }, + }); + }, + [data.search] + ); - const cancelFetch = () => { + const cancelFetch = useCallback(() => { searchSubscription$.current?.unsubscribe(); searchSubscription$.current = undefined; abortCtrl.current.abort(); @@ -127,7 +130,7 @@ export const useFailedTransactionsCorrelationsFetcher = () => { ...prevState, setIsRunning: false, })); - }; + }, []); return { ...fetchState, diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts index f6d38d2837fbf..6471f1f4da7f8 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { useRef, useState } from 'react'; +import { useCallback, useRef, useState } from 'react'; import type { Subscription } from 'rxjs'; import { IKibanaSearchRequest, @@ -84,63 +84,64 @@ export function useTransactionDistributionFetcher() { })); } - const startFetch = ( - params: Omit - ) => { - setFetchState((prevState) => ({ - ...prevState, - error: undefined, - isComplete: false, - })); - searchSubscription$.current?.unsubscribe(); - abortCtrl.current.abort(); - abortCtrl.current = new AbortController(); + const startFetch = useCallback( + (params: Omit) => { + setFetchState((prevState) => ({ + ...prevState, + error: undefined, + isComplete: false, + })); + searchSubscription$.current?.unsubscribe(); + abortCtrl.current.abort(); + abortCtrl.current = new AbortController(); - const searchServiceParams: SearchServiceParams = { - ...params, - analyzeCorrelations: false, - }; - const req = { params: searchServiceParams }; + const searchServiceParams: SearchServiceParams = { + ...params, + analyzeCorrelations: false, + }; + const req = { params: searchServiceParams }; - // Submit the search request using the `data.search` service. - searchSubscription$.current = data.search - .search< - IKibanaSearchRequest, - IKibanaSearchResponse - >(req, { - strategy: 'apmCorrelationsSearchStrategy', - abortSignal: abortCtrl.current.signal, - }) - .subscribe({ - next: (res: IKibanaSearchResponse) => { - setResponse(res); - if (isCompleteResponse(res)) { - searchSubscription$.current?.unsubscribe(); + // Submit the search request using the `data.search` service. + searchSubscription$.current = data.search + .search< + IKibanaSearchRequest, + IKibanaSearchResponse + >(req, { + strategy: 'apmCorrelationsSearchStrategy', + abortSignal: abortCtrl.current.signal, + }) + .subscribe({ + next: (res: IKibanaSearchResponse) => { + setResponse(res); + if (isCompleteResponse(res)) { + searchSubscription$.current?.unsubscribe(); + setFetchState((prevState) => ({ + ...prevState, + isRunnning: false, + isComplete: true, + })); + } else if (isErrorResponse(res)) { + searchSubscription$.current?.unsubscribe(); + setFetchState((prevState) => ({ + ...prevState, + error: (res as unknown) as Error, + setIsRunning: false, + })); + } + }, + error: (error: Error) => { setFetchState((prevState) => ({ ...prevState, - isRunnning: false, - isComplete: true, - })); - } else if (isErrorResponse(res)) { - searchSubscription$.current?.unsubscribe(); - setFetchState((prevState) => ({ - ...prevState, - error: (res as unknown) as Error, + error, setIsRunning: false, })); - } - }, - error: (error: Error) => { - setFetchState((prevState) => ({ - ...prevState, - error, - setIsRunning: false, - })); - }, - }); - }; + }, + }); + }, + [data.search] + ); - const cancelFetch = () => { + const cancelFetch = useCallback(() => { searchSubscription$.current?.unsubscribe(); searchSubscription$.current = undefined; abortCtrl.current.abort(); @@ -148,7 +149,7 @@ export function useTransactionDistributionFetcher() { ...prevState, setIsRunning: false, })); - }; + }, []); return { ...fetchState, diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts index f643623459e93..38d2c7c887d13 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { useRef, useState } from 'react'; +import { useCallback, useRef, useState } from 'react'; import type { Subscription } from 'rxjs'; import { IKibanaSearchRequest, @@ -86,63 +86,64 @@ export const useTransactionLatencyCorrelationsFetcher = () => { })); } - const startFetch = ( - params: Omit - ) => { - setFetchState((prevState) => ({ - ...prevState, - error: undefined, - isComplete: false, - })); - searchSubscription$.current?.unsubscribe(); - abortCtrl.current.abort(); - abortCtrl.current = new AbortController(); + const startFetch = useCallback( + (params: Omit) => { + setFetchState((prevState) => ({ + ...prevState, + error: undefined, + isComplete: false, + })); + searchSubscription$.current?.unsubscribe(); + abortCtrl.current.abort(); + abortCtrl.current = new AbortController(); - const searchServiceParams: SearchServiceParams = { - ...params, - analyzeCorrelations: true, - }; - const req = { params: searchServiceParams }; + const searchServiceParams: SearchServiceParams = { + ...params, + analyzeCorrelations: true, + }; + const req = { params: searchServiceParams }; - // Submit the search request using the `data.search` service. - searchSubscription$.current = data.search - .search< - IKibanaSearchRequest, - IKibanaSearchResponse - >(req, { - strategy: 'apmCorrelationsSearchStrategy', - abortSignal: abortCtrl.current.signal, - }) - .subscribe({ - next: (res: IKibanaSearchResponse) => { - setResponse(res); - if (isCompleteResponse(res)) { - searchSubscription$.current?.unsubscribe(); + // Submit the search request using the `data.search` service. + searchSubscription$.current = data.search + .search< + IKibanaSearchRequest, + IKibanaSearchResponse + >(req, { + strategy: 'apmCorrelationsSearchStrategy', + abortSignal: abortCtrl.current.signal, + }) + .subscribe({ + next: (res: IKibanaSearchResponse) => { + setResponse(res); + if (isCompleteResponse(res)) { + searchSubscription$.current?.unsubscribe(); + setFetchState((prevState) => ({ + ...prevState, + isRunnning: false, + isComplete: true, + })); + } else if (isErrorResponse(res)) { + searchSubscription$.current?.unsubscribe(); + setFetchState((prevState) => ({ + ...prevState, + error: (res as unknown) as Error, + setIsRunning: false, + })); + } + }, + error: (error: Error) => { setFetchState((prevState) => ({ ...prevState, - isRunnning: false, - isComplete: true, - })); - } else if (isErrorResponse(res)) { - searchSubscription$.current?.unsubscribe(); - setFetchState((prevState) => ({ - ...prevState, - error: (res as unknown) as Error, + error, setIsRunning: false, })); - } - }, - error: (error: Error) => { - setFetchState((prevState) => ({ - ...prevState, - error, - setIsRunning: false, - })); - }, - }); - }; + }, + }); + }, + [data.search] + ); - const cancelFetch = () => { + const cancelFetch = useCallback(() => { searchSubscription$.current?.unsubscribe(); searchSubscription$.current = undefined; abortCtrl.current.abort(); @@ -150,7 +151,7 @@ export const useTransactionLatencyCorrelationsFetcher = () => { ...prevState, setIsRunning: false, })); - }; + }, []); return { ...fetchState, From cd871b2ca492801f1761d448900933f73470d8d8 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 26 Aug 2021 11:38:09 +0200 Subject: [PATCH 11/12] [ML] Fix useEffects. --- .../failed_transactions_correlations.tsx | 13 +------------ .../app/correlations/latency_correlations.tsx | 13 +------------ .../app/transaction_details/distribution/index.tsx | 14 +------------- .../transaction_distribution_chart/index.tsx | 2 +- ...use_failed_transactions_correlations_fetcher.ts | 10 +++++----- .../hooks/use_transaction_distribution_fetcher.ts | 10 +++++----- ...use_transaction_latency_correlations_fetcher.ts | 10 +++++----- 7 files changed, 19 insertions(+), 53 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx index 999bbb555ea6a..4fb7bf5d6fcfb 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx @@ -102,20 +102,9 @@ export function FailedTransactionsCorrelations({ end, ]); - // start fetching on load and on changing environment useEffect(() => { - if (isRunning) { - cancelFetch(); - } - startFetchHandler(); - - return () => { - // cancel any running async partial request when unmounting the component - cancelFetch(); - }; - // `isRunning` must not be part of the deps check - // eslint-disable-next-line react-hooks/exhaustive-deps + return cancelFetch; }, [cancelFetch, startFetchHandler]); const [ diff --git a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx index 672c63279acf0..ad8a56a3ac6f9 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx @@ -106,20 +106,9 @@ export function LatencyCorrelations({ onFilter }: { onFilter: () => void }) { end, ]); - // start fetching on load and on changing environment useEffect(() => { - if (isRunning) { - cancelFetch(); - } - startFetchHandler(); - - return () => { - // cancel any running async partial request when unmounting the component - cancelFetch(); - }; - // `isRunning` must not be part of the deps check - // eslint-disable-next-line react-hooks/exhaustive-deps + return cancelFetch; }, [cancelFetch, startFetchHandler]); useEffect(() => { diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx index 30f074587a8de..2da61bc0fc555 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx @@ -108,7 +108,6 @@ export function TransactionDistribution({ const { error, percentileThresholdValue, - isRunning, startFetch, cancelFetch, transactionDistribution, @@ -136,20 +135,9 @@ export function TransactionDistribution({ end, ]); - // start fetching on load and on changing environment useEffect(() => { - if (isRunning) { - cancelFetch(); - } - startFetchHandler(); - - return () => { - // cancel any running async partial request when unmounting the component - cancelFetch(); - }; - // `isRunning` must not be part of the deps check - // eslint-disable-next-line react-hooks/exhaustive-deps + return cancelFetch; }, [cancelFetch, startFetchHandler]); useEffect(() => { diff --git a/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx index 217a614e40862..a58a2887b1576 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx @@ -176,7 +176,7 @@ export function TransactionDistributionChart({ ); useEffect(() => { - if (typeof onHasData === 'function') { + if (onHasData) { onHasData(chartLoadingState.hasData); } }, [chartLoadingState, onHasData]); diff --git a/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts index 7bfba78671448..f12cee7ee0332 100644 --- a/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts @@ -106,7 +106,7 @@ export const useFailedTransactionsCorrelationsFetcher = () => { setFetchState((prevState) => ({ ...prevState, error: (res as unknown) as Error, - setIsRunning: false, + isRunning: false, })); } }, @@ -114,12 +114,12 @@ export const useFailedTransactionsCorrelationsFetcher = () => { setFetchState((prevState) => ({ ...prevState, error, - setIsRunning: false, + isRunning: false, })); }, }); }, - [data.search] + [data.search, setFetchState] ); const cancelFetch = useCallback(() => { @@ -128,9 +128,9 @@ export const useFailedTransactionsCorrelationsFetcher = () => { abortCtrl.current.abort(); setFetchState((prevState) => ({ ...prevState, - setIsRunning: false, + isRunning: false, })); - }, []); + }, [setFetchState]); return { ...fetchState, diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts index 6471f1f4da7f8..2ff1b83ef1782 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts @@ -125,7 +125,7 @@ export function useTransactionDistributionFetcher() { setFetchState((prevState) => ({ ...prevState, error: (res as unknown) as Error, - setIsRunning: false, + isRunning: false, })); } }, @@ -133,12 +133,12 @@ export function useTransactionDistributionFetcher() { setFetchState((prevState) => ({ ...prevState, error, - setIsRunning: false, + isRunning: false, })); }, }); }, - [data.search] + [data.search, setFetchState] ); const cancelFetch = useCallback(() => { @@ -147,9 +147,9 @@ export function useTransactionDistributionFetcher() { abortCtrl.current.abort(); setFetchState((prevState) => ({ ...prevState, - setIsRunning: false, + isRunning: false, })); - }, []); + }, [setFetchState]); return { ...fetchState, diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts index 38d2c7c887d13..d96ef9637ffef 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts @@ -127,7 +127,7 @@ export const useTransactionLatencyCorrelationsFetcher = () => { setFetchState((prevState) => ({ ...prevState, error: (res as unknown) as Error, - setIsRunning: false, + isRunning: false, })); } }, @@ -135,12 +135,12 @@ export const useTransactionLatencyCorrelationsFetcher = () => { setFetchState((prevState) => ({ ...prevState, error, - setIsRunning: false, + isRunning: false, })); }, }); }, - [data.search] + [data.search, setFetchState] ); const cancelFetch = useCallback(() => { @@ -149,9 +149,9 @@ export const useTransactionLatencyCorrelationsFetcher = () => { abortCtrl.current.abort(); setFetchState((prevState) => ({ ...prevState, - setIsRunning: false, + isRunning: false, })); - }, []); + }, [setFetchState]); return { ...fetchState, From f4c3c5f0e8cf8130f0c4b299475473c1b389473a Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 26 Aug 2021 13:31:49 +0200 Subject: [PATCH 12/12] [ML] Fix loading spinner of latency correlations chart when cancel was hit before retrieving any data. --- .../hooks/use_transaction_latency_correlations_fetcher.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts index d96ef9637ffef..0b035c6af2354 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts @@ -149,6 +149,11 @@ export const useTransactionLatencyCorrelationsFetcher = () => { abortCtrl.current.abort(); setFetchState((prevState) => ({ ...prevState, + // If we didn't receive data for the overall histogram yet + // set it to an empty array to indicate loading stopped. + ...(prevState.overallHistogram === undefined + ? { overallHistogram: [] } + : {}), isRunning: false, })); }, [setFetchState]);