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(instrumentation-http): set outgoing request attributes on start span #2349

Merged
merged 8 commits into from
Jul 17, 2021
47 changes: 28 additions & 19 deletions packages/opentelemetry-instrumentation-http/src/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import {
HttpInstrumentationConfig,
HttpRequestArgs,
Https,
ParsedRequestOptions,
ResponseEndArgs,
} from './types';
import * as utils from './utils';
Expand Down Expand Up @@ -72,7 +71,9 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
return this._config;
}

override setConfig(config: HttpInstrumentationConfig & InstrumentationConfig = {}) {
override setConfig(
config: HttpInstrumentationConfig & InstrumentationConfig = {}
) {
this._config = Object.assign({}, config);
}

Expand Down Expand Up @@ -272,20 +273,10 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
* @param span representing the current operation
*/
private _traceClientRequest(
component: 'http' | 'https',
request: http.ClientRequest,
options: ParsedRequestOptions,
hostname: string,
span: Span
): http.ClientRequest {
const hostname =
options.hostname ||
options.host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') ||
'localhost';
const attributes = utils.getOutgoingRequestAttributes(options, {
component,
hostname,
});
span.setAttributes(attributes);
if (this._getConfig().requestHook) {
this._callRequestHook(span, request);
}
Expand Down Expand Up @@ -378,13 +369,17 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
: '/';
const method = request.method || 'GET';

instrumentation._diag.debug('%s instrumentation incomingRequest', component);
instrumentation._diag.debug(
'%s instrumentation incomingRequest',
component
);

if (
utils.isIgnored(
pathname,
instrumentation._getConfig().ignoreIncomingPaths,
(e: Error) => instrumentation._diag.error('caught ignoreIncomingPaths error: ', e)
(e: Error) =>
instrumentation._diag.error('caught ignoreIncomingPaths error: ', e)
)
) {
return context.with(suppressTracing(context.active()), () => {
Expand Down Expand Up @@ -528,15 +523,27 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
utils.isIgnored(
origin + pathname,
instrumentation._getConfig().ignoreOutgoingUrls,
(e: Error) => instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e)
(e: Error) =>
instrumentation._diag.error('caught ignoreOutgoingUrls error: ', e)
)
) {
return original.apply(this, [optionsParsed, ...args]);
}

const operationName = `${component.toUpperCase()} ${method}`;

const hostname =
optionsParsed.hostname ||
optionsParsed.host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') ||
Flarna marked this conversation as resolved.
Show resolved Hide resolved
'localhost';
const attributes = utils.getOutgoingRequestAttributes(optionsParsed, {
component,
hostname,
});

const spanOptions: SpanOptions = {
kind: SpanKind.CLIENT,
attributes,
};
const span = instrumentation._startHttpSpan(operationName, spanOptions);

Expand Down Expand Up @@ -569,12 +576,14 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
}
);

instrumentation._diag.debug('%s instrumentation outgoingRequest', component);
instrumentation._diag.debug(
'%s instrumentation outgoingRequest',
component
);
context.bind(parentContext, request);
return instrumentation._traceClientRequest(
component,
request,
optionsParsed,
hostname,
span
);
});
Expand Down
7 changes: 6 additions & 1 deletion packages/opentelemetry-instrumentation-http/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,13 @@ export const getAbsoluteUrl = (
* Parse status code from HTTP response. [More details](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md#status)
*/
export const parseResponseStatus = (
statusCode: number
statusCode: number | undefined,
): Omit<SpanStatus, 'message'> => {

if(statusCode === undefined) {
return { code: SpanStatusCode.ERROR };
}

// 1xx, 2xx, 3xx are OK
if (statusCode >= 100 && statusCode < 400) {
return { code: SpanStatusCode.OK };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -479,12 +479,29 @@ describe('HttpInstrumentation', () => {
});
}

it('should have 1 ended span when request throw on bad "options" object', () => {
it('should have 1 ended span when request throw on bad "options" object', done => {
try {
http.request({ protocol: 'telnet' });
http.request({ headers: { cookie: undefined} }); // <===== this makes http.request throw
done('exception should have being thrown but did not');
} catch (error) {
const spans = memoryExporter.getFinishedSpans();
Flarna marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(spans.length, 1);

const validations = {
httpStatusCode: undefined,
httpMethod: 'GET',
resHeaders: {},
hostname: 'localhost',
pathname: '/',
forceStatus: {
code: SpanStatusCode.ERROR,
message: 'Invalid value "undefined" for header "cookie"',
blumamir marked this conversation as resolved.
Show resolved Hide resolved
},
component: 'http',
noNetPeer: true,
}
assertSpan(spans[0], SpanKind.CLIENT, validations);
done();
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const assertSpan = (
span: ReadableSpan,
kind: SpanKind,
validations: {
httpStatusCode: number;
httpStatusCode?: number;
httpMethod: string;
resHeaders: http.IncomingHttpHeaders;
hostname: string;
Expand All @@ -37,6 +37,7 @@ export const assertSpan = (
forceStatus?: SpanStatus;
serverName?: string;
component: string;
noNetPeer?: boolean; // we don't expect net peer info when request throw before being sent
}
) => {
assert.strictEqual(span.spanContext().traceId.length, 32);
Expand Down Expand Up @@ -110,14 +111,16 @@ export const assertSpan = (
validations.hostname,
'must be consistent (PEER_NAME and hostname)'
);
assert.ok(
span.attributes[SemanticAttributes.NET_PEER_IP],
'must have PEER_IP'
);
assert.ok(
span.attributes[SemanticAttributes.NET_PEER_PORT],
'must have PEER_PORT'
);
if(!validations.noNetPeer) {
assert.ok(
span.attributes[SemanticAttributes.NET_PEER_IP],
'must have PEER_IP'
);
assert.ok(
span.attributes[SemanticAttributes.NET_PEER_PORT],
'must have PEER_PORT'
);
}
assert.ok(
(span.attributes[SemanticAttributes.HTTP_URL] as string).indexOf(
span.attributes[SemanticAttributes.NET_PEER_NAME] as string
Expand Down