Inspector only enabled when specified (#343)

gaofanmichael · web-flow · commit c15872df2c17 · 2017-10-20T16:06:06.000-07:00
As side effect cannot be guaranteed in runtime.getProperties, we will not enable v8 inspector by default. Instead, use will need to explicitly set GCLOUD_USE_INSPECTOR=true to run with v8 inspector.
diff --git a/appveyor.yml b/appveyor.yml
@@ -19,6 +19,9 @@ install:
 test_script:
   # run tests
   - npm run build
+  - SET GCLOUD_USE_INSPECTOR=
+  - node_modules/.bin/mocha build/test --timeout 4000 --R
+  - SET GCLOUD_USE_INSPECTOR=1
   - node_modules/.bin/mocha build/test --timeout 4000 --R
 
 # Don't actually build using MSBuild
diff --git a/bin/run-test.sh b/bin/run-test.sh
@@ -30,7 +30,8 @@ function run {
 }
 
 # Run test/coverage
-run build/test
+GCLOUD_USE_INSPECTOR= run build/test
+GCLOUD_USE_INSPECTOR=true run build/test
 
 # Conditionally publish coverage
 if [ "$cover" ]; then
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -45,7 +45,7 @@
     "gulp-typescript": "^3.1.6",
     "istanbul": "^0.4.1",
     "merge2": "^1.0.3",
-    "mocha": "^3.0.0",
+    "mocha": "^3.5.3",
     "nock": "^9.0.0",
     "request": "^2.81.0",
     "source-map-support": "^0.4.15",
diff --git a/src/agent/util/utils.ts b/src/agent/util/utils.ts
@@ -28,7 +28,9 @@ export const messages = {
   ASYNC_TRACES_WARNING:
       'The Stackdriver Debugger for Node.js does not require V8 Inspector ' +
       'async stack traces. The INSPECTOR_ASYNC_STACK_TRACES_NOT_AVAILABLE ' +
-      'can be ignored.'
+      'can be ignored.',
+  INSPECTOR_NOT_AVAILABLE:
+      'The V8 Inspector protocol is only available in Node 8+'
 };
 
 export interface Listener {
diff --git a/src/agent/v8/debugapi.ts b/src/agent/v8/debugapi.ts
@@ -41,12 +41,9 @@ interface DebugApiConstructor {
 }
 
 let debugApiConstructor: DebugApiConstructor;
-const nodeVersion = /v(\d+\.\d+\.\d+)/.exec(process.version);
 
-if (!nodeVersion || nodeVersion.length < 2) {
-  const dummyapi = require('./dummy-debugapi');
-  debugApiConstructor = dummyapi.DummyDebugApi;
-} else if (semver.satisfies(nodeVersion[1], '>=8')) {
+if (semver.satisfies(process.version, '>=8') &&
+    process.env.GCLOUD_USE_INSPECTOR) {
   const inspectorapi = require('./inspector-debugapi');
   debugApiConstructor = inspectorapi.InspectorDebugApi;
 } else {
diff --git a/src/agent/v8/dummy-debugapi.ts b/src/agent/v8/dummy-debugapi.ts
diff --git a/src/agent/v8/legacy-debugapi.ts b/src/agent/v8/legacy-debugapi.ts
@@ -90,6 +90,9 @@ export class V8DebugApi implements debugapi.DebugApi {
             this.logger.warn('Internal V8 error on breakpoint event: ' + e);
           }
         };
+    if (process.env.GCLOUD_USE_INSPECTOR) {
+      this.logger.warn(utils.messages.INSPECTOR_NOT_AVAILABLE);
+    }
     if (this.usePermanentListener) {
       this.logger.info('activating v8 breakpoint listener (permanent)');
 
diff --git a/test/test-debuglet.ts b/test/test-debuglet.ts
@@ -341,7 +341,8 @@ describe('Debuglet', function() {
         assert.equal((debuglet.debuggee_ as Debuggee).project, projectId);
         const arch = process.arch;
         if (semver.satisfies(process.version, '>=8.5') &&
-            (arch === 'ia32' || arch === 'x86')) {
+            (arch === 'ia32' || arch === 'x86') &&
+            process.env.GCLOUD_USE_INSPECTOR) {
           assert(logText.includes(utils.messages.ASYNC_TRACES_WARNING));
         } else {
           assert(!logText.includes(utils.messages.ASYNC_TRACES_WARNING));
diff --git a/test/test-v8debugapi.ts b/test/test-v8debugapi.ts
@@ -110,6 +110,44 @@ function validateBreakpoint(breakpoint: stackdriver.Breakpoint): void {
     breakpoint.stackFrames.forEach(validateStackFrame);
   }
 }
+describe('debugapi selection', function() {
+  const config: DebugAgentConfig = extend({}, defaultConfig, {
+    workingDirectory: __dirname,
+    forceNewAgent_: true
+  });
+  const logger = new common.logger({ levelLevel: config.logLevel } as any as LoggerOptions);
+  let logText = '';
+  logger.warn = function(s: string) {
+    logText += s;
+  }
+  it('should use the correct debugapi and have appropriate warning', (done) => {
+    let api: DebugApi;
+    scanner.scan(true, config.workingDirectory as string, /.js$|.map$/)
+    .then(function (fileStats) {
+      const jsStats = fileStats.selectStats(/.js$/);
+      const mapFiles = fileStats.selectFiles(/.map$/, process.cwd());
+      SourceMapper.create(mapFiles, function (err, mapper) {
+        assert(!err);
+        // TODO: Handle the case when mapper is undefined.
+        // TODO: Handle the case when v8debugapi.create returns null
+        api = debugapi.create(logger, config, jsStats, mapper as SourceMapper.SourceMapper) as DebugApi;
+        if (process.env.GCLOUD_USE_INSPECTOR && semver.satisfies(process.version, '>=8')) {
+          const inspectorapi = require('../src/agent/v8/inspector-debugapi');
+          assert.ok(api instanceof inspectorapi.InspectorDebugApi);
+        } else {
+          const v8debugapi = require('../src/agent/v8/legacy-debugapi');
+          assert.ok(api instanceof v8debugapi.V8DebugApi);
+        }
+        if (process.env.GCLOUD_USE_INSPECTOR && semver.satisfies(process.version, '<8')) {
+          assert(logText.includes(utils.messages.INSPECTOR_NOT_AVAILABLE));
+        } else {
+          assert(!logText.includes(utils.messages.INSPECTOR_NOT_AVAILABLE));
+        }
+        done();
+      });
+    });
+  });
+});
 
 describe('v8debugapi', function() {
   const config: DebugAgentConfig = extend({}, defaultConfig, {

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,8 @@ function run {`
`30`	`30`	`}`
`31`	`31`
`32`	`32`	`# Run test/coverage`
`33`		`-run build/test`
	`33`	`+GCLOUD_USE_INSPECTOR= run build/test`
	`34`	`+GCLOUD_USE_INSPECTOR=true run build/test`
`34`	`35`
`35`	`36`	`# Conditionally publish coverage`
`36`	`37`	`if [ "$cover" ]; then`
Original file line number	Diff line number	Diff line change
`@@ -90,6 +90,9 @@ export class V8DebugApi implements debugapi.DebugApi {`
`90`	`90`	`this.logger.warn('Internal V8 error on breakpoint event: ' + e);`
`91`	`91`	`}`
`92`	`92`	`};`
	`93`	`+ if (process.env.GCLOUD_USE_INSPECTOR) {`
	`94`	`+ this.logger.warn(utils.messages.INSPECTOR_NOT_AVAILABLE);`
	`95`	`+ }`
`93`	`96`	`if (this.usePermanentListener) {`
`94`	`97`	`this.logger.info('activating v8 breakpoint listener (permanent)');`
`95`	`98`