ref(node): Remove query strings from transaction and span names

packages/node/src/handlers.ts

@@ -11,6 +11,7 @@ import * as os from 'os';
 import * as url from 'url';
 
 import { NodeClient } from './client';
+import { stripQueryString } from './integrations/http';
 import { flush } from './sdk';
 
 const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
@@ -32,7 +33,7 @@ export function tracingHandler(): (
     // TODO: At this point `req.route.path` (which we use in `extractTransaction`) is not available
     // but `req.path` or `req.url` should do the job as well. We could unify this here.
     const reqMethod = (req.method || '').toUpperCase();
-    const reqUrl = req.url;
+    const reqUrl = req.url && stripQueryString(req.url);
 
     let traceId;
     let parentSpanId;

packages/node/src/integrations/http.ts

@@ -102,6 +102,7 @@ function createHandlerWrapper(
           }
           if (tracingEnabled && span) {
             span.setHttpStatus(res.statusCode);
+            cleanDescription(options, this, span);
             span.finish();
           }
         })
@@ -111,6 +112,7 @@ function createHandlerWrapper(
           }
           if (tracingEnabled && span) {
             span.setHttpStatus(500);
+            cleanDescription(options, this, span);
             span.finish();
           }
         });
@@ -145,23 +147,76 @@ function addRequestBreadcrumb(event: string, url: string, req: http.IncomingMess
 }
 
 /**
- * Function that can combine together a url that'll be used for our breadcrumbs.
+ * Strip the query string off of a URL or path
  *
- * @param options url that should be returned or an object containing it's parts.
- * @returns constructed url
+ * @param path Path including possible query string
+ * @returns Path without query string
  */
-function extractUrl(options: string | http.ClientRequestArgs): string {
-  if (typeof options === 'string') {
-    return options;
+export function stripQueryString(path: string): string {
+  if (path.includes('?')) {
+    return path.split('?')[0];
   }
-  const protocol = options.protocol || '';
-  const hostname = options.hostname || options.host || '';
+
+  return path;
+}
+
+/**
+ * Assemble a URL to be used for breadcrumbs and spans.
+ *
+ * @param requestArgs URL string or object containing the component parts
+ * @returns Fully-formed URL
+ */
+export function extractUrl(requestArgs: string | http.ClientRequestArgs): string {
+  if (typeof requestArgs === 'string') {
+    return stripQueryString(requestArgs);
+  }
+  const protocol = requestArgs.protocol || '';
+  const hostname = requestArgs.hostname || requestArgs.host || '';
   // Don't log standard :80 (http) and :443 (https) ports to reduce the noise
-  const port = !options.port || options.port === 80 || options.port === 443 ? '' : `:${options.port}`;
-  const path = options.path || '/';
+  const port = !requestArgs.port || requestArgs.port === 80 || requestArgs.port === 443 ? '' : `:${requestArgs.port}`;
+  const path = requestArgs.path ? stripQueryString(requestArgs.path) : '/';
   return `${protocol}//${hostname}${port}${path}`;
 }
 
+/**
+ * Handle various edge cases with urls in the span description. Runs just before the span closes because it relies on
+ * data from the response object.
+ *
+ * @param requestOptions Configuration data for the request
+ * @param response Constructed request object
+ * @param span Span representing the request
+ */
+function cleanDescription(
+  requestOptions: string | http.ClientRequestArgs,
+  response: http.IncomingMessage,
+  span: Span,
+): void {
+  // There are some libraries which don't pass the request protocol in the options object, so attempt to retrieve it
+  // from the response and run the URL processing again. We only do this in the presence of a (non-empty) host value,
+  // because if we're missing both, it's likely we're dealing with an internal route, in which case we don't want to be
+  // jamming a random `http:` on the front of it.
+  if (typeof requestOptions !== 'string' && !Object.keys(requestOptions).includes('protocol') && requestOptions.host) {
+    // Neither http.IncomingMessage nor any of its ancestors have an `agent` property in their type definitions, and
+    // http.Agent doesn't have a `protocol` property in its type definition. Nonetheless, at least one request library
+    // (superagent) arranges things that way, so might as well give it a shot.
+    try {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
+      requestOptions.protocol = (response as any).agent.protocol;
+      span.description = extractUrl(requestOptions);
+    } catch (error) {
+      // well, we tried
+    }
+  }
+
+  // Internal routes have neither a protocol nor a host, which can leave us with span descriptions like
+  // `///my/internal/route`.
+  if (span.description && span.description.startsWith('///')) {
+    span.description = span.description.slice(2);
+  }
+
+  // nothing to return since we've mutated the span directly
+}
+
 /**
  * Checks whether given url points to Sentry server
  * @param url url to verify

packages/node/test/integrations/http.test.ts

@@ -0,0 +1,33 @@
+import { RequestOptions } from 'http';
+
+import { extractUrl } from './../../src/integrations/http';
+
+describe('extractUrl()', () => {
+  const urlString = 'http://dogs.are.great:1231/yay/';
+  const urlParts: RequestOptions = {
+    protocol: 'http:',
+    host: 'dogs.are.great',
+    method: 'GET',
+    path: '/yay/',
+    port: 1231,
+  };
+  const queryString = '?furry=yes&funny=very';
+
+  it('accepts a url string', () => {
+    expect(extractUrl(urlString)).toBe(urlString);
+  });
+
+  it('accepts a http.RequestOptions object and returns a string with everything in the right place', () => {
+    expect(extractUrl(urlParts)).toBe('http://dogs.are.great:1231/yay/');
+  });
+
+  it('strips query string from url string', () => {
+    const urlWithQueryString = `${urlString}${queryString}`;
+    expect(extractUrl(urlWithQueryString)).toBe(urlString);
+  });
+
+  it('strips query string from path in http.RequestOptions object', () => {
+    const urlPartsWithQueryString = { ...urlParts, path: `${urlParts.path}${queryString}` };
+    expect(extractUrl(urlPartsWithQueryString)).toBe(urlString);
+  });
+}); // end describe('extractUrl()')