From 416031f8079ccb91ed01ebb3467b98bef7df0e6c Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Mon, 19 Dec 2016 22:13:46 -0800 Subject: [PATCH] Move initConfig logic to debuglet (#202) --- src/agent/config.js | 7 ---- src/agent/debuglet.js | 37 +++++++++++++++++---- src/index.js | 37 ++------------------- test/e2e/test-capture-time.js | 4 +-- test/standalone/test-debuglet.js | 29 ++++++++-------- test/standalone/test-options-credentials.js | 11 +++--- 6 files changed, 52 insertions(+), 73 deletions(-) diff --git a/src/agent/config.js b/src/agent/config.js index 80792e81..6ec1a1c7 100644 --- a/src/agent/config.js +++ b/src/agent/config.js @@ -20,13 +20,6 @@ */ module.exports = { - /** - * @property {boolean} Whether the debug agent should be started. - * @memberof DebugAgentConfig - * @default - */ - enabled: true, - // FIXME(ofrobots): presently this is dependent what cwd() is at the time this // file is first required. We should make the default config static. /** diff --git a/src/agent/debuglet.js b/src/agent/debuglet.js index 58d6dd07..c50987aa 100644 --- a/src/agent/debuglet.js +++ b/src/agent/debuglet.js @@ -19,12 +19,14 @@ var fs = require('fs'); var path = require('path'); var EventEmitter = require('events').EventEmitter; +var extend = require('extend'); var util = require('util'); var semver = require('semver'); var v8debugapi = require('./v8debugapi.js'); var Debuggee = require('../debuggee.js'); var DebugletApi = require('../controller.js'); +var defaultConfig = require('./config.js'); var scanner = require('./scanner.js'); var common = require('@google/cloud-diagnostics-common'); var Logger = common.logger; @@ -41,20 +43,21 @@ var BREAKPOINT_ACTION_MESSAGE = 'The only currently supported breakpoint actions module.exports = Debuglet; /** - * @param {Object} config The option parameters for the Debuglet. + * @param {Debug} debug - A Debug instance. + * @param {object=} config - The option parameters for the Debuglet. * @event 'error' on startup errors * @event 'started' once the startup tasks are completed * @event 'registered' once successfully registered to the debug api * @event 'stopped' if the agent stops due to a fatal error after starting * @constructor */ -function Debuglet(debug, config, logger) { +function Debuglet(debug, config) { + /** @private {object} */ + this.config_ = this.normalizeConfig_(config); + /** @private {Debug} */ this.debug_ = debug; - /** @private {object} */ - this.config_ = config || {}; - /** * @private {object} V8 Debug API. This can be null if the Node.js version * is out of date. @@ -71,7 +74,7 @@ function Debuglet(debug, config, logger) { this.fetcherActive_ = false; /** @private {Logger} */ - this.logger_ = logger; + this.logger_ = Logger.create(this.config_.logLevel, '@google-cloud/debug'); /** @private {DebugletApi} */ this.debugletApi_ = new DebugletApi(this.debug_); @@ -90,6 +93,28 @@ function Debuglet(debug, config, logger) { util.inherits(Debuglet, EventEmitter); +Debuglet.prototype.normalizeConfig_ = function(config) { + config = extend({}, defaultConfig, config); + + if (config.keyFilename || config.credentials || config.projectId) { + throw new Error('keyFilename, projectId or credentials should be provided' + + ' to the Debug module constructor rather than startAgent'); + } + + if (process.env.hasOwnProperty('GCLOUD_DEBUG_LOGLEVEL')) { + config.logLevel = process.env.GCLOUD_DEBUG_LOGLEVEL; + } + 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; +}; + /** * Starts the Debuglet. It is important that this is as quick as possible * as it is on the critical path of application startup. diff --git a/src/index.js b/src/index.js index ff4fcd20..f6a89923 100644 --- a/src/index.js +++ b/src/index.js @@ -16,11 +16,9 @@ 'use strict'; -var logger = require('@google/cloud-diagnostics-common').logger; var common = require('@google-cloud/common'); var Debuglet = require('./agent/debuglet.js'); var util = require('util'); -var _ = require('lodash'); /** *

@@ -73,30 +71,6 @@ function Debug(options) { } util.inherits(Debug, common.Service); -var initConfig = function(config_) { - var config = config_ || {}; - - if (config.keyFilename || config.credentials || config.projectId) { - throw new Error('keyFilename, projectId or credentials should be provided' + - ' to the Debug module constructor rather than startAgent'); - } - - var defaults = require('./agent/config.js'); - _.defaultsDeep(config, defaults); - if (process.env.hasOwnProperty('GCLOUD_DEBUG_LOGLEVEL')) { - config.logLevel = process.env.GCLOUD_DEBUG_LOGLEVEL; - } - 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; -}; - var debuglet; /** @@ -118,14 +92,9 @@ Debug.prototype.startAgent = function(config) { throw new Error('Debug Agent has already been started'); } - // FIXME(ofrobots): the initConfig logic belongs in the agent/ directory. - config = initConfig(config); - if (config.enabled) { - debuglet = new Debuglet( - this, config, logger.create(config.logLevel, '@google-cloud/debug')); - debuglet.start(); - this.private_ = debuglet; - } + debuglet = new Debuglet(this, config); + debuglet.start(); + this.private_ = debuglet; }; module.exports = Debug; diff --git a/test/e2e/test-capture-time.js b/test/e2e/test-capture-time.js index e4796e05..67788658 100644 --- a/test/e2e/test-capture-time.js +++ b/test/e2e/test-capture-time.js @@ -17,7 +17,6 @@ var assert = require('assert'); var request = require('request'); -var logger = require('@google/cloud-diagnostics-common').logger; var config = require('../../src/agent/config.js'); var semver = require('semver'); var Debuglet = require('../../src/debuglet.js'); @@ -35,8 +34,7 @@ var debuglet; describe(__filename, function(){ beforeEach(function() { process.env.GCLOUD_PROJECT = 0; - debuglet = new Debuglet( - config, logger.create(config.logLevel, '@google/cloud-debug')); + debuglet = new Debuglet(config); debuglet.once('started', function() { debuglet.debugletApi_.request_ = request; // Avoid authing. }); diff --git a/test/standalone/test-debuglet.js b/test/standalone/test-debuglet.js index 18ea1e85..2fc63468 100644 --- a/test/standalone/test-debuglet.js +++ b/test/standalone/test-debuglet.js @@ -16,7 +16,6 @@ 'use strict'; var assert = require('assert'); -var loggerModule = require('@google/cloud-diagnostics-common').logger; var defaultConfig = require('../../src/agent/config.js'); var Debuglet = require('../../src/agent/debuglet.js'); var extend = require('extend'); @@ -32,8 +31,6 @@ var nock = require('nock'); var nocks = require('../nocks.js'); nock.disableNetConnect(); -var logger = loggerModule.create(process.env.GCLOUD_DEBUG_LOGLEVEL || 0, 'test'); - var bp = { id: 'test', action: 'CAPTURE', @@ -59,7 +56,7 @@ describe(__filename, function(){ it('should not start when projectId is not available', function(done) { var debug = require('../..')(); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); // The following mock is neccessary for the case when the test is running // on GCP. In that case we will get the projectId from the metadata service. @@ -80,7 +77,7 @@ describe(__filename, function(){ it('should not crash without project num', function(done) { var debug = require('../..')(); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); // The following mock is neccessary for the case when the test is running // on GCP. In that case we will get the projectId from the metadata service. @@ -101,7 +98,7 @@ describe(__filename, function(){ it('should accept non-numeric GCLOUD_PROJECT', function(done) { var debug = require('../..')( {projectId: '11020304f2934', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -125,7 +122,7 @@ describe(__filename, function(){ this.timeout(10000); process.env.GCLOUD_PROJECT='11020304f2934'; var debug = require('../..')({credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -153,7 +150,7 @@ describe(__filename, function(){ var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); var config = extend({}, defaultConfig, {workingDirectory: __dirname}); - debuglet = new Debuglet(debug, config, logger); + debuglet = new Debuglet(debug, config); nocks.oauth2(); debuglet.once('initError', function(err) { @@ -167,7 +164,7 @@ describe(__filename, function(){ it('should register successfully otherwise', function(done) { var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = @@ -188,7 +185,7 @@ describe(__filename, function(){ var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -216,7 +213,7 @@ describe(__filename, function(){ this.timeout(4000); var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -253,7 +250,7 @@ describe(__filename, function(){ it('should re-register when registration expires', function(done) { var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -288,7 +285,7 @@ describe(__filename, function(){ this.timeout(2000); var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -320,7 +317,7 @@ describe(__filename, function(){ var debug = require('../..')( {projectId: 'fake-project', credentials: fakeCredentials}); - debuglet = new Debuglet(debug, defaultConfig, logger); + debuglet = new Debuglet(debug, defaultConfig); nocks.oauth2(); var scope = nock(API) @@ -390,7 +387,7 @@ describe(__filename, function(){ }) .reply(200); - debuglet = new Debuglet(debug, config, logger); + debuglet = new Debuglet(debug, config); debuglet.once('registered', function(id) { assert.equal(id, DEBUGGEE_ID); setTimeout(function() { @@ -444,7 +441,7 @@ describe(__filename, function(){ breakpoints: [bp] }); - debuglet = new Debuglet(debug, config, logger); + debuglet = new Debuglet(debug, config); debuglet.once('registered', function(id) { assert.equal(id, DEBUGGEE_ID); setTimeout(function() { diff --git a/test/standalone/test-options-credentials.js b/test/standalone/test-options-credentials.js index 8e8b9e64..696d1bf6 100644 --- a/test/standalone/test-options-credentials.js +++ b/test/standalone/test-options-credentials.js @@ -20,7 +20,6 @@ var assert = require('assert'); var nock = require('nock'); var nocks = require('../nocks.js'); var extend = require('extend'); -var logger = require('@google/cloud-diagnostics-common').logger; var defaultOptions = {}; var config = require('../../src/agent/config.js'); var Debuglet = require('../../src/agent/debuglet.js'); @@ -66,8 +65,7 @@ describe('test-options-credentials', function() { return true; }); - debuglet = - new Debuglet(debug, config, logger.create(logger.WARN, 'testing')); + debuglet = new Debuglet(debug, config); debuglet.start(); }); @@ -90,8 +88,7 @@ describe('test-options-credentials', function() { setImmediate(done); return true; }); - debuglet = - new Debuglet(debug, config, logger.create(logger.WARN, 'testing')); + debuglet = new Debuglet(debug, config); debuglet.start(); }); @@ -113,7 +110,7 @@ describe('test-options-credentials', function() { setImmediate(done); return true; }); - debuglet = new Debuglet(debug, config, logger.create(undefined, 'testing')); + debuglet = new Debuglet(debug, config); debuglet.start(); }); @@ -148,7 +145,7 @@ describe('test-options-credentials', function() { assert(options.credentials.hasOwnProperty(field)); assert.notEqual(options.credentials[field], fileCredentials[field]); }); - debuglet = new Debuglet(debug, config, logger.create(undefined, 'testing')); + debuglet = new Debuglet(debug, config); debuglet.start(); }); });