fix: force http and https clients to be patched (#1084)

package.json

@@ -66,6 +66,7 @@
     "@types/mongoose": "^5.3.26",
     "@types/nock": "^10.0.0",
     "@types/node": "~10.7.2",
+    "@types/node-fetch": "^2.5.0",
     "@types/once": "^1.4.0",
     "@types/proxyquire": "^1.3.28",
     "@types/request": "^2.48.1",

src/tracing.ts

@@ -154,6 +154,11 @@ export class Tracing implements Component {
       .create(this.config.pluginLoaderConfig, tracerComponents)
       .activate();
 
+    // Require http and https again, now that the plugin loader is activated.
+    // This forces them to be patched.
+    require('http');
+    require('https');
+
     if (
       typeof this.config.writerConfig.authOptions.projectId !== 'string' &&
       typeof this.config.writerConfig.authOptions.projectId !== 'undefined'

test/fixtures/plugin-fixtures.json

@@ -151,6 +151,11 @@
       "mysql2": "^1.5.1"
     }
   },
+  "node-fetch2": {
+    "dependencies": {
+      "node-fetch": "^2.6.0"
+    }
+  },
   "pg6": {
     "dependencies": {
       "pg": "^6.1.2"

test/plugins/test-trace-http.ts

@@ -16,11 +16,8 @@
 
 import * as assert from 'assert';
 import {EventEmitter} from 'events';
-import * as fs from 'fs';
 import * as httpModule from 'http';
 import * as httpsModule from 'https';
-import {AddressInfo} from 'net';
-import * as path from 'path';
 import * as stream from 'stream';
 import {URL} from 'url';
 
@@ -30,6 +27,7 @@ import {parseContextFromHeader, TraceContext} from '../../src/util';
 import * as testTraceModule from '../trace';
 import {assertSpanDuration, DEFAULT_SPAN_DURATION} from '../utils';
 import {Express4} from '../web-frameworks/express';
+import {Express4Secure} from '../web-frameworks/express-secure';
 
 // This type describes (http|https).(get|request).
 type HttpRequest = (
@@ -80,39 +78,6 @@ class WaitForResponse {
 
 const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
 
-/**
- * A modification of the Express4 test server that uses HTTPS instead.
- */
-class Express4Secure extends Express4 {
-  static key = fs.readFileSync(
-    path.join(__dirname, '..', 'fixtures', 'key.pem')
-  );
-  static cert = fs.readFileSync(
-    path.join(__dirname, '..', 'fixtures', 'cert.pem')
-  );
-  private https: typeof httpsModule;
-
-  constructor() {
-    super();
-    this.https = require('https');
-  }
-
-  listen(port: number): number {
-    // The types of (http|https).Server are not compatible, but we don't
-    // access any properties that aren't present on both in the test.
-    this.server = (this.https.createServer(
-      {key: Express4Secure.key, cert: Express4Secure.cert},
-      this.app
-    ) as {}) as httpModule.Server;
-    this.server.listen(port);
-    return (this.server.address() as AddressInfo).port;
-  }
-
-  shutdown() {
-    this.server!.close();
-  }
-}
-
 // Server abstraction class definitions. These are borrowed from web framework
 // tests -- which are useful because they already expose a Promise API.
 const servers = {

test/plugins/test-trace-node-fetch.ts

@@ -0,0 +1,116 @@
+/**
+ * Copyright 2019 Google Inc. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as fetchTypes from 'node-fetch'; // For types only.
+import * as testTraceModule from '../trace';
+import * as assert from 'assert';
+import {describeInterop} from '../utils';
+import {Express4} from '../web-frameworks/express';
+import {Express4Secure} from '../web-frameworks/express-secure';
+import {Agent} from 'https';
+import {SpanKind} from '../../src/trace';
+
+// Server abstraction class definitions. These are borrowed from web framework
+// tests -- which are useful because they already expose a Promise API.
+const servers = {
+  http: Express4,
+  https: Express4Secure,
+};
+
+/**
+ * This test is needed because @google-cloud/common uses node-fetch under the
+ * covers, so there is a possibility that we miss the opportunity to patch
+ * http/https core modules. This occurs when the user requires `node-fetch`,
+ * and never transitively requires (one of) `http` or `https` outside of
+ * `node-fetch`, because then the plugin loader will never get the chance to
+ * hook into a `http` or `https` module require.
+ */
+describeInterop<typeof fetchTypes & typeof fetchTypes.default>(
+  'node-fetch',
+  fixture => {
+    before(() => {
+      testTraceModule.setPluginLoaderForTest();
+      testTraceModule.setCLSForTest();
+    });
+
+    after(() => {
+      testTraceModule.setPluginLoaderForTest(testTraceModule.TestPluginLoader);
+      testTraceModule.setCLSForTest(testTraceModule.TestCLS);
+    });
+
+    beforeEach(() => {
+      testTraceModule.clearTraceData();
+    });
+
+    for (const protocol of Object.keys(servers) as Array<
+      keyof typeof servers
+    >) {
+      it(`works with the Trace Agent, ${protocol}`, async () => {
+        // Set up a server. To preserve the condition described in the top-level
+        // description of this test, we ensure that this constructor is called
+        // before the Trace Agent is started, so that the Trace Agent never has
+        // an opportunity to patch http or https upon user require.
+        const server = new servers[protocol]();
+        // Require node-fetch once before starting the Trace Agent. We do this
+        // in lieu of letting it be required when the Trace Agent is started,
+        // because we've mocked out the Trace Writer instance that would
+        // require node-fetch in typical usage.
+        fixture.require();
+        const tracer = testTraceModule.start();
+        const fetch = fixture.require();
+
+        // Set up the server.
+        server.addHandler({
+          path: '/',
+          hasResponse: true,
+          fn: async () => ({statusCode: 200, message: 'OK'}),
+        });
+        const port = server.listen(0);
+
+        // Allow self-signed certificates.
+        let agent: Agent | undefined;
+        if (protocol === 'https') {
+          agent = new Agent({
+            rejectUnauthorized: false,
+          });
+        }
+
+        try {
+          // Make a request against the above server.
+          await tracer.runInRootSpan({name: 'outer'}, async span => {
+            assert.ok(tracer.isRealSpan(span));
+            const response = await fetch(`${protocol}://localhost:${port}`, {
+              agent,
+            });
+            assert.strictEqual(await response.text(), 'OK');
+            span.endSpan();
+          });
+
+          // Get the trace that represents the root span from above..
+          const traces = testTraceModule.getOneTrace(trace =>
+            trace.spans.some(span => span.name === 'outer')
+          );
+          // There should be an HTTP client span.
+          assert.ok(
+            traces.spans.some(span => span.kind === SpanKind.RPC_CLIENT)
+          );
+        } finally {
+          server.shutdown();
+        }
+      });
+    }
+  }
+);

test/web-frameworks/express-secure.ts

@@ -0,0 +1,56 @@
+/**
+ * Copyright 2019 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import {AddressInfo} from 'net';
+
+import * as fs from 'fs';
+import * as path from 'path';
+import {Express4} from './express';
+import * as httpModule from 'http';
+import * as httpsModule from 'https';
+
+/**
+ * A modification of the Express4 test server that uses HTTPS instead.
+ */
+export class Express4Secure extends Express4 {
+  static key = fs.readFileSync(
+    path.join(__dirname, '..', 'fixtures', 'key.pem')
+  );
+  static cert = fs.readFileSync(
+    path.join(__dirname, '..', 'fixtures', 'cert.pem')
+  );
+  private https: typeof httpsModule;
+
+  constructor() {
+    super();
+    this.https = require('https');
+  }
+
+  listen(port: number): number {
+    // The types of (http|https).Server are not compatible, but we don't
+    // access any properties that aren't present on both in the test.
+    this.server = (this.https.createServer(
+      {key: Express4Secure.key, cert: Express4Secure.cert},
+      this.app
+    ) as {}) as httpModule.Server;
+    this.server.listen(port);
+    return (this.server.address() as AddressInfo).port;
+  }
+
+  shutdown() {
+    this.server!.close();
+  }
+}

tsconfig.json

@@ -26,6 +26,7 @@
     "test/plugins/test-trace-http2.ts",
     "test/plugins/test-trace-knex.ts",
     "test/plugins/test-trace-mongoose-async-await.ts",
+    "test/plugins/test-trace-node-fetch.ts",
     "test/logger.ts",
     "test/nocks.ts",
     "test/test-cls.ts",