From 2ea37558573e07fd07184f4edbe65867955b9edb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Sat, 13 Jun 2020 17:23:57 +0200 Subject: [PATCH] Check number of returned buckets --- .../components/app/TraceOverview/index.tsx | 12 ++- .../app/TransactionOverview/index.tsx | 40 ++++--- .../apm/public/hooks/useTransactionList.ts | 28 ++++- .../public/services/rest/createCallApmApi.ts | 10 +- .../__snapshots__/fetcher.test.ts.snap | 4 +- .../__snapshots__/queries.test.ts.snap | 4 +- .../lib/transaction_groups/fetcher.test.ts | 7 +- .../server/lib/transaction_groups/fetcher.ts | 7 +- .../server/lib/transaction_groups/index.ts | 8 +- .../lib/transaction_groups/queries.test.ts | 8 +- .../lib/transaction_groups/transform.test.ts | 100 ++++++++++++++---- .../lib/transaction_groups/transform.ts | 21 +++- 12 files changed, 187 insertions(+), 62 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/TraceOverview/index.tsx b/x-pack/plugins/apm/public/components/app/TraceOverview/index.tsx index cb6003c58e90d1..9a3282d878c16e 100644 --- a/x-pack/plugins/apm/public/components/app/TraceOverview/index.tsx +++ b/x-pack/plugins/apm/public/components/app/TraceOverview/index.tsx @@ -12,11 +12,19 @@ import { useUrlParams } from '../../../hooks/useUrlParams'; import { useTrackPageview } from '../../../../../observability/public'; import { LocalUIFilters } from '../../shared/LocalUIFilters'; import { PROJECTION } from '../../../../common/projections/typings'; +import { APIReturnType } from '../../../services/rest/createCallApmApi'; + +type TracesAPIResponse = APIReturnType<'/api/apm/traces'>; +const DEFAULT_RESPONSE: TracesAPIResponse = { + transactionGroups: [], + isAggregationAccurate: true, + bucketSize: 0, +}; export function TraceOverview() { const { urlParams, uiFilters } = useUrlParams(); const { start, end } = urlParams; - const { status, data = [] } = useFetcher( + const { status, data = DEFAULT_RESPONSE } = useFetcher( (callApmApi) => { if (start && end) { return callApmApi({ @@ -56,7 +64,7 @@ export function TraceOverview() { diff --git a/x-pack/plugins/apm/public/components/app/TransactionOverview/index.tsx b/x-pack/plugins/apm/public/components/app/TransactionOverview/index.tsx index b5f91c7fdeb545..2ea3152578644a 100644 --- a/x-pack/plugins/apm/public/components/app/TransactionOverview/index.tsx +++ b/x-pack/plugins/apm/public/components/app/TransactionOverview/index.tsx @@ -18,6 +18,7 @@ import { import { Location } from 'history'; import { first } from 'lodash'; import React, { useMemo } from 'react'; +import { i18n } from '@kbn/i18n'; import { useTransactionList } from '../../../hooks/useTransactionList'; import { useTransactionCharts } from '../../../hooks/useTransactionCharts'; import { IUrlParams } from '../../../context/UrlParamsContext/types'; @@ -157,23 +158,34 @@ export function TransactionOverview() {

Transactions

- -

- The number of unique transaction names exceeds the configured - value of 200. Try reconfiguring your agents to group similar - transactions or increase the value of - xpack.apm.ui.transactionGroupBucketSize.{' '} - Learn more in the docs.. -

- + {!transactionListData.isAggregationAccurate && ( + +

+ The number of unique transaction names exceeds the configured + value of {transactionListData.bucketSize}. Try reconfiguring + your agents to group similar transactions or increase the + value of + + xpack.apm.ui.transactionGroupBucketSize + .{' '} + Learn more in the docs.. +

+ + )} diff --git a/x-pack/plugins/apm/public/hooks/useTransactionList.ts b/x-pack/plugins/apm/public/hooks/useTransactionList.ts index 202437ae722572..b61b6446b03bb4 100644 --- a/x-pack/plugins/apm/public/hooks/useTransactionList.ts +++ b/x-pack/plugins/apm/public/hooks/useTransactionList.ts @@ -8,8 +8,7 @@ import { useMemo } from 'react'; import { IUrlParams } from '../context/UrlParamsContext/types'; import { useUiFilters } from '../context/UrlParamsContext'; import { useFetcher } from './useFetcher'; -// eslint-disable-next-line @kbn/eslint/no-restricted-paths -import { TransactionGroupListAPIResponse } from '../../server/lib/transaction_groups'; +import { APIReturnType } from '../services/rest/createCallApmApi'; const getRelativeImpact = ( impact: number, @@ -21,7 +20,13 @@ const getRelativeImpact = ( 1 ); -function getWithRelativeImpact(items: TransactionGroupListAPIResponse) { +type TransactionsAPIResponse = APIReturnType< + '/api/apm/services/{serviceName}/transaction_groups' +>; + +function getWithRelativeImpact( + items: TransactionsAPIResponse['transactionGroups'] +) { const impacts = items .map(({ impact }) => impact) .filter((impact) => impact !== null) as number[]; @@ -40,10 +45,16 @@ function getWithRelativeImpact(items: TransactionGroupListAPIResponse) { }); } +const DEFAULT_RESPONSE: TransactionsAPIResponse = { + transactionGroups: [], + isAggregationAccurate: true, + bucketSize: 0, +}; + export function useTransactionList(urlParams: IUrlParams) { const { serviceName, transactionType, start, end } = urlParams; const uiFilters = useUiFilters(urlParams); - const { data = [], error, status } = useFetcher( + const { data = DEFAULT_RESPONSE, error, status } = useFetcher( (callApmApi) => { if (serviceName && start && end && transactionType) { return callApmApi({ @@ -63,7 +74,14 @@ export function useTransactionList(urlParams: IUrlParams) { [serviceName, start, end, transactionType, uiFilters] ); - const memoizedData = useMemo(() => getWithRelativeImpact(data), [data]); + const memoizedData = useMemo( + () => ({ + transactionGroups: getWithRelativeImpact(data.transactionGroups), + isAggregationAccurate: data.isAggregationAccurate, + bucketSize: data.bucketSize, + }), + [data] + ); return { data: memoizedData, status, diff --git a/x-pack/plugins/apm/public/services/rest/createCallApmApi.ts b/x-pack/plugins/apm/public/services/rest/createCallApmApi.ts index 44768c94f3b1df..8babc72ef129ce 100644 --- a/x-pack/plugins/apm/public/services/rest/createCallApmApi.ts +++ b/x-pack/plugins/apm/public/services/rest/createCallApmApi.ts @@ -8,7 +8,7 @@ import { callApi, FetchOptions } from './callApi'; // eslint-disable-next-line @kbn/eslint/no-restricted-paths import { APMAPI } from '../../../server/routes/create_apm_api'; // eslint-disable-next-line @kbn/eslint/no-restricted-paths -import { Client } from '../../../server/routes/typings'; +import { Client, HttpMethod } from '../../../server/routes/typings'; export type APMClient = Client; export type APMClientOptions = Omit & { @@ -43,3 +43,11 @@ export function createCallApmApi(http: HttpSetup) { }); }) as APMClient; } + +// infer return type from API +export type APIReturnType< + TPath extends keyof APMAPI['_S'], + TMethod extends HttpMethod = 'GET' +> = APMAPI['_S'][TPath] extends { [key in TMethod]: { ret: any } } + ? APMAPI['_S'][TPath][TMethod]['ret'] + : unknown; diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/fetcher.test.ts.snap b/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/fetcher.test.ts.snap index 64f06ad0a81cd0..087dc6afc9a587 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/fetcher.test.ts.snap +++ b/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/fetcher.test.ts.snap @@ -46,7 +46,7 @@ Array [ }, }, "composite": Object { - "size": 10000, + "size": 101, "sources": Array [ Object { "service": Object { @@ -159,7 +159,7 @@ Array [ }, }, "composite": Object { - "size": 10000, + "size": 101, "sources": Array [ Object { "transaction": Object { diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/queries.test.ts.snap b/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/queries.test.ts.snap index b93f842b878cb0..496533cf97e65d 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/queries.test.ts.snap +++ b/x-pack/plugins/apm/server/lib/transaction_groups/__snapshots__/queries.test.ts.snap @@ -44,7 +44,7 @@ Object { }, }, "composite": Object { - "size": 10000, + "size": 101, "sources": Array [ Object { "service": Object { @@ -153,7 +153,7 @@ Object { }, }, "composite": Object { - "size": 10000, + "size": 101, "sources": Array [ Object { "transaction": Object { diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.test.ts b/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.test.ts index 00702be6744ec8..a26c3d85a3fc47 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.test.ts +++ b/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.test.ts @@ -39,7 +39,8 @@ describe('transactionGroupsFetcher', () => { describe('type: top_traces', () => { it('should call client.search with correct query', async () => { const setup = getSetup(); - await transactionGroupsFetcher({ type: 'top_traces' }, setup); + const bucketSize = 100; + await transactionGroupsFetcher({ type: 'top_traces' }, setup, bucketSize); expect(setup.client.search.mock.calls).toMatchSnapshot(); }); }); @@ -47,13 +48,15 @@ describe('transactionGroupsFetcher', () => { describe('type: top_transactions', () => { it('should call client.search with correct query', async () => { const setup = getSetup(); + const bucketSize = 100; await transactionGroupsFetcher( { type: 'top_transactions', serviceName: 'opbeans-node', transactionType: 'request', }, - setup + setup, + bucketSize ); expect(setup.client.search.mock.calls).toMatchSnapshot(); }); diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts b/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts index d10c45ecbdbfb1..595ee9d8da2dcf 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts +++ b/x-pack/plugins/apm/server/lib/transaction_groups/fetcher.ts @@ -36,9 +36,10 @@ interface TopTraceOptions { export type Options = TopTransactionOptions | TopTraceOptions; export type ESResponse = PromiseReturnType; -export function transactionGroupsFetcher( +export async function transactionGroupsFetcher( options: Options, - setup: Setup & SetupTimeRange & SetupUIFilters + setup: Setup & SetupTimeRange & SetupUIFilters, + bucketSize: number ) { const { client } = setup; @@ -71,7 +72,7 @@ export function transactionGroupsFetcher( aggs: { transaction_groups: { composite: { - size: 10000, + size: bucketSize + 1, // 1 extra bucket is added to check whether the total number of buckets exceed the specified bucket size. sources: [ ...(isTopTraces ? [{ service: { terms: { field: SERVICE_NAME } } }] diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/index.ts b/x-pack/plugins/apm/server/lib/transaction_groups/index.ts index 30c4975120483e..893e586b351a80 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/index.ts +++ b/x-pack/plugins/apm/server/lib/transaction_groups/index.ts @@ -11,20 +11,18 @@ import { } from '../helpers/setup_request'; import { transactionGroupsFetcher, Options } from './fetcher'; import { transactionGroupsTransformer } from './transform'; -import { PromiseReturnType } from '../../../../observability/typings/common'; -export type TransactionGroupListAPIResponse = PromiseReturnType< - typeof getTransactionGroupList ->; export async function getTransactionGroupList( options: Options, setup: Setup & SetupTimeRange & SetupUIFilters ) { const { start, end } = setup; - const response = await transactionGroupsFetcher(options, setup); + const bucketSize = setup.config['xpack.apm.ui.transactionGroupBucketSize']; + const response = await transactionGroupsFetcher(options, setup, bucketSize); return transactionGroupsTransformer({ response, start, end, + bucketSize, }); } diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/queries.test.ts b/x-pack/plugins/apm/server/lib/transaction_groups/queries.test.ts index 58d770bebce979..2c5aa79bb3483c 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/queries.test.ts +++ b/x-pack/plugins/apm/server/lib/transaction_groups/queries.test.ts @@ -18,6 +18,7 @@ describe('transaction group queries', () => { }); it('fetches top transactions', async () => { + const bucketSize = 100; mock = await inspectSearchParams((setup) => transactionGroupsFetcher( { @@ -25,7 +26,8 @@ describe('transaction group queries', () => { serviceName: 'foo', transactionType: 'bar', }, - setup + setup, + bucketSize ) ); @@ -33,12 +35,14 @@ describe('transaction group queries', () => { }); it('fetches top traces', async () => { + const bucketSize = 100; mock = await inspectSearchParams((setup) => transactionGroupsFetcher( { type: 'top_traces', }, - setup + setup, + bucketSize ) ); diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/transform.test.ts b/x-pack/plugins/apm/server/lib/transaction_groups/transform.test.ts index e5ec9a8eae782b..35420d223d22df 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/transform.test.ts +++ b/x-pack/plugins/apm/server/lib/transaction_groups/transform.test.ts @@ -10,13 +10,20 @@ import { transactionGroupsTransformer } from './transform'; describe('transactionGroupsTransformer', () => { it('should match snapshot', () => { - expect( - transactionGroupsTransformer({ - response: transactionGroupsResponse, - start: 100, - end: 2000, - }) - ).toMatchSnapshot(); + const { + bucketSize, + isAggregationAccurate, + transactionGroups, + } = transactionGroupsTransformer({ + response: transactionGroupsResponse, + start: 100, + end: 2000, + bucketSize: 100, + }); + + expect(bucketSize).toBe(100); + expect(isAggregationAccurate).toBe(true); + expect(transactionGroups).toMatchSnapshot(); }); it('should transform response correctly', () => { @@ -43,17 +50,59 @@ describe('transactionGroupsTransformer', () => { } as unknown) as ESResponse; expect( - transactionGroupsTransformer({ response, start: 100, end: 20000 }) - ).toEqual([ - { - averageResponseTime: 255966.30555555556, - impact: 0, - name: 'POST /api/orders', - p95: 320238.5, - sample: 'sample source', - transactionsPerMinute: 542.713567839196, + transactionGroupsTransformer({ + response, + start: 100, + end: 20000, + bucketSize: 100, + }) + ).toEqual({ + bucketSize: 100, + isAggregationAccurate: true, + transactionGroups: [ + { + averageResponseTime: 255966.30555555556, + impact: 0, + name: 'POST /api/orders', + p95: 320238.5, + sample: 'sample source', + transactionsPerMinute: 542.713567839196, + }, + ], + }); + }); + + it('`isAggregationAccurate` should be false if number of bucket is higher than `bucketSize`', () => { + const bucket = { + key: { transaction: 'POST /api/orders' }, + doc_count: 180, + avg: { value: 255966.30555555556 }, + p95: { values: { '95.0': 320238.5 } }, + sum: { value: 3000000000 }, + sample: { + hits: { + total: 180, + hits: [{ _source: 'sample source' }], + }, }, - ]); + }; + + const response = ({ + aggregations: { + transaction_groups: { + buckets: [bucket, bucket, bucket, bucket], // four buckets returned + }, + }, + } as unknown) as ESResponse; + + const { isAggregationAccurate } = transactionGroupsTransformer({ + response, + start: 100, + end: 20000, + bucketSize: 3, // bucket size of three + }); + + expect(isAggregationAccurate).toEqual(false); }); it('should calculate impact from sum', () => { @@ -74,10 +123,17 @@ describe('transactionGroupsTransformer', () => { }, } as unknown) as ESResponse; - expect( - transactionGroupsTransformer({ response, start: 100, end: 20000 }).map( - (bucket) => bucket.impact - ) - ).toEqual([100, 25, 0]); + const { transactionGroups } = transactionGroupsTransformer({ + response, + start: 100, + end: 20000, + bucketSize: 100, + }); + + expect(transactionGroups.map((bucket) => bucket.impact)).toEqual([ + 100, + 25, + 0, + ]); }); }); diff --git a/x-pack/plugins/apm/server/lib/transaction_groups/transform.ts b/x-pack/plugins/apm/server/lib/transaction_groups/transform.ts index 2f34d365e5be9a..d405d9fc5576c6 100644 --- a/x-pack/plugins/apm/server/lib/transaction_groups/transform.ts +++ b/x-pack/plugins/apm/server/lib/transaction_groups/transform.ts @@ -60,11 +60,17 @@ export function transactionGroupsTransformer({ response, start, end, + bucketSize, }: { response: ESResponse; start: number; end: number; -}): ITransactionGroup[] { + bucketSize: number; +}): { + transactionGroups: ITransactionGroup[]; + isAggregationAccurate: boolean; + bucketSize: number; +} { const buckets = getBuckets(response); const duration = moment.duration(end - start); const minutes = duration.asMinutes(); @@ -72,5 +78,16 @@ export function transactionGroupsTransformer({ getTransactionGroup(bucket, minutes) ); - return calculateRelativeImpacts(transactionGroups); + const transactionGroupsWithRelativeImpact = calculateRelativeImpacts( + transactionGroups + ); + + return { + transactionGroups: transactionGroupsWithRelativeImpact, + + // The aggregation is considered accurate if the configured bucket size is larger or equal to the number of buckets returned + // the actual number of buckets retrieved are `bucketsize + 1` to detect whether it's above the limit + isAggregationAccurate: bucketSize >= buckets.length, + bucketSize, + }; }