respond to code reviews

Author: lobsterkatie

packages/node/src/integrations/http.ts

@@ -11,7 +11,6 @@ import {
   normalizeRequestArgs,
   RequestMethod,
   RequestMethodArgs,
-  RequestOptions,
 } from './utils/http';
 
 const NODE_VERSION = parseSemver(process.versions.node);
@@ -92,13 +91,16 @@ function _createWrappedRequestMethodFactory(
 ): WrappedRequestMethodFactory {
   return function wrappedRequestMethodFactory(originalRequestMethod: OriginalRequestMethod): WrappedRequestMethod {
     return function wrappedMethod(this: typeof http | typeof https, ...args: RequestMethodArgs): http.ClientRequest {
+      // eslint-disable-next-line @typescript-eslint/no-this-alias
+      const httpModule = this;
+
       const requestArgs = normalizeRequestArgs(args);
       const requestOptions = requestArgs[0];
       const requestUrl = extractUrl(requestOptions);
 
       // we don't want to record requests to Sentry as either breadcrumbs or spans, so just use the original method
       if (isSentryRequest(requestUrl)) {
-        return originalRequestMethod.apply(this, requestArgs);
+        return originalRequestMethod.apply(httpModule, requestArgs);
       }
 
       let span: Span | undefined;
@@ -112,30 +114,40 @@ function _createWrappedRequestMethodFactory(
             description: `${requestOptions.method || 'GET'} ${requestUrl}`,
             op: 'request',
           });
-          addSentryTraceHeader(span, requestOptions);
+
+          const sentryTraceHeader = span.toTraceparent();
+          logger.log(`[Tracing] Adding sentry-trace header to outgoing request: ${sentryTraceHeader}`);
+          requestOptions.headers = { ...requestOptions.headers, 'sentry-trace': sentryTraceHeader };
         }
       }
 
       // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
       return originalRequestMethod
-        .apply(this, requestArgs)
-        .once('response', function(this: http.IncomingMessage, res: http.ServerResponse): void {
+        .apply(httpModule, requestArgs)
+        .once('response', function(this: http.ClientRequest, res: http.IncomingMessage): void {
+          // eslint-disable-next-line @typescript-eslint/no-this-alias
+          const req = this;
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('response', requestUrl, this, res);
+            addRequestBreadcrumb('response', requestUrl, req, res);
           }
           if (tracingEnabled && span) {
-            span.setHttpStatus(res.statusCode);
-            cleanSpanDescription(requestOptions, this, span);
+            if (res.statusCode) {
+              span.setHttpStatus(res.statusCode);
+            }
+            span.description = cleanSpanDescription(span.description, requestOptions, req);
             span.finish();
           }
         })
-        .once('error', function(this: http.IncomingMessage): void {
+        .once('error', function(this: http.ClientRequest): void {
+          // eslint-disable-next-line @typescript-eslint/no-this-alias
+          const req = this;
+
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('error', requestUrl, this);
+            addRequestBreadcrumb('error', requestUrl, req);
           }
           if (tracingEnabled && span) {
             span.setHttpStatus(500);
-            cleanSpanDescription(requestOptions, this, span);
+            span.description = cleanSpanDescription(span.description, requestOptions, req);
             span.finish();
           }
         });
@@ -146,7 +158,7 @@ function _createWrappedRequestMethodFactory(
 /**
  * Captures Breadcrumb based on provided request/response pair
  */
-function addRequestBreadcrumb(event: string, url: string, req: http.IncomingMessage, res?: http.ServerResponse): void {
+function addRequestBreadcrumb(event: string, url: string, req: http.ClientRequest, res?: http.IncomingMessage): void {
   if (!getCurrentHub().getIntegration(Http)) {
     return;
   }
@@ -168,16 +180,3 @@ function addRequestBreadcrumb(event: string, url: string, req: http.IncomingMess
     },
   );
 }
-
-/**
- * Add the `sentry-trace` header to the request options passed to `http(s).request()` and `http(s).get()`.
- *
- * @param span The span representing this outgoing request.
- * @param requestArgs The arguments passed to `http(s).request()` or `http(s).get()`
- *
- */
-function addSentryTraceHeader(span: Span, requestOptions: RequestOptions): void {
-  const headerValue = span.toTraceparent();
-  logger.log(`[Tracing] Adding sentry-trace header to outgoing request: ${headerValue}`);
-  requestOptions.headers = { ...requestOptions.headers, 'sentry-trace': headerValue };
-}

packages/node/src/integrations/utils/http.ts

@@ -1,5 +1,4 @@
 import { getCurrentHub } from '@sentry/core';
-import { Span } from '@sentry/types';
 import * as http from 'http';
 import { URL } from 'url';
 
@@ -39,26 +38,41 @@ export function extractUrl(requestOptions: RequestOptions): string {
 }
 
 /**
- * Handle various edge cases in the span description. Runs just before the span closes because in one case it relies on
- * data from the response object.
+ * Handle various edge cases in the span description (for spans representing http(s) requests).
  *
+ * @param description current `description` property of the span representing the request
  * @param requestOptions Configuration data for the request
- * @param response Response object
- * @param span Span representing the request
+ * @param Request Request object
+ *
+ * @returns The cleaned description
  */
-export function cleanSpanDescription(requestOptions: RequestOptions, response: http.IncomingMessage, span: Span): void {
+export function cleanSpanDescription(
+  description: string | undefined,
+  requestOptions: RequestOptions,
+  request: http.ClientRequest,
+): string | undefined {
+  // nothing to clean
+  if (!description) {
+    return description;
+  }
+
+  // eslint-disable-next-line prefer-const
+  let [method, requestUrl] = description.split(' ');
+
   // superagent sticks the protocol in a weird place (we check for host because if both host *and* protocol are missing,
   // we're likely dealing with an internal route and this doesn't apply)
-  if (requestOptions.host && !('protocol' in requestOptions)) {
+  if (requestOptions.host && !requestOptions.protocol) {
     // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
-    requestOptions.protocol = (response as any).agent?.protocol; // worst comes to worst, this is undefined and nothing changes
-    span.description = `${requestOptions.method || 'GET'} ${extractUrl(requestOptions)}`;
+    requestOptions.protocol = (request as any)?.agent?.protocol; // worst comes to worst, this is undefined and nothing changes
+    requestUrl = extractUrl(requestOptions);
   }
 
   // internal routes can end up starting with a triple slash rather than a single one
-  if (span.description?.startsWith('///')) {
-    span.description = span.description.slice(2);
+  if (requestUrl?.startsWith('///')) {
+    requestUrl = requestUrl.slice(2);
   }
+
+  return `${method} ${requestUrl}`;
 }
 
 // the node types are missing a few properties which node's `urlToOptions` function spits out

scripts/test.sh

@@ -9,7 +9,6 @@ if [[ "$(cut -d. -f1 <<< "$TRAVIS_NODE_VERSION")" -le 6 ]]; then
   # current versions of nock don't support node 6
   cd packages/node
   yarn add --dev --ignore-engines [email protected]
-  yarn list --pattern nock
   cd ../..
   # ember requires Node >= 10 to build
   yarn build --ignore="@sentry/ember" --ignore="@sentry/serverless" --ignore="@sentry/gatsby" --ignore="@sentry/react"