fix(instrumentation-http): set outgoing request attributes on start s…

…pan (#2349) Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
open-telemetry · Jul 17, 2021 · 0e3f03f · 0e3f03f
1 parent 5aabcc7
commit 0e3f03f
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 28 deletions.
diff --git a/packages/opentelemetry-instrumentation-http/src/http.ts b/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -38,7 +38,6 @@ import {
   HttpInstrumentationConfig,
   HttpRequestArgs,
   Https,
-  ParsedRequestOptions,
   ResponseEndArgs,
 } from './types';
 import * as utils from './utils';
@@ -272,20 +271,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);
     }
@@ -535,8 +524,19 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
       }
 
       const operationName = `${component.toUpperCase()} ${method}`;
+
+      const hostname =
+        optionsParsed.hostname ||
+        optionsParsed.host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') ||
+        'localhost';
+      const attributes = utils.getOutgoingRequestAttributes(optionsParsed, {
+        component,
+        hostname,
+      });
+
       const spanOptions: SpanOptions = {
         kind: SpanKind.CLIENT,
+        attributes,
       };
       const span = instrumentation._startHttpSpan(operationName, spanOptions);
 
@@ -572,9 +572,8 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
         instrumentation._diag.debug('%s instrumentation outgoingRequest', component);
         context.bind(parentContext, request);
         return instrumentation._traceClientRequest(
-          component,
           request,
-          optionsParsed,
+          hostname,
           span
         );
       });

diff --git a/packages/opentelemetry-instrumentation-http/src/utils.ts b/packages/opentelemetry-instrumentation-http/src/utils.ts
@@ -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 };

diff --git a/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts
@@ -480,12 +480,26 @@ describe('HttpInstrumentation', () => {
       }
 
       it('should have 1 ended span when request throw on bad "options" object', () => {
-        try {
-          http.request({ protocol: 'telnet' });
-        } catch (error) {
+        assert.throws(() => http.request({ headers: { cookie: undefined} }), err => {
           const spans = memoryExporter.getFinishedSpans();
           assert.strictEqual(spans.length, 1);
-        }
+
+          const validations = {
+            httpStatusCode: undefined,
+            httpMethod: 'GET',
+            resHeaders: {},
+            hostname: 'localhost',
+            pathname: '/',
+            forceStatus: {
+              code: SpanStatusCode.ERROR, 
+              message: err.message,
+            },
+            component: 'http',
+            noNetPeer: true,
+          }
+          assertSpan(spans[0], SpanKind.CLIENT, validations);
+          return true;
+        });
       });
 
       it('should have 1 ended span when response.end throw an exception', async () => {

diff --git a/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts b/packages/opentelemetry-instrumentation-http/test/utils/assertSpan.ts
@@ -27,7 +27,7 @@ export const assertSpan = (
   span: ReadableSpan,
   kind: SpanKind,
   validations: {
-    httpStatusCode: number;
+    httpStatusCode?: number;
     httpMethod: string;
     resHeaders: http.IncomingHttpHeaders;
     hostname: string;
@@ -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);
@@ -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