From 4857bbfb05cd5a149459ca74adb57d5de70e520d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cau=C3=AA=20Marcondes?= <55978943+cauemarcondes@users.noreply.github.com> Date: Tue, 5 Jan 2021 22:23:41 +0100 Subject: [PATCH] [APM] `transactionType` should be required on service-specific endpoints (#86893) (#87316) * making transaction type required on some apis * addressing PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../transaction_error_rate_chart/index.tsx | 2 +- ...se_transaction_throughput_chart_fetcher.ts | 6 +++-- ..._timeseries_data_for_transaction_groups.ts | 5 ---- .../get_service_transaction_groups/index.ts | 1 + .../merge_transaction_group_data.ts | 21 +++------------ .../get_throughput_charts/index.ts | 9 +++---- .../plugins/apm/server/routes/transactions.ts | 24 +++++------------ .../basic/tests/feature_controls.ts | 7 ----- .../basic/tests/transactions/throughput.ts | 26 +++++++++++++++---- 9 files changed, 40 insertions(+), 61 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/transaction_error_rate_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/transaction_error_rate_chart/index.tsx index 3f10b572b42a3..90877a895b05b 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/transaction_error_rate_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/transaction_error_rate_chart/index.tsx @@ -36,7 +36,7 @@ export function TransactionErrorRateChart({ const { start, end, transactionName } = urlParams; const { data, status } = useFetcher(() => { - if (serviceName && start && end) { + if (transactionType && serviceName && start && end) { return callApmApi({ endpoint: 'GET /api/apm/services/{serviceName}/transactions/charts/error_rate', diff --git a/x-pack/plugins/apm/public/hooks/use_transaction_throughput_chart_fetcher.ts b/x-pack/plugins/apm/public/hooks/use_transaction_throughput_chart_fetcher.ts index c03bb8efc79b3..e030d514a5283 100644 --- a/x-pack/plugins/apm/public/hooks/use_transaction_throughput_chart_fetcher.ts +++ b/x-pack/plugins/apm/public/hooks/use_transaction_throughput_chart_fetcher.ts @@ -10,18 +10,20 @@ import { useFetcher } from './use_fetcher'; import { useUrlParams } from '../context/url_params_context/use_url_params'; import { getThrouputChartSelector } from '../selectors/throuput_chart_selectors'; import { useTheme } from './use_theme'; +import { useApmServiceContext } from '../context/apm_service/use_apm_service_context'; export function useTransactionThroughputChartsFetcher() { const { serviceName } = useParams<{ serviceName?: string }>(); + const { transactionType } = useApmServiceContext(); const theme = useTheme(); const { - urlParams: { transactionType, start, end, transactionName }, + urlParams: { start, end, transactionName }, uiFilters, } = useUrlParams(); const { data, error, status } = useFetcher( (callApmApi) => { - if (serviceName && start && end) { + if (transactionType && serviceName && start && end) { return callApmApi({ endpoint: 'GET /api/apm/services/{serviceName}/transactions/charts/throughput', diff --git a/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/get_timeseries_data_for_transaction_groups.ts b/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/get_timeseries_data_for_transaction_groups.ts index 15ed46a23cae8..937155bc31602 100644 --- a/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/get_timeseries_data_for_transaction_groups.ts +++ b/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/get_timeseries_data_for_transaction_groups.ts @@ -87,11 +87,6 @@ export async function getTimeseriesDataForTransactionGroups({ size, }, aggs: { - transaction_types: { - terms: { - field: TRANSACTION_TYPE, - }, - }, timeseries: { date_histogram: { field: '@timestamp', diff --git a/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts b/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts index 41df70c9d5429..2df4b4e4c31f5 100644 --- a/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts +++ b/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/index.ts @@ -80,6 +80,7 @@ export async function getServiceTransactionGroups({ start, end, latencyAggregationType, + transactionType, }), totalTransactionGroups, isAggregationAccurate, diff --git a/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/merge_transaction_group_data.ts b/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/merge_transaction_group_data.ts index 6d9fc2fd3d579..a8794e3c09a40 100644 --- a/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/merge_transaction_group_data.ts +++ b/x-pack/plugins/apm/server/lib/services/get_service_transaction_groups/merge_transaction_group_data.ts @@ -4,17 +4,10 @@ * you may not use this file except in compliance with the Elastic License. */ -import { LatencyAggregationType } from '../../../../common/latency_aggregation_types'; import { EVENT_OUTCOME } from '../../../../common/elasticsearch_fieldnames'; - -import { - TRANSACTION_PAGE_LOAD, - TRANSACTION_REQUEST, -} from '../../../../common/transaction_types'; +import { LatencyAggregationType } from '../../../../common/latency_aggregation_types'; import { getLatencyValue } from '../../helpers/latency_aggregation_type'; - import { TransactionGroupTimeseriesData } from './get_timeseries_data_for_transaction_groups'; - import { TransactionGroupWithoutTimeseriesData } from './get_transaction_groups_for_page'; export function mergeTransactionGroupData({ @@ -23,12 +16,14 @@ export function mergeTransactionGroupData({ transactionGroups, timeseriesData, latencyAggregationType, + transactionType, }: { start: number; end: number; transactionGroups: TransactionGroupWithoutTimeseriesData[]; timeseriesData: TransactionGroupTimeseriesData; latencyAggregationType: LatencyAggregationType; + transactionType: string; }) { const deltaAsMinutes = (end - start) / 1000 / 60; @@ -37,16 +32,6 @@ export function mergeTransactionGroupData({ ({ key }) => key === transactionGroup.name ); - const transactionTypes = - groupBucket?.transaction_types.buckets.map( - (bucket) => bucket.key as string - ) ?? []; - - const transactionType = - transactionTypes.find( - (type) => type === TRANSACTION_PAGE_LOAD || type === TRANSACTION_REQUEST - ) ?? transactionTypes[0]; - const timeseriesBuckets = groupBucket?.timeseries.buckets ?? []; return timeseriesBuckets.reduce( diff --git a/x-pack/plugins/apm/server/lib/transactions/get_throughput_charts/index.ts b/x-pack/plugins/apm/server/lib/transactions/get_throughput_charts/index.ts index e7007f8db0197..be374ccfe3400 100644 --- a/x-pack/plugins/apm/server/lib/transactions/get_throughput_charts/index.ts +++ b/x-pack/plugins/apm/server/lib/transactions/get_throughput_charts/index.ts @@ -34,7 +34,7 @@ async function searchThroughput({ intervalString, }: { serviceName: string; - transactionType: string | undefined; + transactionType: string; transactionName: string | undefined; setup: Setup & SetupTimeRange; searchAggregatedTransactions: boolean; @@ -48,6 +48,7 @@ async function searchThroughput({ ...getDocumentTypeFilterForAggregatedTransactions( searchAggregatedTransactions ), + { term: { [TRANSACTION_TYPE]: transactionType } }, ...setup.esFilter, ]; @@ -55,10 +56,6 @@ async function searchThroughput({ filter.push({ term: { [TRANSACTION_NAME]: transactionName } }); } - if (transactionType) { - filter.push({ term: { [TRANSACTION_TYPE]: transactionType } }); - } - const field = getTransactionDurationFieldForAggregatedTransactions( searchAggregatedTransactions ); @@ -104,7 +101,7 @@ export async function getThroughputCharts({ searchAggregatedTransactions, }: { serviceName: string; - transactionType: string | undefined; + transactionType: string; transactionName: string | undefined; setup: Setup & SetupTimeRange; searchAggregatedTransactions: boolean; diff --git a/x-pack/plugins/apm/server/routes/transactions.ts b/x-pack/plugins/apm/server/routes/transactions.ts index 12a2ea113b1a1..50510fba78512 100644 --- a/x-pack/plugins/apm/server/routes/transactions.ts +++ b/x-pack/plugins/apm/server/routes/transactions.ts @@ -35,9 +35,7 @@ export const transactionGroupsRoute = createRoute({ serviceName: t.string, }), query: t.intersection([ - t.type({ - transactionType: t.string, - }), + t.type({ transactionType: t.string }), uiFiltersRt, rangeRt, ]), @@ -199,10 +197,8 @@ export const transactionThroughputChatsRoute = createRoute({ serviceName: t.string, }), query: t.intersection([ - t.partial({ - transactionType: t.string, - transactionName: t.string, - }), + t.type({ transactionType: t.string }), + t.partial({ transactionName: t.string }), uiFiltersRt, rangeRt, ]), @@ -287,12 +283,8 @@ export const transactionChartsBreakdownRoute = createRoute({ serviceName: t.string, }), query: t.intersection([ - t.type({ - transactionType: t.string, - }), - t.partial({ - transactionName: t.string, - }), + t.type({ transactionType: t.string }), + t.partial({ transactionName: t.string }), uiFiltersRt, rangeRt, ]), @@ -322,10 +314,8 @@ export const transactionChartsErrorRateRoute = createRoute({ query: t.intersection([ uiFiltersRt, rangeRt, - t.partial({ - transactionType: t.string, - transactionName: t.string, - }), + t.type({ transactionType: t.string }), + t.partial({ transactionName: t.string }), ]), }), options: { tags: ['access:apm'] }, diff --git a/x-pack/test/apm_api_integration/basic/tests/feature_controls.ts b/x-pack/test/apm_api_integration/basic/tests/feature_controls.ts index 94d8f65bb6dc7..8e06dd3bda732 100644 --- a/x-pack/test/apm_api_integration/basic/tests/feature_controls.ts +++ b/x-pack/test/apm_api_integration/basic/tests/feature_controls.ts @@ -126,13 +126,6 @@ export default function featureControlsTests({ getService }: FtrProviderContext) expectForbidden: expect403, expectResponse: expect200, }, - { - req: { - url: `/api/apm/services/foo/transactions/charts/throughput?start=${start}&end=${end}&uiFilters=%7B%22environment%22%3A%22testing%22%7D`, - }, - expectForbidden: expect403, - expectResponse: expect200, - }, { req: { url: `/api/apm/services/foo/transactions/charts/throughput?start=${start}&end=${end}&transactionType=bar&transactionName=baz&uiFilters=%7B%22environment%22%3A%22testing%22%7D`, diff --git a/x-pack/test/apm_api_integration/basic/tests/transactions/throughput.ts b/x-pack/test/apm_api_integration/basic/tests/transactions/throughput.ts index beec346eb8d51..5a7daf8d42908 100644 --- a/x-pack/test/apm_api_integration/basic/tests/transactions/throughput.ts +++ b/x-pack/test/apm_api_integration/basic/tests/transactions/throughput.ts @@ -4,6 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ import expect from '@kbn/expect'; +import url from 'url'; import archives_metadata from '../../../common/archives_metadata'; import { PromiseReturnType } from '../../../../../plugins/observability/typings/common'; import { FtrProviderContext } from '../../../../common/ftr_provider_context'; @@ -16,15 +17,22 @@ export default function ApiTest({ getService }: FtrProviderContext) { const metadata = archives_metadata[archiveName]; // url parameters - const start = encodeURIComponent(metadata.start); - const end = encodeURIComponent(metadata.end); - const uiFilters = encodeURIComponent(JSON.stringify({ environment: 'testing' })); + const { start, end } = metadata; + const uiFilters = JSON.stringify({ environment: 'testing' }); describe('Throughput', () => { describe('when data is not loaded ', () => { it('handles the empty state', async () => { const response = await supertest.get( - `/api/apm/services/opbeans-node/transactions/charts/throughput?start=${start}&end=${end}&uiFilters=${uiFilters}` + url.format({ + pathname: `/api/apm/services/opbeans-node/transactions/charts/throughput`, + query: { + start, + end, + uiFilters, + transactionType: 'request', + }, + }) ); expect(response.status).to.be(200); @@ -41,7 +49,15 @@ export default function ApiTest({ getService }: FtrProviderContext) { before(async () => { response = await supertest.get( - `/api/apm/services/opbeans-node/transactions/charts/throughput?start=${start}&end=${end}&uiFilters=${uiFilters}` + url.format({ + pathname: `/api/apm/services/opbeans-node/transactions/charts/throughput`, + query: { + start, + end, + uiFilters, + transactionType: 'request', + }, + }) ); });