Repository: honeycombio/beeline-nodejs (js)

Issue:
beeline's http instrumentation breaks @sentry/node and potentially other modules with `Error: write EPROTO 140735813014400:error:140770FC:SSL`
Honeycomb beeline seems to break @sentry/node's error reporting.
When using them together, every error seems to get reported as:
```
Error: write EPROTO 140735813014400:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:../deps/openssl/openssl/ssl/s23_clnt.c:827:
```

## Reproduction:
- node.js version: v8.12.0
- `npm start` and then run `curl http://localhost:3000`.

```json
{
  "main": "app.js",
  "scripts": {
    "start": "node app.js"
  },
  "dependencies": {
    "@sentry/node": "4.4.1",
    "bluebird": "3.5.3",
    "express": "4.16.4",
    "honeycomb-beeline": "1.2.1"
  }
}
```

```js
require('honeycomb-beeline')({
  writeKey: 'REDACTED',
  dataset: 'api-traffic',
  serviceName: 'error-tracking'
});
global.Promise = require('bluebird');
Promise.config({
  longStackTraces: true
});
const Sentry = require('@sentry/node');
Sentry.init({ dsn: 'https://REDACTED@sentry.io/REDACTED' });

const app = require('express')();

app.use(Sentry.Handlers.requestHandler());

app.use((req, res, next) => {
  throw new Error('foo');
});

app.use(Sentry.Handlers.errorHandler());

app.listen(3000);
```

### expected:
`Error('foo')` reported to sentry

### actual:
```
Error: write EPROTO 140735813014400:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:../deps/openssl/openssl/ssl/s23_clnt.c:827:

  File "net.js", line 868, col 14, in WriteWrap.afterWrite [as oncomplete]
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/node/dist/transports/base.js", line 40, col 39, in HTTPSTransport.<anonymous>
    return [2 /*return*/, new Promise(function (resolve, reject) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 133, col 27, in step
    op = body.call(thisArg, _);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 114, col 57, in Object.next
    function verb(n) { return function (v) { return step([n, v]); }; }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 107, col 75, in null.<anonymous>
    step((generator = generator.apply(thisArg, _arguments || [])).next());
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 103, col 16, in Object.__awaiter
    return new (P || (P = Promise))(function (resolve, reject) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/core/dist/baseclient.js", line 156, col 115, in null.<anonymous>
    '{snip} processEvent(event, function (finalEvent) { return tslib_1.__awaiter(_this, void 0, void 0, function () { return tslib_1.__generator(this, f {snip}
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/core/dist/baseclient.js", line 334, col 46, in NodeClient.<anonymous>
    return [4 /*yield*/, send(finalEvent)];
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 133, col 27, in step
    op = body.call(thisArg, _);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 114, col 57, in Object.next
    function verb(n) { return function (v) { return step([n, v]); }; }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 104, col 62, in fulfilled
    function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/honeycomb-beeline/lib/async_tracker.js", line 47, col 19, in null.<anonymous>
    return fn.apply(this, args);
  File "domain.js", line 301, col 14, in bound
  File "domain.js", line 314, col 12, in runBound
  File "timers.js", line 810, col 20, in runCallback
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/honeycomb-beeline/lib/instrumentation/bluebird.js", line 18, col 23, in Promise.then
    return original.apply(this, args);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 106, col 128, in step
    '{snip} tion step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 107, col 13, in null.<anonymous>
    step((generator = generator.apply(thisArg, _arguments || [])).next());
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 103, col 16, in Object.__awaiter
    return new (P || (P = Promise))(function (resolve, reject) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/core/dist/baseclient.js", line 147, col 24, in NodeClient.BaseClient.captureEvent
    return tslib_1.__awaiter(this, void 0, void 0, function () {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/core/dist/baseclient.js", line 115, col 64, in NodeClient.<anonymous>
    return [2 /*return*/, this.captureEvent(event, hint, scope)];
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 133, col 27, in step
    op = body.call(thisArg, _);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 114, col 57, in Object.next
    function verb(n) { return function (v) { return step([n, v]); }; }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 104, col 62, in fulfilled
    function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/honeycomb-beeline/lib/async_tracker.js", line 47, col 19, in null.<anonymous>
    return fn.apply(this, args);
  File "domain.js", line 301, col 14, in bound
  File "domain.js", line 314, col 12, in runBound
  File "timers.js", line 810, col 20, in runCallback
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/honeycomb-beeline/lib/instrumentation/bluebird.js", line 18, col 23, in Promise.then
    return original.apply(this, args);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 106, col 128, in step
    '{snip} tion step(result) { result.done ? resolve(result.value) : new P(function (resolve) { resolve(result.value); }).then(fulfilled, rejected); }
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 107, col 13, in null.<anonymous>
    step((generator = generator.apply(thisArg, _arguments || [])).next());
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/tslib/tslib.js", line 103, col 16, in Object.__awaiter
    return new (P || (P = Promise))(function (resolve, reject) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/core/dist/baseclient.js", line 105, col 24, in NodeClient.BaseClient.captureException
    return tslib_1.__awaiter(this, void 0, void 0, function () {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/hub/dist/hub.js", line 67, col 39, in Hub.invokeClientAsync
    (_a = top.client)[method].apply(_a, tslib_1.__spread(args, [top.scope])).catch(function (err) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/hub/dist/hub.js", line 179, col 14, in Hub.captureException
    this.invokeClientAsync('captureException', exception, tslib_1.__assign({}, hint, { event_id: eventId }));
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/minimal/dist/index.js", line 18, col 28, in callOnHub
    return hub[method].apply(hub, tslib_1.__spread(args));
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/minimal/dist/index.js", line 36, col 12, in Object.captureException
    return callOnHub('captureException', exception, {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/@sentry/node/dist/handlers.js", line 179, col 16, in sentryErrorMiddleware
    core_1.captureException(error);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/honeycomb-beeline/lib/instrumentation/express.js", line 167, col 23, in null.<anonymous>
    return middleware.apply(this, [err, req, res, wrappedNext]);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/layer.js", line 71, col 5, in Layer.handle_error
    fn(error, req, res, next);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 315, col 13, in trim_prefix
    layer.handle_error(layerError, req, res, next);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 284, col 7, in null.<anonymous>
    trim_prefix(layer, layerError, layerPath, path);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 335, col 12, in Function.process_params
    return done();
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 275, col 10, in next
    self.process_params(layer, paramcalled, req, res, function (err) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/layer.js", line 97, col 5, in Layer.handle [as handle_request]
    next(err);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 317, col 13, in trim_prefix
    layer.handle_request(req, res, next);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 284, col 7, in null.<anonymous>
    trim_prefix(layer, layerError, layerPath, path);
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 335, col 12, in Function.process_params
    return done();
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/express/lib/router/index.js", line 275, col 10, in next
    self.process_params(layer, paramcalled, req, res, function (err) {
  File "/Users/christian/dev/solvvy-apis/error-tracking/node_modules/honeycomb-beeline/lib/instrumentation/express.js", line 118, col 19, in null.<anonymous>
    return next.apply(this, args);
```
reported to sentry.

Again this happens only when using honeycomb and sentry together. If I remove honeycomb sentry continues to work properly and reports `Error('foo')`. Unfortunately until this is resolved I have to hold off using honeycomb in our apps :/

Pull request:
[http/https] clone the options we're going to mutate for formatting purposes
Fixes #104 

Instead of mutating the options we're going to pass to the normal node http layer, make another clone.  we normalize that separately and only use it for formatting.

We do still mutate the actual options, but only to pass in our header.

Patch:
diff --git a/lib/index.js b/lib/index.js
index 6b3231e..3491ded 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -18,4 +18,6 @@ Object.keys(api).forEach(k => {
   configure[k] = api[k];
 });
 
+configure.getInstrumentations = instrumentation.getInstrumentations;
+
 module.exports = configure;
diff --git a/lib/instrumentation.js b/lib/instrumentation.js
index 9ce9c22..396987c 100644
--- a/lib/instrumentation.js
+++ b/lib/instrumentation.js
@@ -7,7 +7,7 @@ const shimmer = require("shimmer"),
   pkg = require(path.join(__dirname, "..", "package.json")),
   debug = require("debug")(`${pkg.name}:instrumentation`);
 
-const instrumentedModules = new Set([
+const instrumentations = [
   "express",
   "react-dom/server",
   "mysql2",
@@ -19,7 +19,9 @@ const instrumentedModules = new Set([
   "bluebird",
   "sequelize",
   "child_process",
-]);
+];
+
+let enabledInstrumentations = new Set(instrumentations);
 const instrumentedPaths = new Map();
 const instrumentationsActive = new Set();
 exports.activeInstrumentations = () => Array.from(instrumentationsActive.keys()).sort();
@@ -73,7 +75,7 @@ function getPackageVersion(request, options) {
 }
 
 const instrumentLoad = (exports.instrumentLoad = (mod, loadRequest, parent, opts = {}) => {
-  if (parent.id.includes(`node_modules/${pkg.name}`) || !instrumentedModules.has(loadRequest)) {
+  if (parent.id.includes(`node_modules/${pkg.name}`) || !enabledInstrumentations.has(loadRequest)) {
     // no magic here
     return mod;
   }
@@ -117,7 +119,7 @@ const instrumentLoad = (exports.instrumentLoad = (mod, loadRequest, parent, opts
 const checkForAlreadyRequiredModules = () => {
   let modulesRequired = [];
 
-  for (let m of instrumentedModules.values()) {
+  for (let m of enabledInstrumentations.values()) {
     try {
       // try to resolve our known modules in the context of the main module
       let resolvedPath = require.resolve(m, { paths: require.main.paths });
@@ -144,6 +146,11 @@ exports.configure = (opts = {}) => {
   if (opts.disableInstrumentation) {
     return;
   }
+
+  if (opts.enabledInstrumentations !== undefined) {
+    enabledInstrumentations = new Set(opts.enabledInstrumentations);
+  }
+
   if (!opts.disableInstrumentationOnLoad) {
     checkForAlreadyRequiredModules();
 
@@ -157,7 +164,10 @@ exports.configure = (opts = {}) => {
   instrumentPreload();
 };
 
+exports.getInstrumentations = () => instrumentations.slice();
+
 exports.clearInstrumentationForTesting = () => {
+  enabledInstrumentations = new Set(instrumentations);
   instrumentationsActive.clear();
   instrumentedPaths.clear();
 };
diff --git a/lib/instrumentation/http.js b/lib/instrumentation/http.js
index f89bc27..d8224fb 100644
--- a/lib/instrumentation/http.js
+++ b/lib/instrumentation/http.js
@@ -23,18 +23,21 @@ let instrumentHTTP = (http, opts = {}) => {
 
       // let's make sure we have an object (or a url) for options.
       if (typeof options === "string") {
-        options = url.parse(options);
+        options = url.parse(options); // node 10 doesn't use url.parse (except as a fallback).  figure out how to handle that?
       } else {
-        // shallow-clone options since we'll be modifying it below
+        // shallow-clone options since we'll be modifying it below (adding our header)
         options = Object.assign({}, options);
       }
 
+      // another copy so we can normalize things
+      let formatOptions = Object.assign({}, options);
+
       // make sure options is populated enough for url.format to give us reasonable results.  these are all defaults from nodejs docs.
-      options.protocol = options.protocol || "http:";
-      options.port = options.port || 80;
-      options.pathname = options.path || "/";
-      if (!options.hostname && !options.host) {
-        options.hostname = "localhost";
+      formatOptions.protocol = formatOptions.protocol || "http:";
+      formatOptions.port = formatOptions.port !== 80 ? formatOptions.port : null;
+      formatOptions.pathname = formatOptions.path || "/";
+      if (!formatOptions.hostname && !formatOptions.host) {
+        formatOptions.hostname = "localhost";
       }
 
       return api.startAsyncSpan(
@@ -42,7 +45,7 @@ let instrumentHTTP = (http, opts = {}) => {
           [schema.EVENT_TYPE]: "http",
           [schema.PACKAGE_VERSION]: opts.packageVersion,
           [schema.TRACE_SPAN_NAME]: options.method || "GET",
-          url: url.format(options),
+          url: url.format(formatOptions),
         },
         span => {
           let wrapped_cb = function(res) {
diff --git a/lib/instrumentation/https.js b/lib/instrumentation/https.js
index 9c05e2d..a871416 100644
--- a/lib/instrumentation/https.js
+++ b/lib/instrumentation/https.js
@@ -24,18 +24,21 @@ let instrumentHTTPS = (https, opts = {}) => {
 
       // let's make sure we have an object (or a url) for options.
       if (typeof options === "string") {
-        options = url.parse(options);
+        options = url.parse(options); // node 10 doesn't use url.parse (except as a fallback).  figure out how to handle that?
       } else {
-        // shallow-clone options since we'll be modifying it below
+        // shallow-clone options since we'll be modifying it below (adding our header)
         options = Object.assign({}, options);
       }
 
+      // another copy so we can normalize things
+      let formatOptions = Object.assign({}, options);
+
       // make sure options is populated enough for url.format to give us reasonable results.  these are all defaults from nodejs docs.
-      options.protocol = options.protocol || "https:";
-      options.port = options.port !== 443 ? options.port : null;
-      options.pathname = options.path || "/";
-      if (!options.hostname && !options.host) {
-        options.hostname = "localhost";
+      formatOptions.protocol = formatOptions.protocol || "https:";
+      formatOptions.port = formatOptions.port !== 443 ? formatOptions.port : null;
+      formatOptions.pathname = formatOptions.path || "/";
+      if (!formatOptions.hostname && !formatOptions.host) {
+        formatOptions.hostname = "localhost";
       }
 
       return api.startAsyncSpan(
@@ -43,7 +46,7 @@ let instrumentHTTPS = (https, opts = {}) => {
           [schema.EVENT_TYPE]: "https",
           [schema.PACKAGE_VERSION]: opts.packageVersion,
           [schema.TRACE_SPAN_NAME]: options.method || "GET",
-          url: url.format(options),
+          url: url.format(formatOptions),
         },
         span => {
           // in node 8.x, https calls into http.  currently we don't have a way


Test patch:
diff --git a/lib/instrumentation.test.js b/lib/instrumentation.test.js
index 1bd9c13..03048e2 100644
--- a/lib/instrumentation.test.js
+++ b/lib/instrumentation.test.js
@@ -53,6 +53,33 @@ test("instrumentation is keyed off the requested module name, not the module its
   expect(m.__wrapped).toBeUndefined();
 });
 
+test("if an instrumentation is disabled, don't instrument the module", () => {
+  instrumentation.clearInstrumentationForTesting();
+  instrumentation.configure({ enabledInstrumentations: [], disableInstrumentationOnLoad: true });
+  let fakeExpress = createFakeExpress();
+  let m = instrumentation.instrumentLoad(fakeExpress, "express", {
+    id: "foo/stuff/node_modules/",
+  });
+
+  expect(m).toBe(fakeExpress);
+  expect(m.__wrapped).toBeUndefined();
+});
+
+test("if an instrumentation is enabled, instrument the module", () => {
+  instrumentation.clearInstrumentationForTesting();
+  instrumentation.configure({
+    enabledInstrumentations: ["express"],
+    disableInstrumentationOnLoad: true,
+  });
+  let fakeExpress = createFakeExpress();
+  let m = instrumentation.instrumentLoad(fakeExpress, "express", {
+    id: "foo/stuff/node_modules/",
+  });
+
+  expect(m).not.toBe(fakeExpress);
+  expect(m.__wrapped).toBe(true);
+});
+
 test("we keep track of the active instrumentations in a sorted array", () => {
   instrumentation.clearInstrumentationForTesting();
   let fakeExpress = createFakeExpress();
diff --git a/lib/instrumentation/https.test.js b/lib/instrumentation/https.test.js
index a13ebad..5851840 100644
--- a/lib/instrumentation/https.test.js
+++ b/lib/instrumentation/https.test.js
@@ -68,7 +68,7 @@ test("url as options", done => {
               {
                 [schema.EVENT_TYPE]: "http",
                 [schema.TRACE_SPAN_NAME]: "GET",
-                url: "https://google.com:443/",
+                url: "http://google.com:443/",
               },
               {
                 [schema.EVENT_TYPE]: "https",