Skip to content

Commit

Permalink
HTTP 400 status code should not set span status to error on servers (#…
Browse files Browse the repository at this point in the history 
…2789)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
  • Loading branch information
@nordfjord @dyladan
nordfjord and dyladan authored Mar 28, 2022
1 parent b749b0d commit 7870e95
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 66 deletions.
10 changes: 6 additions & 4 deletions experimental/packages/opentelemetry-instrumentation-http/src/http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
if (response.aborted && !response.complete) {
status = { code: SpanStatusCode.ERROR };
} else {
status = utils.parseResponseStatus(response.statusCode);
status = { code: utils.parseResponseStatus(SpanKind.CLIENT, response.statusCode) };
}

span.setStatus(status);
Expand All @@ -333,7 +333,9 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
});
response.on('error', (error: Err) => {
this._diag.debug('outgoingRequest on error()', error);
utils.setSpanWithError(span, error, response);
utils.setSpanWithError(span, error);
const code = utils.parseResponseStatus(SpanKind.CLIENT, response.statusCode);
span.setStatus({ code, message: error.message });
this._closeHttpSpan(span);
});
}
Expand All @@ -346,7 +348,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
});
request.on('error', (error: Err) => {
this._diag.debug('outgoingRequest on request error()', error);
utils.setSpanWithError(span, error, request);
utils.setSpanWithError(span, error);
this._closeHttpSpan(span);
});

Expand Down Expand Up @@ -470,7 +472,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {

span
.setAttributes(attributes)
.setStatus(utils.parseResponseStatus(response.statusCode));
.setStatus({ code: utils.parseResponseStatus(SpanKind.SERVER, response.statusCode) });

if (instrumentation._getConfig().applyCustomAttributesOnSpan) {
safeExecuteInTheMiddle(
Expand Down
45 changes: 11 additions & 34 deletions experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,14 @@ import {
SpanAttributes,
SpanStatusCode,
Span,
SpanStatus,
context,
SpanKind,
} from '@opentelemetry/api';
import {
NetTransportValues,
SemanticAttributes,
} from '@opentelemetry/semantic-conventions';
import {
ClientRequest,
IncomingHttpHeaders,
IncomingMessage,
OutgoingHttpHeaders,
Expand Down Expand Up @@ -65,24 +64,20 @@ export const getAbsoluteUrl = (

return `${protocol}//${host}${path}`;
};

/**
* 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 | 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 };
export const parseResponseStatus = (kind: SpanKind, statusCode?: number): SpanStatusCode => {
const upperBound = kind === SpanKind.CLIENT ? 400 : 500;
// 1xx, 2xx, 3xx are OK on client and server
// 4xx is OK on server
if (statusCode && statusCode >= 100 && statusCode < upperBound) {
return SpanStatusCode.UNSET;
}

// All other codes are error
return { code: SpanStatusCode.ERROR };
return SpanStatusCode.ERROR;
};

/**
Expand Down Expand Up @@ -142,12 +137,10 @@ export const isIgnored = (
* Sets the span with the error passed in params
* @param {Span} span the span that need to be set
* @param {Error} error error that will be set to span
* @param {(IncomingMessage | ClientRequest)} [obj] used for enriching the status by checking the statusCode.
*/
export const setSpanWithError = (
span: Span,
error: Err,
obj?: IncomingMessage | ClientRequest
error: Err
): void => {
const message = error.message;

Expand All @@ -156,23 +149,7 @@ export const setSpanWithError = (
[AttributeNames.HTTP_ERROR_MESSAGE]: message,
});

if (!obj) {
span.setStatus({ code: SpanStatusCode.ERROR, message });
return;
}

let status: SpanStatus;
if ((obj as IncomingMessage).statusCode) {
status = parseResponseStatus((obj as IncomingMessage).statusCode);
} else if ((obj as ClientRequest).aborted) {
status = { code: SpanStatusCode.ERROR };
} else {
status = { code: SpanStatusCode.ERROR };
}

status.message = message;

span.setStatus(status);
span.setStatus({ code: SpanStatusCode.ERROR, message });
};

/**
Expand Down
58 changes: 31 additions & 27 deletions experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,29 @@ import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
describe('Utility', () => {
describe('parseResponseStatus()', () => {
it('should return ERROR code by default', () => {
const status = utils.parseResponseStatus(
(undefined as unknown) as number
);
assert.deepStrictEqual(status, { code: SpanStatusCode.ERROR });
const status = utils.parseResponseStatus(SpanKind.CLIENT, undefined);
assert.deepStrictEqual(status, SpanStatusCode.ERROR);
});

it('should return OK for Success HTTP status code', () => {
it('should return UNSET for Success HTTP status code', () => {
for (let index = 100; index < 400; index++) {
const status = utils.parseResponseStatus(index);
assert.deepStrictEqual(status, { code: SpanStatusCode.OK });
const status = utils.parseResponseStatus(SpanKind.CLIENT, index);
assert.deepStrictEqual(status, SpanStatusCode.UNSET);
}
for (let index = 100; index < 500; index++) {
const status = utils.parseResponseStatus(SpanKind.SERVER, index);
assert.deepStrictEqual(status, SpanStatusCode.UNSET);
}
});

it('should not return OK for Bad HTTP status code', () => {
it('should return ERROR for bad status codes', () => {
for (let index = 400; index <= 600; index++) {
const status = utils.parseResponseStatus(index);
assert.notStrictEqual(status.code, SpanStatusCode.OK);
const status = utils.parseResponseStatus(SpanKind.CLIENT, index);
assert.notStrictEqual(status, SpanStatusCode.UNSET);
}
for (let index = 500; index <= 600; index++) {
const status = utils.parseResponseStatus(SpanKind.SERVER, index);
assert.notStrictEqual(status, SpanStatusCode.UNSET);
}
});
});
Expand Down Expand Up @@ -227,23 +233,21 @@ describe('Utility', () => {
describe('setSpanWithError()', () => {
it('should have error attributes', () => {
const errorMessage = 'test error';
for (const obj of [undefined, { statusCode: 400 }]) {
const span = new Span(
new BasicTracerProvider().getTracer('default'),
ROOT_CONTEXT,
'test',
{ spanId: '', traceId: '', traceFlags: TraceFlags.SAMPLED },
SpanKind.INTERNAL
);
/* tslint:disable-next-line:no-any */
utils.setSpanWithError(span, new Error(errorMessage), obj as any);
const attributes = span.attributes;
assert.strictEqual(
attributes[AttributeNames.HTTP_ERROR_MESSAGE],
errorMessage
);
assert.ok(attributes[AttributeNames.HTTP_ERROR_NAME]);
}
const span = new Span(
new BasicTracerProvider().getTracer('default'),
ROOT_CONTEXT,
'test',
{ spanId: '', traceId: '', traceFlags: TraceFlags.SAMPLED },
SpanKind.INTERNAL
);
/* tslint:disable-next-line:no-any */
utils.setSpanWithError(span, new Error(errorMessage));
const attributes = span.attributes;
assert.strictEqual(
attributes[AttributeNames.HTTP_ERROR_MESSAGE],
errorMessage
);
assert.ok(attributes[AttributeNames.HTTP_ERROR_NAME]);
});
});

Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export const assertSpan = (
assert.deepStrictEqual(
span.status,
validations.forceStatus ||
utils.parseResponseStatus(validations.httpStatusCode)
{ code: utils.parseResponseStatus(span.kind, validations.httpStatusCode) }
);

assert.ok(span.endTime, 'must be finished');
Expand Down

0 comments on commit 7870e95

Please sign in to comment.