Skip to content

[APM] Get rid of pre middleware #26256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sorenlouv merged 1 commit into from
Nov 28, 2018
Merged

[APM] Get rid of pre middleware #26256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions x-pack/plugins/apm/server/lib/helpers/setup_request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<T, U>(
type: string,
params: SearchParams
): AggregationSearchResponse<T, U> {
const client: ESClient = (type, params) => {
if (query._debug) {
console.log(`DEBUG ES QUERY:`);
console.log(
Expand All @@ -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(),
Expand Down
11 changes: 1 addition & 10 deletions x-pack/plugins/apm/server/lib/transactions/spans/get_spans.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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' } }]
Copy link
Copy Markdown
Contributor Author

@sorenlouv sorenlouv Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change: I stumbled upon this aggregation which is never used. I could make a new PR but it felt small enough to go into this one.

}
};

Expand Down
25 changes: 15 additions & 10 deletions x-pack/plugins/apm/server/routes/__test__/routeFailures.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { Server } from 'hapi';
import { flatten } from 'lodash';
// @ts-ignore
import { initErrorsApi } from '../errors';
import { initServicesApi } from '../services';
Expand All @@ -13,31 +14,35 @@ 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) {
const mockServer = { route: jest.fn() };
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({
Copy link
Copy Markdown
Contributor Author

@sorenlouv sorenlouv Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonrhodes I made a minor change to the test output:

screenshot 2018-11-27 at 13 37 33

ok?

Copy link
Copy Markdown
Member

@jasonrhodes jasonrhodes Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

message: 'request failed',
isBoom: true
});
Expand Down
10 changes: 3 additions & 7 deletions x-pack/plugins/apm/server/routes/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -25,7 +24,6 @@ export function initErrorsApi(server) {
method: 'GET',
path: ROOT,
config: {
pre,
validate: {
query: withDefaultValidators({
sortField: Joi.string(),
Expand All @@ -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;

Expand All @@ -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
Expand All @@ -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(
Expand Down
24 changes: 8 additions & 16 deletions x-pack/plugins/apm/server/routes/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -29,42 +28,35 @@ 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;
}
});

server.route({
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);
}
Expand Down
7 changes: 2 additions & 5 deletions x-pack/plugins/apm/server/routes/status_check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand All @@ -22,15 +21,14 @@ export function initStatusApi(server) {
method: 'GET',
path: `${ROOT}/server`,
config: {
pre,
validate: {
query: Joi.object().keys({
_debug: Joi.bool()
})
}
},
handler: req => {
const { setup } = req.pre;
const setup = setupRequest(req);
return getServerStatus({ setup }).catch(defaultErrorHandler);
}
});
Expand All @@ -39,15 +37,14 @@ export function initStatusApi(server) {
method: 'GET',
path: `${ROOT}/agent`,
config: {
pre,
validate: {
query: Joi.object().keys({
_debug: Joi.bool()
})
}
},
handler: req => {
const { setup } = req.pre;
const setup = setupRequest(req);
return getAgentStatus({ setup }).catch(defaultErrorHandler);
}
});
Expand Down
7 changes: 2 additions & 5 deletions x-pack/plugins/apm/server/routes/traces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
Expand All @@ -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);
}
});
Expand Down
16 changes: 5 additions & 11 deletions x-pack/plugins/apm/server/routes/transactions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -28,7 +27,6 @@ export function initTransactionsApi(server: Server) {
method: 'GET',
path: ROOT,
options: {
pre,
validate: {
query: withDefaultValidators({
transaction_type: Joi.string().default('request'),
Expand All @@ -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,
Expand All @@ -55,7 +53,6 @@ export function initTransactionsApi(server: Server) {
method: 'GET',
path: `${ROOT}/{transactionId}`,
options: {
pre,
validate: {
query: withDefaultValidators({
traceId: Joi.string().allow('')
Expand All @@ -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
);
Expand All @@ -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);
}
});
Expand All @@ -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'),
Expand All @@ -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;
Expand All @@ -124,15 +119,14 @@ export function initTransactionsApi(server: Server) {
method: 'GET',
path: `${ROOT}/distribution`,
options: {
pre,
validate: {
query: withDefaultValidators({
transaction_name: Joi.string().required()
})
}
},
handler: req => {
const { setup } = req.pre;
const setup = setupRequest(req);
const { serviceName } = req.params;
const { transaction_name: transactionName } = req.query as {
transaction_name: string;
Expand Down