From ef9096b088d0014d9fc21d4b751c626761d52e32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Tue, 27 Nov 2018 11:15:26 +0100 Subject: [PATCH] [APM] Get rid of `pre` middleware Rename apmIndexPattern to apmIndexPatternTitle and narrow down search query Fix tests Remove unused aggregation Revert "Rename apmIndexPattern to apmIndexPatternTitle and narrow down search query" This reverts commit 5aa86744a0b360ceb75a59ebc8a0a084b24fbe50. --- .../apm/server/lib/helpers/setup_request.ts | 9 +++---- .../lib/transactions/spans/get_spans.ts | 11 +------- .../routes/__test__/routeFailures.test.ts | 25 +++++++++++-------- x-pack/plugins/apm/server/routes/errors.js | 10 +++----- x-pack/plugins/apm/server/routes/services.ts | 24 ++++++------------ .../plugins/apm/server/routes/status_check.js | 7 ++---- x-pack/plugins/apm/server/routes/traces.ts | 7 ++---- .../plugins/apm/server/routes/transactions.ts | 16 ++++-------- 8 files changed, 39 insertions(+), 70 deletions(-) diff --git a/x-pack/plugins/apm/server/lib/helpers/setup_request.ts b/x-pack/plugins/apm/server/lib/helpers/setup_request.ts index 7d46d07865ba9..9bf11b98a2126 100644 --- a/x-pack/plugins/apm/server/lib/helpers/setup_request.ts +++ b/x-pack/plugins/apm/server/lib/helpers/setup_request.ts @@ -48,14 +48,11 @@ interface APMRequestQuery { esFilterQuery: string; } -export function setupRequest(req: Request) { +export function setupRequest(req: Request): Setup { const query = (req.query as unknown) as APMRequestQuery; const cluster = req.server.plugins.elasticsearch.getCluster('data'); - function client( - type: string, - params: SearchParams - ): AggregationSearchResponse { + const client: ESClient = (type, params) => { if (query._debug) { console.log(`DEBUG ES QUERY:`); console.log( @@ -67,7 +64,7 @@ export function setupRequest(req: Request) { console.log(JSON.stringify(params.body, null, 4)); } return cluster.callWithRequest(req, type, params); - } + }; return { start: moment.utc(query.start).valueOf(), diff --git a/x-pack/plugins/apm/server/lib/transactions/spans/get_spans.ts b/x-pack/plugins/apm/server/lib/transactions/spans/get_spans.ts index e3c794c01a033..a6fb6f7aac711 100644 --- a/x-pack/plugins/apm/server/lib/transactions/spans/get_spans.ts +++ b/x-pack/plugins/apm/server/lib/transactions/spans/get_spans.ts @@ -8,7 +8,6 @@ import { Span } from 'x-pack/plugins/apm/typings/Span'; import { PROCESSOR_EVENT, SPAN_START, - SPAN_TYPE, TRANSACTION_ID } from '../../../../common/constants'; import { Setup } from '../../helpers/setup_request'; @@ -42,15 +41,7 @@ export async function getSpans( ] } }, - sort: [{ [SPAN_START]: { order: 'asc' } }], - aggs: { - types: { - terms: { - field: SPAN_TYPE, - size: 100 - } - } - } + sort: [{ [SPAN_START]: { order: 'asc' } }] } }; diff --git a/x-pack/plugins/apm/server/routes/__test__/routeFailures.test.ts b/x-pack/plugins/apm/server/routes/__test__/routeFailures.test.ts index 44a82dd25f25c..7c3087ec4fcfc 100644 --- a/x-pack/plugins/apm/server/routes/__test__/routeFailures.test.ts +++ b/x-pack/plugins/apm/server/routes/__test__/routeFailures.test.ts @@ -5,6 +5,7 @@ */ import { Server } from 'hapi'; +import { flatten } from 'lodash'; // @ts-ignore import { initErrorsApi } from '../errors'; import { initServicesApi } from '../services'; @@ -13,7 +14,7 @@ import { initStatusApi } from '../status_check'; import { initTracesApi } from '../traces'; import { initTransactionsApi } from '../transactions'; -describe('route handlers fail properly', () => { +describe('route handlers should fail with a Boom error', () => { let consoleErrorSpy: any; async function testRouteFailures(init: (server: Server) => void) { @@ -21,23 +22,27 @@ describe('route handlers fail properly', () => { init((mockServer as unknown) as Server); expect(mockServer.route).toHaveBeenCalled(); - const routes = mockServer.route.mock.calls; + const mockCluster = { + callWithRequest: () => Promise.reject(new Error('request failed')) + }; + const mockConfig = { get: jest.fn() }; const mockReq = { params: {}, query: {}, - pre: { - setup: { - config: { get: jest.fn() }, - client: jest.fn(() => Promise.reject(new Error('request failed'))) + server: { + config: () => mockConfig, + plugins: { + elasticsearch: { + getCluster: () => mockCluster + } } } }; + const routes = flatten(mockServer.route.mock.calls); routes.forEach(async (route, i) => { - test(`route ${i + 1} of ${ - routes.length - } should fail with a Boom error`, async () => { - await expect(route[0].handler(mockReq)).rejects.toMatchObject({ + test(`${route.method} ${route.path}"`, async () => { + await expect(route.handler(mockReq)).rejects.toMatchObject({ message: 'request failed', isBoom: true }); diff --git a/x-pack/plugins/apm/server/routes/errors.js b/x-pack/plugins/apm/server/routes/errors.js index 775664abc90b6..c697b5965f274 100644 --- a/x-pack/plugins/apm/server/routes/errors.js +++ b/x-pack/plugins/apm/server/routes/errors.js @@ -13,7 +13,6 @@ import { getErrorGroup } from '../lib/errors/get_error_group'; import { setupRequest } from '../lib/helpers/setup_request'; import { withDefaultValidators } from '../lib/helpers/input_validation'; -const pre = [{ method: setupRequest, assign: 'setup' }]; const ROOT = '/api/apm/services/{serviceName}/errors'; const defaultErrorHandler = err => { console.error(err.stack); @@ -25,7 +24,6 @@ export function initErrorsApi(server) { method: 'GET', path: ROOT, config: { - pre, validate: { query: withDefaultValidators({ sortField: Joi.string(), @@ -34,7 +32,7 @@ export function initErrorsApi(server) { } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); const { serviceName } = req.params; const { sortField, sortDirection } = req.query; @@ -51,13 +49,12 @@ export function initErrorsApi(server) { method: 'GET', path: `${ROOT}/{groupId}`, config: { - pre, validate: { query: withDefaultValidators() } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); const { serviceName, groupId } = req.params; return getErrorGroup({ serviceName, groupId, setup }).catch( defaultErrorHandler @@ -69,13 +66,12 @@ export function initErrorsApi(server) { method: 'GET', path: `${ROOT}/{groupId}/distribution`, config: { - pre, validate: { query: withDefaultValidators() } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); const { serviceName, groupId } = req.params; return getDistribution({ serviceName, groupId, setup }).catch( diff --git a/x-pack/plugins/apm/server/routes/services.ts b/x-pack/plugins/apm/server/routes/services.ts index e5d225c739356..e1aaf18766e6f 100644 --- a/x-pack/plugins/apm/server/routes/services.ts +++ b/x-pack/plugins/apm/server/routes/services.ts @@ -17,7 +17,6 @@ import { getService } from '../lib/services/get_service'; import { getServices } from '../lib/services/get_services'; const ROOT = '/api/apm/services'; -const pre = [{ method: setupRequest, assign: 'setup' }]; const defaultErrorHandler = (err: Error) => { // tslint:disable-next-line console.error(err.stack); @@ -29,28 +28,22 @@ export function initServicesApi(server: Server) { method: 'GET', path: ROOT, options: { - pre, validate: { query: withDefaultValidators() } }, handler: async req => { - const { setup } = req.pre; + const setup = setupRequest(req); + const services = await getServices(setup).catch(defaultErrorHandler); - let serviceBucketList; - try { - serviceBucketList = await getServices(setup); - } catch (error) { - return defaultErrorHandler(error); - } - - // Store telemetry data derived from serviceBucketList - const apmTelemetry = createApmTelementry( - serviceBucketList.map(({ agentName }) => agentName as AgentName) + // Store telemetry data derived from services + const agentNames = services.map( + ({ agentName }) => agentName as AgentName ); + const apmTelemetry = createApmTelementry(agentNames); storeApmTelemetry(server, apmTelemetry); - return serviceBucketList; + return services; } }); @@ -58,13 +51,12 @@ export function initServicesApi(server: Server) { method: 'GET', path: `${ROOT}/{serviceName}`, options: { - pre, validate: { query: withDefaultValidators() } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); const { serviceName } = req.params; return getService(serviceName, setup).catch(defaultErrorHandler); } diff --git a/x-pack/plugins/apm/server/routes/status_check.js b/x-pack/plugins/apm/server/routes/status_check.js index 8e78fdc4b7848..5aeb79ad73b9b 100644 --- a/x-pack/plugins/apm/server/routes/status_check.js +++ b/x-pack/plugins/apm/server/routes/status_check.js @@ -11,7 +11,6 @@ import { getAgentStatus } from '../lib/status_check/agent_check'; import { setupRequest } from '../lib/helpers/setup_request'; const ROOT = '/api/apm/status'; -const pre = [{ method: setupRequest, assign: 'setup' }]; const defaultErrorHandler = err => { console.error(err.stack); throw Boom.boomify(err, { statusCode: 400 }); @@ -22,7 +21,6 @@ export function initStatusApi(server) { method: 'GET', path: `${ROOT}/server`, config: { - pre, validate: { query: Joi.object().keys({ _debug: Joi.bool() @@ -30,7 +28,7 @@ export function initStatusApi(server) { } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); return getServerStatus({ setup }).catch(defaultErrorHandler); } }); @@ -39,7 +37,6 @@ export function initStatusApi(server) { method: 'GET', path: `${ROOT}/agent`, config: { - pre, validate: { query: Joi.object().keys({ _debug: Joi.bool() @@ -47,7 +44,7 @@ export function initStatusApi(server) { } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); return getAgentStatus({ setup }).catch(defaultErrorHandler); } }); diff --git a/x-pack/plugins/apm/server/routes/traces.ts b/x-pack/plugins/apm/server/routes/traces.ts index f8a8a042286ca..7677d711f9ec6 100644 --- a/x-pack/plugins/apm/server/routes/traces.ts +++ b/x-pack/plugins/apm/server/routes/traces.ts @@ -11,7 +11,6 @@ import { setupRequest } from '../lib/helpers/setup_request'; import { getTopTraces } from '../lib/traces/get_top_traces'; import { getTrace } from '../lib/traces/get_trace'; -const pre = [{ method: setupRequest, assign: 'setup' }]; const ROOT = '/api/apm/traces'; const defaultErrorHandler = (err: Error) => { // tslint:disable-next-line @@ -25,13 +24,12 @@ export function initTracesApi(server: Server) { method: 'GET', path: ROOT, options: { - pre, validate: { query: withDefaultValidators() } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); return getTopTraces(setup).catch(defaultErrorHandler); } @@ -42,14 +40,13 @@ export function initTracesApi(server: Server) { method: 'GET', path: `${ROOT}/{traceId}`, options: { - pre, validate: { query: withDefaultValidators() } }, handler: req => { const { traceId } = req.params; - const { setup } = req.pre; + const setup = setupRequest(req); return getTrace(traceId, setup).catch(defaultErrorHandler); } }); diff --git a/x-pack/plugins/apm/server/routes/transactions.ts b/x-pack/plugins/apm/server/routes/transactions.ts index fa1104534c074..1d706b744df6b 100644 --- a/x-pack/plugins/apm/server/routes/transactions.ts +++ b/x-pack/plugins/apm/server/routes/transactions.ts @@ -15,7 +15,6 @@ import { getTopTransactions } from '../lib/transactions/get_top_transactions'; import { getTransaction } from '../lib/transactions/get_transaction'; import { getSpans } from '../lib/transactions/spans/get_spans'; -const pre = [{ method: setupRequest, assign: 'setup' }]; const ROOT = '/api/apm/services/{serviceName}/transactions'; const defaultErrorHandler = (err: Error) => { // tslint:disable-next-line @@ -28,7 +27,6 @@ export function initTransactionsApi(server: Server) { method: 'GET', path: ROOT, options: { - pre, validate: { query: withDefaultValidators({ transaction_type: Joi.string().default('request'), @@ -41,7 +39,7 @@ export function initTransactionsApi(server: Server) { const { transaction_type: transactionType } = req.query as { transaction_type: string; }; - const { setup } = req.pre; + const setup = setupRequest(req); return getTopTransactions({ serviceName, @@ -55,7 +53,6 @@ export function initTransactionsApi(server: Server) { method: 'GET', path: `${ROOT}/{transactionId}`, options: { - pre, validate: { query: withDefaultValidators({ traceId: Joi.string().allow('') @@ -65,7 +62,7 @@ export function initTransactionsApi(server: Server) { handler: req => { const { transactionId } = req.params; const { traceId } = req.query as { traceId: string }; - const { setup } = req.pre; + const setup = setupRequest(req); return getTransaction(transactionId, traceId, setup).catch( defaultErrorHandler ); @@ -76,14 +73,13 @@ export function initTransactionsApi(server: Server) { method: 'GET', path: `${ROOT}/{transactionId}/spans`, options: { - pre, validate: { query: withDefaultValidators() } }, handler: req => { const { transactionId } = req.params; - const { setup } = req.pre; + const setup = setupRequest(req); return getSpans(transactionId, setup).catch(defaultErrorHandler); } }); @@ -92,7 +88,6 @@ export function initTransactionsApi(server: Server) { method: 'GET', path: `${ROOT}/charts`, options: { - pre, validate: { query: withDefaultValidators({ transaction_type: Joi.string().default('request'), @@ -102,7 +97,7 @@ export function initTransactionsApi(server: Server) { } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); const { serviceName } = req.params; const { transaction_type: transactionType } = req.query as { transaction_type: string; @@ -124,7 +119,6 @@ export function initTransactionsApi(server: Server) { method: 'GET', path: `${ROOT}/distribution`, options: { - pre, validate: { query: withDefaultValidators({ transaction_name: Joi.string().required() @@ -132,7 +126,7 @@ export function initTransactionsApi(server: Server) { } }, handler: req => { - const { setup } = req.pre; + const setup = setupRequest(req); const { serviceName } = req.params; const { transaction_name: transactionName } = req.query as { transaction_name: string;