address comments

Signed-off-by: Won Jun Jang <wjang@uber.com>

src/_flow/throttler.js

@@ -18,6 +18,7 @@
  */
 declare interface Throttler {
   isAllowed(operation: string): boolean;
+  close(callback?: Function): void;
 }
 
 declare type CreditResponse = {

src/configuration.js

@@ -23,6 +23,7 @@ import UDPSender from './reporters/udp_sender';
 import opentracing from 'opentracing';
 import * as constants from './constants.js';
 import RemoteThrottler from './throttler/remote_throttler';
+import Utils from './util.js';
 
 let jaegerSchema = {
   id: '/jaeger',
@@ -131,8 +132,7 @@ export default class Configuration {
   }
 
   static _getThrottler(config, options) {
-    // TODO Use Object.assign instead, only supported in Node 4+
-    const throttlerOptions = JSON.parse(JSON.stringify(config.throttler));
+    const throttlerOptions = Utils.clone(config.throttler);
     if (options.logger) {
       throttlerOptions.logger = options.logger;
     }

src/metrics/metrics.js

@@ -33,6 +33,7 @@ export default class Metrics {
   baggageUpdateSuccess: Counter;
   baggageUpdateFailure: Counter;
   baggageTruncate: Counter;
+  throttledDebugSpans: Counter;
   throttlerUpdateSuccess: Counter;
   throttlerUpdateFailure: Counter;
 
@@ -123,6 +124,8 @@ export default class Metrics {
 
     this.baggageTruncate = this._factory.createCounter('baggage-trucate');
 
+    this.throttledDebugSpans = this._factory.createCounter('throttled_debug_spans');
+
     this.throttlerUpdateSuccess = this._factory.createCounter('throttler-update', {
       result: 'ok',
     });

src/span.js

@@ -204,17 +204,16 @@ export default class Span {
    * @return {Span} - returns this span.
    **/
   addTags(keyValuePairs: any): Span {
-    if (opentracing.Tags.SAMPLING_PRIORITY in keyValuePairs) {
-      // Delete the SAMPLING_PRIORITY tag if it was not set
-      if (!this._setSamplingPriority(keyValuePairs[opentracing.Tags.SAMPLING_PRIORITY])) {
-        delete keyValuePairs[opentracing.Tags.SAMPLING_PRIORITY];
-      }
-    }
-
+    const samplingKey = opentracing.Tags.SAMPLING_PRIORITY;
+    const samplingPriority = keyValuePairs[samplingKey];
+    const samplingPriorityWasSet = samplingPriority != null && this._setSamplingPriority(samplingPriority);
     if (this._isWriteable()) {
       for (let key in keyValuePairs) {
         if (keyValuePairs.hasOwnProperty(key)) {
-          let value = keyValuePairs[key];
+          if (key === samplingKey && !samplingPriorityWasSet) {
+            continue;
+          }
+          const value = keyValuePairs[key];
           this._tags.push({ key: key, value: value });
         }
       }
@@ -287,13 +286,18 @@ export default class Span {
    */
   _setSamplingPriority(priority: number): boolean {
     this._spanContext.finalizeSampling();
-    if (priority == 0) {
-      this._spanContext.flags = this._spanContext.flags & ~constants.SAMPLED_MASK;
-      return true;
-    } else if (this._tracer._isDebugAllowed(this._operationName)) {
-      this._spanContext.flags = this._spanContext.flags | constants.SAMPLED_MASK | constants.DEBUG_MASK;
-      return true;
+    if (priority > 0) {
+      if (this._spanContext.isDebug()) {
+        // If the span is already in debug, no need to set it again
+        return false;
+      }
+      if (this._tracer._isDebugAllowed(this._operationName)) {
+        this._spanContext.flags = this._spanContext.flags | constants.SAMPLED_MASK | constants.DEBUG_MASK;
+        return true;
+      }
+      return false;
     }
+    this._spanContext.flags = this._spanContext.flags & ~constants.SAMPLED_MASK;
     return false;
   }
 }

src/throttler/default_throttler.js

@@ -24,4 +24,12 @@ export default class DefaultThrottler {
   isAllowed(operation: string): boolean {
     return !this._throttleAll;
   }
+
+  setProcess(process: Process): void {
+    // NOP
+  }
+
+  close(callback?: Function): void {
+    // NOP
+  }
 }

src/throttler/remote_throttler.js

@@ -99,12 +99,14 @@ export default class RemoteThrottler {
     }
     // Credits for the operation will be asynchronously fetched
     this._credits[operation] = 0;
+    this._metrics.throttledDebugSpans.increment(1);
     return false;
   }
 
   _isAllowed(operation: string): boolean {
     const credits = this._credits[operation] || 0;
     if (credits < UNIT_CREDIT) {
+      this._metrics.throttledDebugSpans.increment(1);
       return false;
     }
     this._credits[operation] = credits - UNIT_CREDIT;
@@ -132,9 +134,9 @@ export default class RemoteThrottler {
 
   _fetchCredits(operations: any) {
     const serviceName: string = encodeURIComponent(this._serviceName);
-    const clientID: string = encodeURIComponent(this._uuid);
+    const uuid: string = encodeURIComponent(this._uuid);
     const ops: string = operations.map(encodeURIComponent).join('&operations=');
-    const url: string = `/credits?service=${serviceName}&clientID=${clientID}&operations=${ops}`;
+    const url: string = `/credits?service=${serviceName}&uuid=${uuid}&operations=${ops}`;
 
     const success: Function = body => {
       this._parseCreditResponse(body);

src/tracer.js

@@ -40,7 +40,7 @@ export default class Tracer {
   _extractors: any;
   _metrics: any;
   _baggageSetter: BaggageSetter;
-  _debugThrottler: Throttler;
+  _debugThrottler: Throttler & ProcessSetter;
   _process: Process;
 
   /**
@@ -107,9 +107,7 @@ export default class Tracer {
       tags: Utils.convertObjectToTags(this._tags),
       uuid: uuid,
     };
-    if (typeof this._debugThrottler.setProcess === 'function') {
-      this._debugThrottler.setProcess(this._process);
-    }
+    this._debugThrottler.setProcess(this._process);
     // TODO update reporter to implement ProcessSetter
     this._reporter.setProcess(this._process.serviceName, this._process.tags);
   }
@@ -330,6 +328,7 @@ export default class Tracer {
     reporter.close(() => {
       this._sampler.close(callback);
     });
+    this._debugThrottler.close();
   }
 
   /**

test/init_tracer.js

@@ -172,7 +172,7 @@ describe('initTracer', () => {
   });
 
   it('should initialize throttler from config', () => {
-    let config = {
+    const config = {
       serviceName: 'test-service',
       throttler: {
         refreshIntervalMs: 60000,
@@ -183,20 +183,20 @@ describe('initTracer', () => {
   });
 
   it('should delegate throttler initialization to tracer', () => {
-    let config = {
+    const config = {
       serviceName: 'test-service',
     };
     const tracer = initTracer(config);
     expect(tracer._debugThrottler).to.be.an.instanceof(DefaultThrottler);
   });
 
   it('should use throttler passed in via options', () => {
-    let config = {
+    const config = {
       serviceName: 'test-service',
     };
     const throttler = new RemoteThrottler();
     const tracer = initTracer(config, { throttler: throttler });
-    expect(tracer._debugThrottler).to.be.an.instanceof(RemoteThrottler);
+    expect(tracer._debugThrottler).to.equal(throttler);
     throttler.close();
   });
 });

test/span.js

@@ -170,11 +170,11 @@ describe('span should', () => {
     assert.isOk(unnormalizedKey in Span._getBaggageHeaderCache());
   });
 
-  it('not be set to debug if throttled', () => {
+  it('not be set to debug via setTag if throttled', () => {
     tracer._debugThrottler = new DefaultThrottler(true);
     span = new Span(tracer, 'op-name', spanContext, tracer.now());
 
-    let prevTagLength = span._tags.length;
+    const prevTagLength = span._tags.length;
     span.setTag(opentracing.Tags.SAMPLING_PRIORITY, 1);
     assert.isOk(span.context().samplingFinalized);
     assert.isNotOk(span.context().isDebug());
@@ -183,10 +183,13 @@ describe('span should', () => {
       span._tags.length,
       'The sampling.priority tag should not be set if throttled'
     );
+  });
 
+  it('not be set to debug via addTags if throttled', () => {
+    tracer._debugThrottler = new DefaultThrottler(true);
     span = new Span(tracer, 'op-name', spanContext, tracer.now());
 
-    prevTagLength = span._tags.length;
+    const prevTagLength = span._tags.length;
     const tags = {};
     tags[opentracing.Tags.SAMPLING_PRIORITY] = 1;
     span.addTags(tags);
@@ -199,6 +202,23 @@ describe('span should', () => {
     );
   });
 
+  it('ignore sampling.priority tag if span is already debug', () => {
+    tracer._debugThrottler = new DefaultThrottler();
+    const isAllowedSpy = sinon.spy(tracer._debugThrottler, 'isAllowed');
+    span = new Span(tracer, 'op-name', spanContext, tracer.now());
+
+    span.setTag(opentracing.Tags.SAMPLING_PRIORITY, 1);
+    assert.isOk(span.context().samplingFinalized);
+    assert.isOk(span.context().isDebug());
+    assert.deepEqual(span._tags[span._tags.length - 1], { key: 'sampling.priority', value: 1 });
+
+    const prevTagLength = span._tags.length;
+    span.setTag(opentracing.Tags.SAMPLING_PRIORITY, 1);
+    // isAllowed should only be called the first time the sampling.priority tag is set
+    sinon.assert.calledOnce(isAllowedSpy);
+    assert.equal(prevTagLength, span._tags.length, 'The sampling.priority tag should only be set once');
+  });
+
   describe('adaptive sampling tests for span', () => {
     let options = [
       { desc: 'sampled: ', sampling: true, reportedSpans: 1 },
@@ -245,13 +265,15 @@ describe('span should', () => {
         assert.isOk(span.context().samplingFinalized);
         assert.deepEqual(span._tags[span._tags.length - 1], { key: 'sampling.priority', value: 1 });
 
-        let unsampledSpan = tracer.startSpan('unsampled-span');
+        const unsampledSpan = tracer.startSpan('unsampled-span');
+        const prevTagLength = span._tags.length;
         unsampledSpan.setTag(opentracing.Tags.SAMPLING_PRIORITY, -1);
         assert.isOk(unsampledSpan.context().samplingFinalized);
-        assert.deepEqual(unsampledSpan._tags[unsampledSpan._tags.length - 1], {
-          key: 'sampling.priority',
-          value: -1,
-        });
+        assert.equal(
+          prevTagLength,
+          span._tags.length,
+          'The sampling.priority tag should not be set if span is finalized and not sampled'
+        );
       });
 
       it('should trigger on a finish()-ed span', () => {

test/throttler/default_throttler.js

@@ -23,12 +23,16 @@ import DefaultThrottler from '../../src/throttler/default_throttler';
 
 describe('DefaultThrottler should', () => {
   it('throttle everything', () => {
-    let throttler = new DefaultThrottler(true);
+    const throttler = new DefaultThrottler(true);
+    throttler.setProcess({});
     assert.isNotOk(throttler.isAllowed('key'));
+    throttler.close();
   });
 
   it('throttle nothing', () => {
-    let throttler = new DefaultThrottler();
+    const throttler = new DefaultThrottler();
+    throttler.setProcess({});
     assert.isOk(throttler.isAllowed('key'));
+    throttler.close();
   });
 });

test/throttler/remote_throttler.js

@@ -71,6 +71,7 @@ describe('RemoteThrottler should', () => {
       assert.isOk(_throttler.isAllowed(operation));
       assert.equal(_throttler._credits[operation], 2);
       assert.equal(LocalBackend.counterValue(metrics.throttlerUpdateSuccess), 1);
+      assert.equal(LocalBackend.counterValue(metrics.throttledDebugSpans), 1);
       done();
     };
     assert.isNotOk(throttler.isAllowed(operation));
@@ -99,11 +100,13 @@ describe('RemoteThrottler should', () => {
     );
     throttler._credits[operation] = 0;
     assert.isNotOk(throttler._isAllowed(operation), 'operation is set but lacks credit');
+    assert.equal(LocalBackend.counterValue(metrics.throttledDebugSpans), 2);
   });
 
   it("return false for isAllowed if operation doesn't have enough credits", () => {
     throttler._credits[operation] = 0.5;
     assert.isNotOk(throttler._isAllowed(operation));
+    assert.equal(LocalBackend.counterValue(metrics.throttledDebugSpans), 1);
   });
 
   it('succeed when we retrieve credits for multiple operations', done => {

test/tracer.js

@@ -256,6 +256,16 @@ describe('tracer should', () => {
     sinon.assert.calledWith(throttler.setProcess, tracer._process);
   });
 
+  it('close _debugThrottler on close', () => {
+    const throttler = new DefaultThrottler();
+    throttler.close = sinon.spy();
+    tracer = new Tracer('x', reporter, new ConstSampler(true), {
+      debugThrottler: throttler,
+    });
+    tracer.close();
+    sinon.assert.calledOnce(throttler.close);
+  });
+
   describe('Metrics', () => {
     it('startSpan', () => {
       let params = [