move business logic from controller service to the debuglet (#194)

ofrobots · web-flow · commit a80799826114 · 2016-12-15T22:45:21.000-08:00
diff --git a/src/agent/debuglet.js b/src/agent/debuglet.js
@@ -23,9 +23,11 @@ var util = require('util');
 var semver = require('semver');
 
 var v8debugapi = require('./v8debugapi.js');
+var Debuggee = require('../debuggee.js');
 var DebugletApi = require('../controller.js');
 var scanner = require('./scanner.js');
-var Logger = require('@google/cloud-diagnostics-common').logger;
+var common = require('@google/cloud-diagnostics-common');
+var Logger = common.logger;
 var StatusMessage = require('../status-message.js');
 var SourceMapper = require('./sourcemapper.js');
 
@@ -72,7 +74,10 @@ function Debuglet(debug, config, logger) {
   this.logger_ = logger;
 
   /** @private {DebugletApi} */
-  this.debugletApi_ = new DebugletApi(this.config_, this.debug_);
+  this.debugletApi_ = new DebugletApi(this.debug_);
+
+  /** @private {Debuggee} */
+  this.debuggee_ = null;
 
   /** @private {Object.<string, Breakpoint>} */
   this.activeBreakpointMap_ = {};
@@ -125,33 +130,86 @@ Debuglet.prototype.start = function() {
 
         that.logger_.info('Unique ID for this Application: ' + id);
 
-        that.debugletApi_.init(id, that.logger_, function(err, project) {
+        that.getProjectId_(function(err, project, onGCP) {
           if (err) {
-            that.logger_.error('Unable to initialize the debuglet api' +
-              ' -- disabling debuglet', err);
+            that.logger_.error('Unable to discover projectId. Please provide ' +
+                               'the projectId to be able to use the Debuglet',
+                               err);
             that.emit('initError', err);
             return;
           }
 
-          if (semver.satisfies(process.version, '5.2 || <0.12')) {
-            // Using an unsupported version. We report an error message about the
-            // Node.js version, but we keep on running. The idea is that the user
-            // may miss the error message on the console. This way we can report the
-            // error when the user tries to set a breakpoint.
-            that.logger_.error(NODE_VERSION_MESSAGE);
-          }
+          that.getSourceContext_(function(err, sourceContext) {
+            if (err) {
+              that.logger_.warn('Unable to discover source context', err);
+              // This is ignorable.
+            }
 
-          // We can register as a debuggee now.
-          that.running_ = true;
-          that.project_ = project;
-          that.scheduleRegistration_(0 /* immediately */);
-          that.emit('started');
+            if (semver.satisfies(process.version, '5.2 || <0.12')) {
+              // Using an unsupported version. We report an error message about the
+              // Node.js version, but we keep on running. The idea is that the user
+              // may miss the error message on the console. This way we can report the
+              // error when the user tries to set a breakpoint.
+              that.logger_.error(NODE_VERSION_MESSAGE);
+            }
+
+            // We can register as a debuggee now.
+            that.running_ = true;
+            that.project_ = project;
+            that.debuggee_ = new Debuggee(
+                project, id, that.config_.serviceContext, sourceContext,
+                that.config_.description, null, onGCP);
+            that.scheduleRegistration_(0 /* immediately */);
+            that.emit('started');
+          });
         });
       });
     });
   });
 };
 
+
+/**
+ * @private
+ */
+Debuglet.prototype.getProjectId_ = function(callback) {
+  var that = this;
+
+  // We need to figure out whether we are running on GCP. We can use our ability
+  // to access the metadata service as a test for that.
+  // TODO: change this to getProjectId in the future.
+  common.utils.getProjectNumber(function(err, metadataProject) {
+    // We should get an error if we are not on GCP.
+    var onGCP = !err;
+
+    // We perfer to use the locally available projectId as that is least
+    // surprising to users.
+    var project = that.config_.projectId || process.env.GCLOUD_PROJECT ||
+                  metadataProject;
+
+    // We if don't have a projectId by now, we fail with an error.
+    if (!project) {
+      return callback(err);
+    }
+    return callback(null, project, onGCP);
+  });
+};
+
+Debuglet.prototype.getSourceContext_ =
+    function(callback) {
+  fs.readFile('source-context.json', 'utf8', function(err, data) {
+    // TODO: deal with err here
+    var sourceContext;
+    try {
+      sourceContext = JSON.parse(data);
+    } catch (e) {
+      err = 'Malformed source-context.json file:' + e;
+      // But we keep on going.
+    }
+    return callback(err, sourceContext);
+  });
+};
+
 /**
  * @param {number} seconds
  * @private
@@ -172,7 +230,7 @@ Debuglet.prototype.scheduleRegistration_ = function(seconds) {
       return;
     }
 
-    that.debugletApi_.register(function(err, result) {
+    that.debugletApi_.register(that.debuggee_, function(err, result) {
       if (err) {
         onError(err);
         return;
@@ -185,7 +243,7 @@ Debuglet.prototype.scheduleRegistration_ = function(seconds) {
       }
 
       that.logger_.info('Registered as debuggee:', result.debuggee.id);
-
+      that.debuggee_.id = result.debuggee.id;
       that.emit('registered', result.debuggee.id);
       if (!that.fetcherActive_) {
         that.scheduleBreakpointFetch_(0);
diff --git a/src/controller.js b/src/controller.js
@@ -20,116 +20,34 @@
  * @module debug/controller
  */
 
-var fs = require('fs');
 var assert = require('assert');
 var qs = require('querystring');
-var utils = require('@google/cloud-diagnostics-common').utils;
-var Debuggee = require('./debuggee.js');
 
 /** @const {string} Cloud Debug API endpoint */
 var API = 'https://clouddebugger.googleapis.com/v2/controller';
 
 /**
  * @constructor
  */
-function Controller(config, debug) {
-  config = config || {};
-
+function Controller(debug) {
   /** @priavate {Debug} */
   this.debug_ = debug;
 
-  /** @private {string} project id */
-  this.project_ = config.projectId || process.env.GCLOUD_PROJECT;
-
   /** @private {string} debuggee id provided by the server once registered */
   this.debuggeeId_ = null;
 
-  /** @private {string} a descriptor of the current code version */
-  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;
+  /** @private {string} */
+  this.nextWaitToken_ = null;
 }
 
-/**
- * Initializes the Debuglet API. It requires a unique-id 'uniquifier' string
- * that identifies the version of source we have available on this client so
- * that it can be uniquely identified on the server.
- * @param {!string} uid unique identifier for the version of source loaded
- *     in the client
- * @param {Logger} logger a logger
- * @param {!function(?Error)} callback
- */
-Controller.prototype.init = function(uid, logger, callback) {
-  var that = this;
-  that.uid_ = uid;
-  that.nextWaitToken_ = null;
-
-  // We need to figure out whether we are running on GCP. We can use our ability
-  // to access the metadata service as a test for that.
-  // TODO: change this to getProjectId in the future.
-  utils.getProjectNumber(function(err, metadataProject) {
-    // We should get an error if we are not on GCP.
-    that.onGCP = !err;
-
-    // We prefer to use the locally available projectId as that is least 
-    // surprising to users.
-    var project = that.project_ || metadataProject;
-
-    // We if don't have a projectId by now, we fail with an error.
-    if (!project) {
-      return callback(err);
-    } else {
-      that.project_ = project;
-    }
-
-    // Locate the source context.
-    fs.readFile('source-context.json', 'utf8', function(err, data) {
-      try {
-        that.sourceContext_ = JSON.parse(data);
-      } catch (e) {
-        logger.warn('Malformed source-context.json file.');
-        // But we keep on going.
-      }
-      return callback(null, project);
-    });
-  });
-};
-
-/**
- * Register to the API
- * @param {!function(?Error,Object=)} callback
- */
-Controller.prototype.register = function(callback) {
-  this.register_(null, callback);
-};
-
-
-/**
- * Register an error to the API
- * @param {!string} errorMessage to be reported to the Debug API
- */
-Controller.prototype.registerError = function(message) {
-  this.register_(message, function() {});
-};
-
-
 /**
  * Register to the API (implementation)
- * @param {?string} errorMessage Should be null for normal startup, and non-
- *     null if there is a startup error that should be reported to the API
+ * 
  * @param {!function(?Error,Object=)} callback
  * @private
  */
-Controller.prototype.register_ = function(errorMessage, callback) {
+Controller.prototype.register = function(debuggee, callback) {
   var that = this;
-  var debuggee = new Debuggee(
-      that.project_, that.uid_,
-      {service: that.serviceName_, version: that.serviceVersion_},
-      that.sourceContext_, that.descriptor_, errorMessage, that.onGCP);
 
   var options = {
     uri: API + '/debuggees/register',
diff --git a/system-test/test-controller.js b/system-test/test-controller.js
@@ -17,50 +17,46 @@
 'use strict';
 
 var assert = require('assert');
-var Logger = require('@google/cloud-diagnostics-common').logger;
-var logger = Logger.create();
 
 assert.ok(
     process.env.GCLOUD_PROJECT,
     'Need to have GCLOUD_PROJECT defined ' +
         'along with valid application default credentials to be able to run this ' +
         'test');
-var config = {};
 
-var debug = require('../')(config);
-var DebugletApi = require('../src/controller.js');
+var Controller = require('../src/controller.js');
+var Debuggee = require('../src/debuggee.js');
+var debug = require('../')();
 
-describe('Debugletapi', function() {
+
+describe('Controller', function() {
 
   it('should register successfully', function(done) {
-    var debugletApi = new DebugletApi({}, debug);
-    debugletApi.init('test-uid-1', logger, function(err) {
-      assert.ifError(err, 'init should complete successfully');
+    var controller = new Controller(debug);
+    var debuggee =
+        new Debuggee(process.env.GCLOUD_PROJECT, 'test-uid-' + Date.now());
 
-      debugletApi.register(function(err, body) {
-        assert.ifError(err, 'should be able to register successfull');
-        assert.ok(body);
-        assert.ok(body.debuggee);
-        assert.ok(body.debuggee.id);
-        done();
-      });
+    controller.register(debuggee, function(err, body) {
+      assert.ifError(err, 'should be able to register successfull');
+      assert.ok(body);
+      assert.ok(body.debuggee);
+      assert.ok(body.debuggee.id);
+      done();
     });
   });
 
   it('should list breakpoints', function(done) {
-    var debugletApi = new DebugletApi({}, debug);
-    debugletApi.init('test-uid-2', logger, function(err) {
-      assert.ifError(err, 'init should complete successfully');
+    var controller = new Controller(debug);
+    var debuggee =
+        new Debuggee(process.env.GCLOUD_PROJECT, 'test-uid-' + Date.now());
+    controller.register(debuggee, function(err, body) {
+      assert.ifError(err, 'should be able to register successfull');
 
-      debugletApi.register(function(err, body) {
-        assert.ifError(err, 'should be able to register successfull');
-
-        debugletApi.listBreakpoints(function(err, response, body) {
-          assert.ifError(err, 'should successfully list breakpoints');
-          assert.ok(body);
-          assert.ok(body.nextWaitToken);
-          done();
-        });
+      controller.listBreakpoints(function(err, response, body) {
+        assert.ifError(err, 'should successfully list breakpoints');
+        assert.ok(body);
+        assert.ok(body.nextWaitToken);
+        done();
       });
     });
   });
diff --git a/test/e2e/test-breakpoints.js b/test/e2e/test-breakpoints.js
@@ -283,12 +283,12 @@ if (cluster.isMaster) {
   setTimeout(function() {
     assert.ok(debug.private_, 'debuglet has initialized');
     var debuglet = debug.private_;
-    assert.ok(debuglet.debugletApi_, 'debuglet api is active');
-    var api = debuglet.debugletApi_;
-    assert.ok(api.uid_, 'debuglet provided unique id');
-    assert.ok(api.debuggeeId_, 'debuglet has registered');
+    var debuggee = debuglet.debuggee_;
+    assert.ok(debuggee, 'should create debuggee');
+    assert.ok(debuggee.project, 'debuggee should have a project');
+    assert.ok(debuggee.id, 'debuggee should have registered');
     // The parent process needs to know the debuggeeId and project.
-    process.send([api.debuggeeId_, api.project_]);
+    process.send([debuggee.id, debuggee.project]);
     setInterval(fib.bind(null, 12), 2000);
   }, 7000);
 
diff --git a/test/e2e/test-log-throttling.js b/test/e2e/test-log-throttling.js
@@ -212,11 +212,12 @@ if (cluster.isMaster) {
   setTimeout(function() {
     assert.ok(debug.private_, 'debuglet has initialized');
     var debuglet = debug.private_;
-    assert.ok(debuglet.debugletApi_, 'debuglet api is active');
-    var api = debuglet.debugletApi_;
-    assert.ok(api.uid_, 'debuglet provided unique id');
-    assert.ok(api.debuggeeId_, 'debuglet has registered');
-    process.send([api.debuggeeId_, api.project_]);
+    var debuggee = debuglet.debuggee_;
+    assert.ok(debuggee, 'should create debuggee');
+    assert.ok(debuggee.project, 'debuggee should have a project');
+    assert.ok(debuggee.id, 'debuggee should have registered');
+    // The parent process needs to know the debuggeeId and project.
+    process.send([debuggee.id, debuggee.project]);
     setInterval(fib.bind(null, 12), 500);
   }, 7000);
 }
diff --git a/test/standalone/test-config-credentials.js b/test/standalone/test-config-credentials.js
diff --git a/test/test-controller.js b/test/test-controller.js
