newrelic - npm Package Compare versions

Comparing version 0.9.5-65 to 0.9.6-70

lib/agent.js

@@ -19,12 +19,15 @@ 'use strict';
 
+/**
+ * There's a lot of stuff in this constructor, due to Agent acting as the
+ * orchestrator for New Relic within instrumented applications.
+ *
+ * This constructor can throw if, for some reason, the configuration isn't
+ * available. Don't try to recover here, because without configuration the
+ * agent can't be brought up to a useful state.
+ */
 function Agent(options) {
   EventEmitter.call(this);
 
-  this.options     = options || {};
-  this.environment = require(path.join(__dirname, 'environment'));
-
-  // the agent doesn't do anything interesting without a configuration available
-
-  // If configuration is passed in via the options object, use it.
-  // (for testing)
+  // For testing, accept an options object to override bits of agent config.
+  this.options = options || {};
   if (!this.options.config) {
@@ -36,9 +39,13 @@ this.config = require(path.join(__dirname, 'config')).initialize(logger);
   }
-
+  // FIXME: should accept all changes from server-side configuration, not just apdexT
   this.config.on('change', this.updateApdexThreshold.bind(this));
   logger.level(this.config.log_level || 'info');
 
-  this.version = this.config.version;
-  this.errors  = new ErrorService(this.config);
+  this.environment = require(path.join(__dirname, 'environment'));
+  this.version     = this.config.version;
 
+  // error tracing
+  this.errors = new ErrorService(this.config);
+
+  // metrics
   this.apdexT     = (this.config.apdex_t || 0);
@@ -49,18 +56,8 @@ this.renamer    = new RenameRules();
 
-  var Tracer;
-  if (this.config.debug.tracer_tracing) {
-    this.context = new Context(true);
-    Tracer = require(path.join(__dirname, 'transaction', 'tracer', 'debug'));
-    this.tracer  = new Tracer(this, this.context);
-    this.on('transactionFinished', this.logInternalTrace);
-  }
-  else {
-    this.context = new Context();
-    Tracer = require(path.join(__dirname, 'transaction', 'tracer'));
-    this.tracer  = new Tracer(this, this.context);
-  }
-
-  this.traces  = new TraceAggregator(this.config);
+  // transaction traces
+  this.tracer = this.tracerSetup(this.config);
+  this.traces = new TraceAggregator(this.config);
   this.traces.on('harvest', this.submitTransactionSampleData.bind(this));
 
+  // agent events
   this.on('connectReady',        this.collectorSetup.bind(this));
@@ -73,5 +70,10 @@ this.on('transactionFinished', this.mergeTransaction.bind(this));
 
+/**
+ * The agent is meant to only exist once per application, but the singleton is
+ * managed by index.js. An agent will be created even if the agent's disabled by
+ * the configuration. There's probably a better way to do this.
+ */
 Agent.prototype.start = function () {
   if (this.config.agent_enabled !== true) {
-    return logger.warn("The New Relic Node.js agent is disabled in config.js. Not starting!");
+    return logger.warn("The New Relic Node.js agent is disabled by its configuration. Not starting!");
   }
@@ -87,2 +89,9 @@
 
+/**
+ * Any memory claimed by the agent will be retained after stopping.
+ *
+ * FIXME: make it possible to dispose of the agent, as well as do a
+ * "hard" restart. This requires working with shimmer to strip the
+ * current instrumentation and patch to the module loader.
+ */
 Agent.prototype.stop = function () {
@@ -99,9 +108,12 @@ logger.info("Stopping New Relic Node.js instrumentation");
 /**
- * Trigger the listener registered on 'connectReady' in the constructor, but
- * wait a little while if the instrumentation hasn't noticed an application
- * port yet.
+ * Wait a little while for the http instrumentation to notice an application
+ * port, so that information can be sent to the collector with the initial
+ * handshake.
+ *
+ * FIXME: never stops trying to connect to the collector
+ * TODO: make the interval configurable and shorter by default, preferably with back-off
  */
 Agent.prototype.connect = function () {
   if (!this.applicationPort) {
-    logger.debug("No applicationPort set, waiting 15 seconds to try again.");
+    logger.debug("No listeners detected, waiting another 15 seconds before finishing startup.");
     setTimeout(this.emit.bind(this, 'connectReady'), 15 * 1000);
@@ -142,3 +154,3 @@ }
 Agent.prototype.updateRenameRules = function (metricIDs) {
-  if (!metricIDs) logger.warn('Unable to update metric renaming rules: no new rules passed in.');
+  if (!metricIDs) return logger.warn('Unable to update metric renaming rules: no new rules passed in.');
 
@@ -161,2 +173,10 @@ this.renamer.parse(metricIDs);
 
+/**
+ * This is a join point between two event handlers -- the agent instance
+ * waits for a message that it's ready to set up the connection to the
+ * collector, and then registers a bunch of handlers to join functionality
+ * it controls to the collector connection. The messages coming from the
+ * collector connection don't actually appear in the code; they're automatically
+ * created based on the type of the message returned by the collector.
+ */
 Agent.prototype.collectorSetup = function () {
@@ -181,2 +201,31 @@ if (this.connection) return;
 
+/**
+ * To develop the current transaction tracer, I created a tracing tracer that
+ * tracks when transactions, segments and function calls are proxied. This is
+ * used by the tests, but can also be dumped and logged, and is useful for
+ * figuring out where in the execution chain tracing is breaking down.
+ *
+ * @param object config Agent configuration.
+ *
+ * @returns Tracer Either a debugging or production transaction tracer.
+ */
+Agent.prototype.tracerSetup = function (config) {
+  var Tracer;
+  if (config && config.debug && config.debug.tracer_tracing) {
+    this.context = new Context(true);
+    Tracer = require(path.join(__dirname, 'transaction', 'tracer', 'debug'));
+    this.on('transactionFinished', this.logInternalTrace);
+  }
+  else {
+    this.context = new Context();
+    Tracer = require(path.join(__dirname, 'transaction', 'tracer'));
+  }
+
+  return new Tracer(this, this.context);
+};
+
+/**
+ * On agent startup, an interval timer is started that calls this method once
+ * a minute, which in turn invokes the pieces of the harvest cycle.
+ */
 Agent.prototype.harvest = function () {
@@ -191,3 +240,6 @@ if (this.connection && this.connection.isConnected()) {
 /**
- * coalesce and reset the state of the error tracker
+ * For historical reasons, the error handler is reused across harvest cycles
+ * instead of being reused.
+ *
+ * TODO: verify that the error handler doesn't hold references and leak
  */
@@ -201,3 +253,5 @@ Agent.prototype.submitErrorData = function () {
 /**
- * coalesce and reset the state of the gathered metrics
+ * The pieces of supportability metrics are scattered all over the place -- only
+ * send supportability mnetrics if they're explicitly enabled in the
+ * configuration.
  */
@@ -209,3 +263,2 @@ Agent.prototype.submitMetricData = function () {
 
-    // push that thar data to the collector
     this.connection.sendMetricData(metrics.lastSendTime / 1000, Date.now() / 1000, metrics);
@@ -215,3 +268,9 @@ };
 /**
- * When a harvested transaction trace shows up, send it along to be submitted.
+ * The connection methods are written generically, so they expect arrays, even
+ * though top N transaction logic dictates that only one transaction will be
+ * sent per harvest cycle.
+ *
+ * TODO: remove need to wrap trace up in another array.
+ *
+ * @param Array encoded JSON array with encoded contents to be submitted.
  */
@@ -237,2 +296,5 @@ Agent.prototype.submitTransactionSampleData = function (encoded) {
  * upon instantiation.
+ *
+ * @param Metrics metrics The failed metrics submission to be aggregated into
+ *                        the current Metrics instance.
  */
@@ -244,2 +306,7 @@ Agent.prototype.mergeMetrics = function (metrics) {
 
+/**
+ * Trigger the connection to the collector via a side effect.
+ *
+ * @param number port Where the first-noticed HTTP listener is bound.
+ */
 Agent.prototype.noticeAppPort = function (port) {
@@ -287,2 +354,5 @@ logger.debug("Noticed application running on port %d.", port);
  * to agent developers.
+ *
+ * @param Transaction transaction Transaction with a debugging transaction
+ *                                trace.
  */
@@ -289,0 +359,0 @@ Agent.prototype.logInternalTrace = function (transaction) {

lib/context.js

 'use strict';
 
+var path     = require('path')
+  , dominion = require(path.join(__dirname, 'dominion'))
+  ;
+
 /**
@@ -22,8 +26,12 @@ * CONTEXT
 Context.prototype.enter = function (state) {
+  this.state = state;
+
   if (this.stack) this.stack.push(state);
 
-  this.state = state;
+  if (dominion.available && state.domain) state.domain.enter();
 };
 
 Context.prototype.exit = function (state) {
+  if (dominion.available && state.domain) state.domain.exit();
+
   if (this.stack) {
@@ -30,0 +38,0 @@ var top = this.stack.pop();

lib/error.js

@@ -9,16 +9,33 @@ 'use strict';
 
-function createError(transaction) {
-  var message = transaction.statusMessage;
-  if (!message) message = "HttpError " + transaction.statusCode;
+function createError(transaction, exception) {
+  // the collector throws this out
+  var timestamp = 0;
 
+  var scope = 'Unknown';
+  if (transaction && transaction.scope) scope = transaction.scope;
+
+  var message;
+  if (exception && exception.message) {
+    message = exception.message;
+  }
+  else {
+    var code = '';
+    code += (transaction && transaction.statusCode) || 500;
+
+    message = "HttpError " + code;
+  }
+
+  var type = message;
+  if (exception && exception.constructor && exception.constructor.name) {
+    type = exception.constructor.name;
+  }
+
   // FIXME add request_params, custom_params
-  var params = {request_uri : transaction.url};
+  var params = {};
+  if (transaction && transaction.url) params = {request_uri : transaction.url};
 
-  // the collector throws this out
-  var timestamp = 0;
-
   return [timestamp,
-          transaction.scope,
+          scope,
           message,
-          message, // exception class
+          type,
           params];
@@ -45,12 +62,14 @@ }
 ErrorService.prototype.onTransactionFinished = function (transaction) {
-  if (!transaction) throw new Error("Error service was passed a blank transaction.");
+  if (!transaction) throw new Error("Error service got a blank transaction.");
 
   var code = transaction.statusCode;
-  if (code && code >= 400 && !this.ignoreStatusCode(code)) {
-    this.errorCount++;
+  if (code && code >= 400 && !this.ignoreStatusCode(code)) this.add(transaction);
+};
 
-    var error = createError(transaction);
-    logger.trace("Adding error: %j", error);
-    if (this.errors.length < MAX_ERRORS) this.errors.push(error);
-  }
+ErrorService.prototype.add = function (transaction, exception) {
+  this.errorCount++;
+
+  var error = createError(transaction, exception);
+  logger.trace("Adding error: %j", error);
+  if (this.errors.length < MAX_ERRORS) this.errors.push(error);
 };
@@ -57,0 +76,0 @@

lib/instrumentation/core/http.js

@@ -95,10 +95,9 @@ 'use strict';
 
-          /*
-           * Current semantics are that the request has finished when the
-           * first byte hits the stream.
-           *
-           * FIXME: this does not strike me as a sound assumption.
-           */
-          if (this.on) this.on('error', segment.end.bind(segment));
-          if (request.on) request.on('response', segment.end.bind(segment));
+          if (this.once) this.once('error', function (err) {
+            agent.errors.add(err);
+            segment.end();
+          });
+          if (request.on) request.on('response', function (res) {
+            res.once('end', segment.end.bind(segment));
+          });
         }
@@ -127,10 +126,9 @@ }
 
-        /*
-         * Current semantics are that the request has finished when the
-         * first byte hits the stream.
-         *
-         * FIXME: this does not strike me as a sound assumption.
-         */
-        if (this.on) this.on('error', segment.end.bind(segment));
-        if (request.on) request.on('response', segment.end.bind(segment));
+        if (this.once) this.once('error', function (err) {
+          agent.errors.add(err);
+          segment.end();
+        });
+        if (request.on) request.on('response', function (res) {
+          res.once('end', segment.end.bind(segment));
+        });
       }
@@ -137,0 +135,0 @@

lib/transaction/tracer.js

 'use strict';
 
-var path  = require('path')
-  , State = require(path.join(__dirname, 'tracer', 'state'))
+var path     = require('path')
+  , dominion = require(path.join(__dirname, '..', 'dominion'))
+  , State    = require(path.join(__dirname, 'tracer', 'state'))
   ;
@@ -35,4 +36,4 @@
 
-  this.agent           = agent;
-  this.context         = context;
+  this.agent   = agent;
+  this.context = context;
 }
@@ -57,2 +58,3 @@
     var state = new State(transaction, transaction.getTrace().root, handler);
+    if (dominion.available) dominion.add(self.agent, state);
     self.context.enter(state);
@@ -59,0 +61,0 @@ var returned = handler.apply(this, arguments);

lib/transaction/tracer/debug.js

 'use strict';
 
-var path  = require('path')
-  , util  = require('util')
-  , State = require(path.join(__dirname, 'state'))
+var path     = require('path')
+  , util     = require('util')
+  , dominion = require(path.join(__dirname, '..', '..', 'dominion'))
+  , State    = require(path.join(__dirname, 'state'))
   ;
@@ -228,2 +229,3 @@
     var state = new State(transaction, segment, call, true);
+    if (dominion.available) dominion.add(self.agent, state);
     state.describer = describer;
@@ -230,0 +232,0 @@ // NOICE HAX D00D

NEWS.md

@@ -0,1 +1,9 @@
+### v0.9.6-70 / beta-07 (2012-11-30):
+
+* Added first cut at support for error tracing via Node.js 0.8+ domains.
+	Versions of Node.js that support it (v0.8.9 and above) will make a
+	best-faith effort to clean up after errors.
+* Improved non-domain error handling on outbound HTTP requests.
+* Dramatically improved accuracy of HTTP request timing.
+
 ### v0.9.5-63 / beta-06 (2012-11-28):
@@ -2,0 +10,0 @@

package.json

 {
   "name": "newrelic",
-  "version": "0.9.5-65",
+  "version": "0.9.6-70",
   "author": "New Relic Node.js agent team <[e-mail]>",
@@ -5,0 +5,0 @@ "contributors": [

README.md

@@ -11,4 +11,5 @@ # New Relic Node.js agent
 1. [Install node](http://nodejs.org/#download). For now, at least 0.6 is
-   required. Development work is being done against the latest released
-   version.
+   required. Some features (e.g. error tracing) depend in whole or in part on
+   features in 0.8 and above. Development work is being done against the latest
+   released version.
 2. Install this module via `npm install newrelic` for the application you
@@ -22,3 +23,3 @@ want to monitor.
    *IMPORTANT*: formerly this was `require('newrelic_agent')`, and you *MUST*
-	 update your code.
+   update your code.
 
@@ -58,2 +59,2 @@ When you start your app, the agent should start up with it and start reporting
 The New Relic Node.js agent is free-to-use, proprietary software. Please see
-the [full license](LICENSE) for details.
+the full license (found in LICENSE in this distribution) for details.

test/error.test.js

@@ -6,4 +6,6 @@ 'use strict';
   , expect       = chai.expect
+  , should       = chai.should()
   , helper       = require(path.join(__dirname, 'lib', 'agent_helper'))
   , config       = require(path.join(__dirname, '..', 'lib', 'config.default'))
+  , dominion     = require(path.join(__dirname, '..', 'lib', 'dominion'))
   , ErrorService = require(path.join(__dirname, '..', 'lib', 'error'))
@@ -112,3 +114,88 @@ , Transaction  = require(path.join(__dirname, '..', 'lib', 'transaction'))
 
-  it("should put transactions in domains");
+  if (dominion.available) {
+    describe("when domains are available", function () {
+      var mochaHandler
+        , agent
+        , domain
+        , active
+        , json
+        ;
+
+      before(function (done) {
+        /**
+         * Mocha is extremely zealous about trapping errors, and runs each test
+         * in a try / catch block. To get the exception to propagate out to the
+         * domain's uncaughtException handler, we need to put the test in an
+         * asynchronous context and break out of the mocha jail.
+         */
+        process.nextTick(function () {
+          // disable mocha's error handler
+          mochaHandler = process.listeners('uncaughtException').pop();
+
+          agent = helper.loadMockedAgent();
+          var disruptor = agent.tracer.transactionProxy(function () {
+            domain = agent.getTransaction().trace.domain;
+            active = process.domain;
+
+            active.once('error', function (e) {
+              json = agent.errors.errors[0];
+
+              return done();
+            });
+
+            // trigger the domain
+            throw new Error('sample error');
+          });
+
+          disruptor();
+        });
+      });
+
+      after(function () {
+        // ...but be sure to re-enable mocha's error handler
+        process.on('uncaughtException', mochaHandler);
+      });
+
+      it("should put transactions in domains", function () {
+        should.exist(domain);
+        should.exist(active);
+        expect(domain).equal(active);
+      });
+
+      it("should find a single error", function () {
+        expect(agent.errors.errors.length).equal(1);
+      });
+
+      describe("when handed an error from a domain", function () {
+        it("should find the error", function () {
+          should.exist(json);
+        });
+
+        it("should have 5 elements in the trace", function () {
+          expect(json.length).equal(5);
+        });
+
+        it("should always have a 0 (ignored) timestamp", function () {
+          expect(json[0]).equal(0);
+        });
+
+        it("should have the default ('Unknown') scope", function () {
+          expect(json[1]).equal('Unknown');
+        });
+
+        it("should have the error's message", function () {
+          expect(json[2]).equal('sample error');
+        });
+
+        it("should have the error's constructor name (class)", function () {
+          expect(json[3]).equal('Error');
+        });
+
+        it("should default to empty parameters", function () {
+          expect(json[4]).deep.equal({});
+        });
+      });
+    });
+
+  }
 });

test/integration/instrumentation-http.tap.js

@@ -10,3 +10,3 @@ 'use strict';
 
-test("built-in http module instrumentation should trace the reading of directories",
+test("built-in http module instrumentation should handle both internal and external requests",
      function (t) {
@@ -56,3 +56,3 @@ t.plan(12);
                           'Content-Type'   : 'text/html'});
-                         response.end(PAGE);
+      response.end(PAGE);
     };
@@ -76,10 +76,6 @@ };
   this.tearDown(function () {
-    external.close(function shutdown() {
-      server.close(function shutdowner() {
-        t.end();
-      });
-    });
+    external.close();
+    server.close();
   });
 
-  var self = this;
   var testResponseHandler = function (response) {
@@ -103,3 +99,3 @@ if (response.statusCode !== 200) return t.fail(response.statusCode);
         t.bailout("Transaction wasn't set by response handler");
-        return self.emit('end');
+        return this.emit('end');
       }
@@ -130,3 +126,3 @@
     });
-  };
+  }.bind(this);
 
@@ -133,0 +129,0 @@ external.listen(TEST_EXTERNAL_PORT, TEST_HOST, function () {

TODO.md

@@ -16,7 +16,2 @@ ### KNOWN ISSUES:
 * The agent works only with Node.js 0.6 and newer.
-* Server-side configuration is unavailable until support is added within
-  the core New Relic application.
-* Instrumentation for the MongoDB driver only properly instruments queries
-	that include callbacks -- the promise-style and evented interfaces aren't
-	implemented yet.
 * Transaction and error tracing can't be disabled right now.
@@ -30,10 +25,6 @@ * When using Node's included clustering support, each worker process will
 * Additional third-party instrumentation:
-    1. Redis (WIP)
-    2. mikael/request
-    3. PostgreSQL (probably not pre-GA)
-    4. CouchDB (not pre-GA)
-* Use domains for transaction and error tracing when they're available.
+    1. PostgreSQL (probably not pre-GA)
+    2. CouchDB (not pre-GA)
 * Better tests for existing instrumentation.
 * Differentiate between HTTP and HTTPS connections.
-* Publish a build of the agent via npm.
 * Proxy support.
@@ -40,0 +31,0 @@ * Lots more testing of what the data looks like in RPM.

New alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

Dynamic require

Supply chain risk

Dynamic require can indicate the package is performing dangerous or unsafe dynamic code execution.

Found 1 instance in 1 package

Fixed alerts

License Policy Violation

License

This package is not allowed per your license policy. Review the package's license to ensure compliance.

Found 1 instance in 1 package

Improved metrics

Total package byte prevSize

517186

increased by1.45%

Number of package files

176

increased by0.57%

Lines of code

12013

increased by1.66%

Number of lines in readme file

58

increased by1.75%

Worsened metrics

Number of low supply chain risk alerts

206

increased by1.98%

No dependency changes