Skip to content

Commit

Permalink
fix: http server error status
Browse files Browse the repository at this point in the history 
according to the spec 400-499 should be left UNSET in case of server span kind

specifically

> For HTTP status codes in the 4xx range
> span status MUST be left unset in case
> of SpanKind.SERVER and MUST be set to
> Error in case of SpanKind.CLIENT.
  • Loading branch information
@nordfjord
nordfjord committed Feb 16, 2022
1 parent 2da6754 commit a8499c2
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 51 deletions.
9 changes: 5 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 = utils.parseClientResponseStatus(response.statusCode);
}

span.setStatus(status);
Expand All @@ -333,7 +333,8 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
});
response.on('error', (error: Err) => {
this._diag.debug('outgoingRequest on error()', error);
utils.setSpanWithError(span, error, response);
const statusCode = utils.parseClientResponseStatus(response.statusCode);
utils.setSpanWithError(span, error, statusCode.code);
this._closeHttpSpan(span);
});
}
Expand All @@ -346,7 +347,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 +471,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {

span
.setAttributes(attributes)
.setStatus(utils.parseResponseStatus(response.statusCode));
.setStatus(utils.parseServerResponseStatus(response.statusCode));

if (instrumentation._getConfig().applyCustomAttributesOnSpan) {
safeExecuteInTheMiddle(
Expand Down
45 changes: 20 additions & 25 deletions experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
SemanticAttributes,
} from '@opentelemetry/semantic-conventions';
import {
ClientRequest,
IncomingHttpHeaders,
IncomingMessage,
OutgoingHttpHeaders,
Expand Down Expand Up @@ -65,14 +64,26 @@ export const getAbsoluteUrl = (

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

export const parseServerResponseStatus = (statusCode?: number): Omit<SpanStatus, 'message'> => {
if (statusCode === undefined) {
return { code: SpanStatusCode.ERROR };
}

// 1xx, 2xx, 3xx, 4xx are OK from the server perspective
if (statusCode >= 100 && statusCode < 500) {
return { code: SpanStatusCode.OK };
}

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

/**
* 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) {
export const parseClientResponseStatus = (statusCode?: number): Omit<SpanStatus, 'message'> => {
if (statusCode === undefined) {
return { code: SpanStatusCode.ERROR };
}

Expand Down Expand Up @@ -142,12 +153,12 @@ 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.
* @param {SpanStatusCode} [code] used for overriding the status
*/
export const setSpanWithError = (
span: Span,
error: Err,
obj?: IncomingMessage | ClientRequest
code = SpanStatusCode.ERROR
): void => {
const message = error.message;

Expand All @@ -156,23 +167,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, message });
};

/**
Expand Down
73 changes: 52 additions & 21 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 @@ -35,24 +35,45 @@ import { RPCType, setRPCMetadata } from '@opentelemetry/core';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';

describe('Utility', () => {
describe('parseResponseStatus()', () => {
describe('parseClientResponseStatus()', () => {
it('should return ERROR code by default', () => {
const status = utils.parseResponseStatus(
const status = utils.parseClientResponseStatus(
(undefined as unknown) as number
);
assert.deepStrictEqual(status, { code: SpanStatusCode.ERROR });
});

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

it('should not return OK for Bad HTTP status code', () => {
for (let index = 400; index <= 600; index++) {
const status = utils.parseResponseStatus(index);
const status = utils.parseClientResponseStatus(index);
assert.notStrictEqual(status.code, SpanStatusCode.OK);
}
});
});

describe('parseServerResponseStatus()', () => {
it('should return ERROR code by default', () => {
const status = utils.parseClientResponseStatus(undefined);
assert.deepStrictEqual(status, { code: SpanStatusCode.ERROR });
});

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

it('should not return OK for Bad HTTP status code', () => {
for (let index = 500; index <= 600; index++) {
const status = utils.parseClientResponseStatus(index);
assert.notStrictEqual(status.code, SpanStatusCode.OK);
}
});
Expand Down Expand Up @@ -227,23 +248,33 @@ 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]);
});
it('can override the status', () => {
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('error'), SpanStatusCode.UNSET);
assert.strictEqual(span.status, SpanStatusCode.UNSET);
});
});

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)
utils.parseClientResponseStatus(validations.httpStatusCode)
);

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

0 comments on commit a8499c2

Please sign in to comment.