Add the start() Method and the Ability to Specify the Service Name/Version in the Debug Config (#167)

DominicKramer · web-flow · commit a131faf7a848 · 2016-11-01T11:20:20.000-07:00
* Now the cloud debugger is started by explicitly calling the new `start()` method.  If the `start()` method is not called within 1 second, it is automatically called and a message is issued detailing that the `start()` method will need to be explicitly called in future releases.

* The start method now also accepts an optional configuration object.  Further, any configuration option specified via an environment variable takes priority over configuration options specified in the configuration object given to the `start()` method, which take priority over configuration options specified in the config file specified by the `GCLOUD_DIAGNOSTICS_CONFIG` environment variable.

* The service name and configuration can now be specified in the configuration by specifying the new `serviceContext` attribute as illustrated below:
```
{
   ...
   serviceContext: {
      service: 'service-name',
      version: '1'
   },
   ...
}
```
diff --git a/index.js b/index.js
@@ -24,8 +24,9 @@ var Debuglet = require('./lib/debuglet.js');
 var path = require('path');
 var _ = require('lodash');
 
-var initConfig = function() {
-  var config = {};
+var initConfig = function(config_) {
+  var config = config_ || {};
+
   if (process.env.hasOwnProperty('GCLOUD_DIAGNOSTICS_CONFIG')) {
     var c = require(path.resolve(process.env.GCLOUD_DIAGNOSTICS_CONFIG));
     if (c && c.debug) {
@@ -44,17 +45,43 @@ var initConfig = function() {
     config.appPathRelativeToRepository =
       process.env.GCLOUD_DEBUG_REPO_APP_PATH;
   }
+  if (process.env.hasOwnProperty('GAE_MODULE_NAME')) {
+    config.serviceContext = config.serviceContext || {};
+    config.serviceContext.service = process.env.GAE_MODULE_NAME;
+  }
+  if (process.env.hasOwnProperty('GAE_MODULE_VERSION')) {
+    config.serviceContext = config.serviceContext || {};
+    config.serviceContext.version = process.env.GAE_MODULE_VERSION;
+  }
   return config;
 };
 
-// exports is populated by the agent
-module.exports = {};
-var config = initConfig();
+module.exports = {
+  start: start
+};
 
-var log = logger.create(config.logLevel, '@google/cloud-debug');
+var log_;
+function start(config_) {
+  if (start.wasSuccessful_) {
+    return log_.error('The cloud-debug agent has already been started.');
+  }
 
-if (config.enabled) {
-  var debuglet = new Debuglet(config, log);
-  debuglet.start();
-  module.exports.private_ = debuglet;
+  var config = initConfig(config_);
+  log_ = logger.create(config.logLevel, '@google/cloud-debug');
+  if (config.enabled) {
+    var debuglet = new Debuglet(config, log_);
+    debuglet.start();
+    module.exports.private_ = debuglet;
+    start.wasSuccessful_ = true;
+  }
 }
+
+setTimeout(function() {
+  if (!start.wasSuccessful_){
+    start();
+    log_.error('The @google/cloud-debug agent now needs the start() ' +
+      'function to be called in ordered to start operating. To ease ' +
+      'migration we start the agent automatically after a 1 second delay, '+
+      'but this behaviour will be dropped in a future semver major release.');
+  }
+}, 1000);
diff --git a/lib/debuglet.js b/lib/debuglet.js
@@ -69,7 +69,7 @@ function Debuglet(config, logger) {
   this.logger_ = logger;
 
   /** @private {DebugletApi} */
-  this.debugletApi_ = new DebugletApi(config.description);
+  this.debugletApi_ = new DebugletApi(config);
 
   /** @private {Object.<string, Breakpoint>} */
   this.activeBreakpointMap_ = {};
diff --git a/lib/debugletapi.js b/lib/debugletapi.js
@@ -41,7 +41,9 @@ var SCOPES = [
 /**
  * @constructor
  */
-function DebugletApi(descriptor) {
+function DebugletApi(config) {
+  var config_ = config || {};
+
   /** @private {Object} request style request object */
   this.request_ = utils.authorizedRequestFactory(SCOPES);
 
@@ -52,7 +54,13 @@ function DebugletApi(descriptor) {
   this.debuggeeId_ = null;
 
   /** @private {string} a descriptor of the current code version */
-  this.descriptor_ = descriptor;
+  this.descriptor_ = config_.description;
+
+  /** @private {string} the service name of the current code */
+  this.serviceName_ = config_.serviceContext && config_.serviceContext.service;
+
+  /** @private {string} the version of the current code */
+  this.serviceVersion_ = config_.serviceContext && config_.serviceContext.version;
 }
 
 /**
@@ -142,20 +150,23 @@ DebugletApi.prototype.register_ = function(errorMessage, callback) {
     'projectid': that.project_
   };
 
-  if (process.env.GAE_MODULE_NAME) {
-    desc += ' module:' + process.env.GAE_MODULE_NAME;
-    labels[DEBUGGEE_MODULE_LABEL] = process.env.GAE_MODULE_NAME;
+  var serviceName = that.serviceName_;
+  if (serviceName) {
+    desc += ' module:' + serviceName;
+    labels[DEBUGGEE_MODULE_LABEL] = serviceName;
   }
 
-  if (process.env.GAE_MODULE_VERSION) {
-    desc += ' version:' + process.env.GAE_MODULE_VERSION;
-    if (process.env.GAE_MODULE_VERSION !== 'default') {
-      labels[DEBUGGEE_MAJOR_VERSION_LABEL] = process.env.GAE_MODULE_VERSION;
+  var serviceVersion = that.serviceVersion_;
+  if (serviceVersion) {
+    desc += ' version:' + serviceVersion;
+    if (serviceVersion !== 'default') {
+      labels[DEBUGGEE_MAJOR_VERSION_LABEL] = serviceVersion;
     }
   }
 
-  if (that.descriptor_) {
-    desc += ' description:' + that.descriptor_;
+  var descriptor = that.descriptor_;
+  if (descriptor) {
+    desc += ' description:' + descriptor;
   }
 
   if (process.env.GAE_MINOR_VERSION) {
diff --git a/test/fixtures/test-config.js b/test/fixtures/test-config.js
@@ -23,6 +23,14 @@ module.exports = {
     // An identifier for the current code deployment.
     description: 'test config',
 
+    serviceContext: {
+      // An identifier for the current code's service name
+      service: 'test service name',
+
+      // An identifier for the current code's version
+      version: 'test version'
+    },
+
     // Log levels: 0-disabled,1-error,2-warn,3-info,4-debug.
     logLevel: 4,
 
@@ -31,6 +39,9 @@ module.exports = {
       includeNodeModules: true,
     },
 
+    // This is only for testing the config priority
+    testPriority: 'from the config file',
+
   // These configuration options are for internal experimentation only.
   internal: {
     registerDelayOnFetcherErrorSec: 300 // 5 minutes.
diff --git a/test/standalone/test-env-config.js b/test/standalone/test-env-config.js
@@ -17,16 +17,32 @@
 'use strict';
 
 process.env.GCLOUD_DIAGNOSTICS_CONFIG = 'test/fixtures/test-config.js';
-process.env.GCLOUD_DEBUG_LOGLEVEL = 4;
+process.env.GCLOUD_DEBUG_LOGLEVEL = 1;
+process.env.GAE_MODULE_NAME = 'a new name';
+process.env.GAE_MODULE_VERSION = 'a new version';
 
 var assert = require('assert');
 
 describe('should respect environment variables', function() {
   it('should respect GCLOUD_DIAGNOSTICS_CONFIG', function() {
     var agent = require('../..');
+    agent.start({
+      testPriority: 'from the supplied config',
+      logLevel: 2 // this value is intentionally different from the value
+                  // specified in the config file specified by 
+                  // GCLOUD_DIAGNOSTICS_CONFIG and the value of the 
+                  // environment value GCLOUD_DEBUG_LOGLEVEL
+    });
     var config = agent.private_.config_;
+    // This assert tests that the value set by an environment variable 
+    // takes priority over the value in the config file and priority over 
+    // the value given to the config supplied to the start() method.
     // Set by env var
-    assert.equal(config.logLevel, 4);
+    assert.equal(config.logLevel, 1);
+    // Set by env var
+    assert.equal(config.serviceContext.service, 'a new name');
+    // Set by env var
+    assert.equal(config.serviceContext.version, 'a new version');
     // Set default + user config
     assert.equal(config.internal.registerDelayOnFetcherErrorSec, 300);
     // Set by user
@@ -37,6 +53,11 @@ describe('should respect environment variables', function() {
     assert.equal(config.description, 'test config');
     // In top level config not set by user
     assert.equal(config.breakpointUpdateIntervalSec, 10);
+    // This assert verifies that the value specified in the config given 
+    // to the start() method takes priority over the value specified in 
+    // the config file.
+    // In the config passed to the start() method
+    assert.equal(config.testPriority, 'from the supplied config');
   });
 
 });
diff --git a/test/standalone/test-env-log-level.js b/test/standalone/test-env-log-level.js
@@ -23,6 +23,7 @@ var assert = require('assert');
 describe('should respect environment variables', function() {
   it('should respect GCLOUD_DEBUG_LOGLEVEL', function() {
     var agent = require('../../');
+    agent.start();
     var logger = agent.private_.logger_;
     var STRING1 = 'jjjjjjjjjjjjjjjjjfjfjfjf';
     var STRING2 = 'kkkkkkkfkfkfkfkfkkffkkkk';
diff --git a/test/standalone/test-env-relative-repository-path.js b/test/standalone/test-env-relative-repository-path.js
@@ -29,6 +29,7 @@ var h = require('../fixtures/a/hello.js');
 describe('repository relative paths', function() {
 
   before(function(done) {
+    agent.start();
     setTimeout(function() {
       // Wait for v8debug api to initialize.
       api = agent.private_.v8debug_;
diff --git a/test/standalone/test-module-autostart.js b/test/standalone/test-module-autostart.js
@@ -0,0 +1,29 @@
+/**
+ * Copyright 2016 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.
+ */
+
+'use strict';
+
+var assert = require('assert');
+
+describe('module', function() {
+  it('should autostart', function(done) {
+    var agent = require('../..');
+    setTimeout(function() {
+      assert.strictEqual(agent.start.wasSuccessful_, true);
+      done();
+    }, 2500);
+  });
+});
diff --git a/test/standalone/test-module.js b/test/standalone/test-module.js
@@ -19,18 +19,23 @@
 var assert = require('assert');
 var Debuglet = require('../../lib/debuglet.js');
 
-
 describe('module', function() {
+  var agent;
+
+  before(function() {
+    agent = require('../..');
+    agent.start();
+    assert.strictEqual(agent.start.wasSuccessful_, true);
+  });
 
   it('should return the same agent on a second require', function() {
-    var obj1 = require('../..');
-    var obj2 = require('../..');
-    assert(obj1 === obj2);
+    var obj = require('../..');
+    obj.start();
+    assert(agent === obj);
   });
 
   // Some tests depend on this private property.
   it('should have a debuglet as the private property', function() {
-    var agent = require('../..');
     assert(agent.private_);
 
     // The private_ property needs to be a debuglet.
diff --git a/test/test-debugletapi.js b/test/test-debugletapi.js
@@ -46,7 +46,13 @@ nock.disableNetConnect();
 
 describe('Debuglet API', function() {
 
-  var debugletapi = new DebugletApi('Test debuglet');
+  var debugletapi = new DebugletApi({
+    description: 'Test debuglet',
+    serviceContext: {
+      service: 'TestDebugletName',
+      version: 'TestDebugletVersion'
+    }
+  });
 
   it('should return an instance when constructed', function() {
     assert.ok(debugletapi);
@@ -56,6 +62,14 @@ describe('Debuglet API', function() {
     assert.equal(debugletapi.descriptor_, 'Test debuglet');
   });
 
+  it('should have correct service name', function() {
+    assert.equal(debugletapi.serviceName_, 'TestDebugletName');
+  });
+
+  it('should have correct service version', function() {
+    assert.equal(debugletapi.serviceVersion_, 'TestDebugletVersion');
+  });
+
   it('should acquire the project number during init', function(done) {
     debugletapi.init('uid123', { warn: function() {} }, function(err, project) {
       assert(!err);
