Stop throwing unnecessary errors with adding annotations, metadata, errors (#467)

* Stop throwing unnecessary errors with adding annotations, metadata, and errors.

* Validate segments do not throw errors in tests.

* Avoid adding invalid keys to segment metadata and annotations.

Author: wparad

packages/core/lib/segments/attributes/subsegment.js

@@ -122,12 +122,16 @@ Subsegment.prototype.addPrecursorId = function(id) {
  */
 
 Subsegment.prototype.addAnnotation = function(key, value) {
-  if (!(typeof value === 'boolean' || typeof value === 'string' || (typeof value === 'number' && isFinite(value)))) {
-    throw new Error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
+  if (typeof value !== 'boolean' && typeof value !== 'string' && !isFinite(value)) {
+    logger.getLogger().error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
       this.name + '. Value must be of type string, number or boolean.');
-  } else if (typeof key !== 'string') {
-    throw new Error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
+    return;
+  }
+
+  if (typeof key !== 'string') {
+    logger.getLogger().error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
       this.name + '. Key must be of type string.');
+    return;
   }
 
   if (this.annotations === undefined) {
@@ -147,11 +151,15 @@ Subsegment.prototype.addAnnotation = function(key, value) {
 
 Subsegment.prototype.addMetadata = function(key, value, namespace) {
   if (typeof key !== 'string') {
-    throw new Error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
+    logger.getLogger().error('Failed to add metadata key: ' + key + ' value: ' + value + ' to subsegment ' +
       this.name + '. Key must be of type string.');
-  } else if (namespace && typeof namespace !== 'string') {
-    throw new Error('Failed to add annotation key: ' + key + ' value: ' + value + 'namespace: ' + namespace + ' to subsegment ' +
+    return;
+  }
+
+  if (namespace && typeof namespace !== 'string') {
+    logger.getLogger().error('Failed to add metadata key: ' + key + ' value: ' + value + ' to subsegment ' +
       this.name + '. Namespace must be of type string.');
+    return;
   }
 
   var ns = namespace || 'default';
@@ -164,7 +172,7 @@ Subsegment.prototype.addMetadata = function(key, value, namespace) {
     this.metadata[ns] = {};
   }
 
-  this.metadata[ns][key] = value;
+  this.metadata[ns][key] = value !== null && value !== undefined ? value : '';
 };
 
 Subsegment.prototype.addSqlData = function addSqlData(sqlData) {
@@ -182,8 +190,9 @@ Subsegment.prototype.addSqlData = function addSqlData(sqlData) {
 
 Subsegment.prototype.addError = function addError(err, remote) {
   if (err == null || typeof err !== 'object' && typeof(err) !== 'string') {
-    throw new Error('Failed to add error:' + err + ' to subsegment "' + this.name +
-      '".  Not an object or string literal.');
+    logger.getLogger().error('Failed to add error:' + err + ' to subsegment "' + this.name +
+    '".  Not an object or string literal.');
+    return;
   }
 
   this.addFaultFlag();
@@ -323,8 +332,9 @@ Subsegment.prototype.isClosed = function isClosed() {
 
 Subsegment.prototype.flush = function flush() {
   if (!this.parent || !this.segment) {
-    throw new Error('Failed to flush subsegment: ' + this.name + '. Subsegment must be added ' +
+    logger.getLogger().error('Failed to flush subsegment: ' + this.name + '. Subsegment must be added ' +
       'to a segment chain to flush.');
+    return;
   }
 
   if (this.segment.trace_id) {

packages/core/lib/segments/segment.js

@@ -82,8 +82,14 @@ Segment.prototype.addIncomingRequestData = function addIncomingRequestData(data)
 
 Segment.prototype.addAnnotation = function addAnnotation(key, value) {
   if (typeof value !== 'boolean' && typeof value !== 'string' && !isFinite(value)) {
-    logger.getLogger().error('Add annotation key: ' + key + ' value: ' + value + ' failed.' +
-      ' Annotations must be of type string, number or boolean.');
+    logger.getLogger().error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
+      this.name + '. Value must be of type string, number or boolean.');
+    return;
+  }
+
+  if (typeof key !== 'string') {
+    logger.getLogger().error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
+      this.name + '. Key must be of type string.');
     return;
   }
 
@@ -116,11 +122,15 @@ Segment.prototype.setUser = function (user) {
 
 Segment.prototype.addMetadata = function(key, value, namespace) {
   if (typeof key !== 'string') {
-    throw new Error('Failed to add annotation key: ' + key + ' value: ' + value + ' to subsegment ' +
+    logger.getLogger().error('Failed to add metadata key: ' + key + ' value: ' + value + ' to segment ' +
       this.name + '. Key must be of type string.');
-  } else if (namespace && typeof namespace !== 'string') {
-    throw new Error('Failed to add annotation key: ' + key + ' value: ' + value + 'namespace: ' + namespace + ' to subsegment ' +
+    return;
+  }
+
+  if (namespace && typeof namespace !== 'string') {
+    logger.getLogger().error('Failed to add metadata key: ' + key + ' value: ' + value + ' to segment ' +
       this.name + '. Namespace must be of type string.');
+    return;
   }
 
   var ns = namespace || 'default';
@@ -133,7 +143,7 @@ Segment.prototype.addMetadata = function(key, value, namespace) {
     this.metadata[ns] = {};
   }
 
-  this.metadata[ns][key] = value;
+  this.metadata[ns][key] = value !== null && value !== undefined ? value : '';
 };
 
 /**
@@ -255,8 +265,9 @@ Segment.prototype.removeSubsegment = function removeSubsegment(subsegment) {
 
 Segment.prototype.addError = function addError(err, remote) {
   if (err == null || typeof err !== 'object' && typeof(err) !== 'string') {
-    throw new Error('Failed to add error:' + err + ' to subsegment "' + this.name +
-      '".  Not an object or string literal.');
+    logger.getLogger().error('Failed to add error:' + err + ' to subsegment "' + this.name +
+    '".  Not an object or string literal.');
+    return;
   }
 
   this.addFaultFlag();

packages/core/test/unit/segments/attributes/subsegment.test.js

@@ -6,6 +6,7 @@ var sinonChai = require('sinon-chai');
 var CapturedException = require('../../../../lib/segments/attributes/captured_exception');
 var SegmentUtils = require('../../../../lib/segments/segment_utils');
 var Subsegment = require('../../../../lib/segments/attributes/subsegment');
+var logger = require('../../../../lib/logger');
 
 chai.should();
 chai.use(sinonChai);
@@ -117,10 +118,12 @@ describe('Subsegment', function() {
       assert.equal(subsegment.cause.exceptions.length, 2);
     });
 
-    it('should throw an error on other types', function() {
-      assert.throws(function() {
-        subsegment.addError(3);
-      });
+    it('should accept invalid types and log an error', function() {
+      const loggerMock = sandbox.mock(logger.getLogger());
+      loggerMock.expects('error').once();
+      subsegment.addError(3);
+      loggerMock.verify();
+      assert.notEqual(subsegment.fault, true);
     });
 
     it('should set fault to true by default', function() {
@@ -261,18 +264,12 @@ describe('Subsegment', function() {
       emitStub.reset();
     });
 
-    it('should throw an error if the subsegment has no parent', function() {
+    it('should not throw an error when subsegment flushes', function() {
       delete child.parent;
-      assert.throws( function() {
-        child.flush();
-      }, Error);
-    });
-
-    it('should throw an error if the subsegment has no segment', function() {
-      delete child.segment;
-      assert.throws( function() {
-        child.flush();
-      }, Error);
+      const loggerMock = sandbox.mock(logger.getLogger());
+      loggerMock.expects('error').once();
+      child.flush();
+      loggerMock.verify();
     });
 
     it('should set the parent_id, trace_id and type properties', function() {

packages/core/test/unit/segments/segment.test.js

@@ -8,6 +8,7 @@ var SegmentEmitter = require('../../../lib/segment_emitter');
 var SegmentUtils = require('../../../lib/segments/segment_utils');
 var Segment = require('../../../lib/segments/segment');
 var Subsegment = require('../../../lib/segments/attributes/subsegment');
+var logger = require('../../../lib/logger');
 
 chai.should();
 chai.use(sinonChai);
@@ -280,10 +281,12 @@ describe('Segment', function() {
       assert.equal(segment.cause.exceptions.length, 2);
     });
 
-    it('should throw an error on other types', function() {
-      assert.throws(function() {
-        segment.addError(3);
-      });
+    it('should accept invalid types and log an error', function() {
+      const loggerMock = sandbox.mock(logger.getLogger());
+      loggerMock.expects('error').once();
+      segment.addError(3);
+      loggerMock.verify();
+      assert.notEqual(segment.fault, true);
     });
 
     it('should set fault to true by default', function() {