HTTP 400 status code should not set span status to error on servers

experimental/packages/opentelemetry-instrumentation-http/src/http.ts

@@ -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);
@@ -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 code = utils.parseResponseStatus(SpanKind.CLIENT, response.statusCode);
+          utils.setSpanWithError(span, error, code);
           this._closeHttpSpan(span);
         });
       }
@@ -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);
     });
 
@@ -470,7 +471,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(

experimental/packages/opentelemetry-instrumentation-http/src/utils.ts

@@ -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,
@@ -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;
 };
 
 /**
@@ -142,12 +137,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;
 
@@ -156,23 +151,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 });
 };
 
 /**

experimental/packages/opentelemetry-instrumentation-http/test/functionals/utils.test.ts

@@ -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);
       }
     });
   });
@@ -227,23 +233,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.code, SpanStatusCode.UNSET);
     });
   });
 

experimental/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts

@@ -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');