Skip to content
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.

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

propagate trace.id to http response log manually #118466

Merged
merged 17 commits into from
Nov 24, 2021
4 changes: 4 additions & 0 deletions src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
getServerOptions,
getRequestId,
} from '@kbn/server-http-tools';
import agent from 'elastic-apm-node';

import type { Duration } from 'moment';
import { Observable } from 'rxjs';
Expand Down Expand Up @@ -345,6 +346,9 @@ export class HttpServer {
...(request.app ?? {}),
requestId,
requestUuid: uuid.v4(),
// Kibana stores trace.id until https://github.com/elastic/apm-agent-nodejs/issues/2353 is resolved
// The current implementation of the APM agent ends a request transaction before "response" log is emitted.
traceId: agent.currentTraceIds['trace.id'],
} as KibanaRequestState;
return responseToolkit.continue;
});
Expand Down
15 changes: 15 additions & 0 deletions src/core/server/http/logging/get_response_log.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ interface RequestFixtureOptions {
path?: string;
query?: Record<string, any>;
response?: Record<string, any> | Boom.Boom;
app?: Record<string, any>;
}

function createMockHapiRequest({
Expand All @@ -39,6 +40,7 @@ function createMockHapiRequest({
path = '/path',
query = {},
response = { headers: {}, statusCode: 200 },
app = {},
}: RequestFixtureOptions = {}): Request {
return {
auth,
Expand All @@ -50,6 +52,7 @@ function createMockHapiRequest({
path,
query,
response,
app,
} as unknown as Request;
}

Expand Down Expand Up @@ -143,6 +146,17 @@ describe('getEcsResponseLog', () => {
expect(result.message).toMatchInlineSnapshot(`"GET /path 200"`);
});

test('set traceId stored in the request app storage', () => {
const req = createMockHapiRequest({
app: {
foo: 'bar',
traceId: 'trace_id',
},
});
const result = getEcsResponseLog(req, logger);
expect(result.meta?.trace?.id).toBe('trace_id');
});

test('handles Boom errors in the response', () => {
const req = createMockHapiRequest({
response: Boom.badRequest(),
Expand Down Expand Up @@ -280,6 +294,7 @@ describe('getEcsResponseLog', () => {
"status_code": 200,
},
},
"trace": undefined,
"url": Object {
"path": "/path",
"query": "",
Expand Down
4 changes: 4 additions & 0 deletions src/core/server/http/logging/get_response_log.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import numeral from '@elastic/numeral';
import { LogMeta } from '@kbn/logging';
import { Logger } from '../../logging';
import { getResponsePayloadBytes } from './get_payload_size';
import type { KibanaRequestState } from '../router';

const FORBIDDEN_HEADERS = ['authorization', 'cookie', 'set-cookie'];
const REDACTED_HEADER_TEXT = '[REDACTED]';
Expand Down Expand Up @@ -65,6 +66,8 @@ export function getEcsResponseLog(request: Request, log: Logger) {
const bytes = getResponsePayloadBytes(response, log);
const bytesMsg = bytes ? ` - ${numeral(bytes).format('0.0b')}` : '';

const traceId = (request.app as KibanaRequestState).traceId;

const meta: LogMeta = {
client: {
ip: request.info.remoteAddress,
Expand Down Expand Up @@ -95,6 +98,7 @@ export function getEcsResponseLog(request: Request, log: Logger) {
user_agent: {
original: request.headers['user-agent'],
},
trace: traceId ? { id: traceId } : undefined,
lukeelmers marked this conversation as resolved.
Show resolved Hide resolved
};

return {
Expand Down
1 change: 1 addition & 0 deletions src/core/server/http/router/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export interface KibanaRequestState extends RequestApplicationState {
requestId: string;
requestUuid: string;
rewrittenUrl?: URL;
traceId?: string;
}

/**
Expand Down
12 changes: 6 additions & 6 deletions src/core/server/logging/layouts/json_layout.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,9 +343,9 @@ test('format() meta can not override tracing properties', () => {
trace: { id: 'trace_override' },
transaction: { id: 'transaction_override' },
},
spanId: 'spanId-1',
traceId: 'traceId-1',
transactionId: 'transactionId-1',
spanId: 'spanId',
traceId: 'traceId',
transactionId: 'transactionId',
})
)
).toStrictEqual({
Expand All @@ -359,8 +359,8 @@ test('format() meta can not override tracing properties', () => {
process: {
pid: 3,
},
span: { id: 'spanId-1' },
trace: { id: 'traceId-1' },
transaction: { id: 'transactionId-1' },
span: { id: 'span_override' },
trace: { id: 'trace_override' },
transaction: { id: 'transaction_override' },
});
});
10 changes: 7 additions & 3 deletions src/core/server/logging/layouts/json_layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ export class JsonLayout implements Layout {
}

public format(record: LogRecord): string {
const spanId = record.meta?.span?.id ?? record.spanId;
const traceId = record.meta?.trace?.id ?? record.traceId;
const transactionId = record.meta?.transaction?.id ?? record.transactionId;

const log: Ecs = {
ecs: { version: '8.0.0' },
'@timestamp': moment(record.timestamp).format('YYYY-MM-DDTHH:mm:ss.SSSZ'),
Expand All @@ -54,9 +58,9 @@ export class JsonLayout implements Layout {
process: {
pid: record.pid,
},
span: record.spanId ? { id: record.spanId } : undefined,
trace: record.traceId ? { id: record.traceId } : undefined,
transaction: record.transactionId ? { id: record.transactionId } : undefined,
span: spanId ? { id: spanId } : undefined,
trace: traceId ? { id: traceId } : undefined,
transaction: transactionId ? { id: transactionId } : undefined,
};
const output = record.meta ? merge({ ...record.meta }, log) : log;

Expand Down
4 changes: 4 additions & 0 deletions x-pack/test/functional_execution_context/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
'--logging.loggers[1].name=execution_context',
'--logging.loggers[1].level=debug',
`--logging.loggers[1].appenders=${JSON.stringify(['file'])}`,

'--logging.loggers[2].name=http.server.response',
'--logging.loggers[2].level=all',
`--logging.loggers[2].appenders=${JSON.stringify(['file'])}`,
],
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import apmAgent from 'elastic-apm-node';

import { Plugin, CoreSetup } from 'kibana/server';
import type { Plugin, CoreSetup } from 'kibana/server';
import { PluginSetupContract as AlertingPluginSetup } from '../../../../../../plugins/alerting/server/plugin';
import { EncryptedSavedObjectsPluginStart } from '../../../../../../plugins/encrypted_saved_objects/server';
import { PluginSetupContract as FeaturesPluginSetup } from '../../../../../../plugins/features/server';
Expand Down Expand Up @@ -93,10 +93,14 @@ export class FixturePlugin implements Plugin<void, void, FixtureSetupDeps, Fixtu
},
},
async (ctx, req, res) => {
// Kibana might set transactiopnSampleRate < 1.0 on CI, so we need to
// enforce transaction creation to prevent the test from failing.
const transaction = apmAgent.startTransaction();
const subscription = req.events.completed$.subscribe(() => {
transaction?.end();
subscription.unsubscribe();
setTimeout(() => {
transaction?.end();
subscription.unsubscribe();
}, 1_000);
});

await ctx.core.elasticsearch.client.asInternalUser.ping();
Expand Down
31 changes: 24 additions & 7 deletions x-pack/test/functional_execution_context/tests/log_correlation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,40 @@ export default function ({ getService }: FtrProviderContext) {

describe('Log Correlation', () => {
it('Emits "trace.id" into the logs', async () => {
const response1 = await supertest
.get('/emit_log_with_trace_id')
.set('x-opaque-id', 'myheader1');

const response1 = await supertest.get('/emit_log_with_trace_id');
expect(response1.status).to.be(200);
expect(response1.body.traceId).to.be.a('string');

const response2 = await supertest.get('/emit_log_with_trace_id');
expect(response2.status).to.be(200);
expect(response1.body.traceId).to.be.a('string');

expect(response2.body.traceId).not.to.be(response1.body.traceId);

let responseTraceId: string | undefined;
await assertLogContains({
description: 'traceId included in the Kibana logs',
predicate: (record) =>
description: 'traceId included in the http logs',
predicate: (record) => {
// we don't check trace.id value since trace.id in the test plugin and Kibana are different on CI.
// because different 'elastic-apm-node' instaces are imported
Boolean(record.http?.request?.id?.includes('myheader1') && record.trace?.id),
if (
record.log?.logger === 'http.server.response' &&
record.url?.path === '/emit_log_with_trace_id'
) {
responseTraceId = record.trace?.id;
return true;
}
return false;
},
retry,
});

expect(responseTraceId).to.be.a('string');

await assertLogContains({
description: 'elasticsearch logs have the same traceId',
predicate: (record) =>
record.log?.logger === 'elasticsearch.query.data' && record.trace?.id === responseTraceId,
mshustov marked this conversation as resolved.
Show resolved Hide resolved
retry,
});
});
Expand Down