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

fix: link net and http instrumentations correctly #2046

Merged
merged 12 commits into from
Mar 29, 2021
62 changes: 40 additions & 22 deletions packages/opentelemetry-instrumentation-http/src/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,12 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
this._callRequestHook(span, request);
}

request.on(
/*
* User 'response' event listeners can be added before our listener,
* force our listener to be the first, so response emitter is bound
* before any user listeners are added to it.
*/
request.prependListener(
'response',
(response: http.IncomingMessage & { aborted?: boolean }) => {
const responseAttributes = utils.getOutgoingRequestAttributesOnResponse(
Expand Down Expand Up @@ -427,6 +432,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
response.end = originalEnd;
// Cannot pass args of type ResponseEndArgs,
const returned = safeExecuteInTheMiddle(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
() => response.end.apply(this, arguments as any),
error => {
if (error) {
Expand Down Expand Up @@ -528,33 +534,45 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
kind: SpanKind.CLIENT,
};
const span = instrumentation._startHttpSpan(operationName, spanOptions);

const parentContext = context.active();
const requestContext = setSpan(parentContext, span);

if (!optionsParsed.headers) {
optionsParsed.headers = {};
}
propagation.inject(
setSpan(context.active(), span),
optionsParsed.headers
);
propagation.inject(requestContext, optionsParsed.headers);

return context.with(requestContext, () => {
/*
* The response callback is registered before ClientRequest is bound,
* thus it is needed to bind it before the function call.
*/
const cb = args[args.length - 1];
if (typeof cb === 'function') {
args[args.length - 1] = context.bind(cb, parentContext);
}

const request: http.ClientRequest = safeExecuteInTheMiddle(
() => original.apply(this, [optionsParsed, ...args]),
error => {
if (error) {
utils.setSpanWithError(span, error);
instrumentation._closeHttpSpan(span);
throw error;
const request: http.ClientRequest = safeExecuteInTheMiddle(
() => original.apply(this, [optionsParsed, ...args]),
error => {
if (error) {
utils.setSpanWithError(span, error);
instrumentation._closeHttpSpan(span);
throw error;
}
}
}
);
);

diag.debug('%s instrumentation outgoingRequest', component);
context.bind(request);
return instrumentation._traceClientRequest(
component,
request,
optionsParsed,
span
);
diag.debug('%s instrumentation outgoingRequest', component);
context.bind(request, parentContext);
return instrumentation._traceClientRequest(
component,
request,
optionsParsed,
span
);
});
};
}

Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-instrumentation-http/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export type ParsedRequestOptions =
| http.RequestOptions;
export type Http = typeof http;
export type Https = typeof https;
/* tslint:disable-next-line:no-any */
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Func<T> = (...args: any[]) => T;
export type ResponseEndArgs =
| [((() => void) | undefined)?]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ describe('HttpInstrumentation', () => {
);
});
});

describe('with good instrumentation options', () => {
beforeEach(() => {
memoryExporter.reset();
Expand Down Expand Up @@ -705,7 +706,15 @@ describe('HttpInstrumentation', () => {
assert.deepStrictEqual(getSpan(context.active()), undefined);
http.get(`${protocol}://${hostname}:${serverPort}/test`, res => {
assert.deepStrictEqual(getSpan(context.active()), undefined);
done();

res.on('data', () => {
assert.deepStrictEqual(getSpan(context.active()), undefined);
});

res.on('end', () => {
assert.deepStrictEqual(getSpan(context.active()), undefined);
done();
});
});
});
});
Expand Down