Address feedback

jaegertracing · Apr 4, 2017 · 01239e0 · 01239e0
1 parent 0aa3cad
commit 01239e0
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 33 deletions.
diff --git a/src/span.js b/src/span.js
@@ -20,7 +20,6 @@
 // THE SOFTWARE.
 
 import * as constants from './constants.js';
-import NullLogger from './logger.js';
 import SpanContext from './span_context.js';
 import * as opentracing from 'opentracing';
 import Utils from './util.js';
@@ -118,12 +117,21 @@ export default class Span {
     }
 
     /**
-     * Returns the span context that represents this span.
+     * Finalizes the span and returns the span context that represents this span.
      *
      * @return {SpanContext} - Returns this span's span context.
      **/
     context(): SpanContext {
-        return this._spanContext;
+        let ctx = this._spanContext;
+        if (!ctx.samplingFinalized) {
+            if (ctx.isDebug() || this._tracer._sampler.isSampled(this.operationName, this._tags)) {
+                ctx._flags |= constants.SAMPLED_MASK
+            } else {
+                ctx._flags &= ~constants.SAMPLED_MASK
+            }
+            ctx.finalizeSampling()
+        }
+        return ctx;
     }
 
     /**

diff --git a/src/tracer.js b/src/tracer.js
@@ -118,7 +118,7 @@ export default class Tracer {
 
         // emit metrics
         this._metrics.spansStarted.increment(1);
-        if (span.context().isSampled()) {
+        if (spanContext.isSampled()) {
             this._metrics.spansSampled.increment(1);
             if (!hadParent) {
                 this._metrics.tracesStartedSampled.increment(1);
@@ -228,15 +228,6 @@ export default class Tracer {
             ctx.parentId = null;
             ctx.flags = flags;
         } else {
-            if (!parent.samplingFinalized) {
-                if (this._sampler.isSampled(operationName, internalTags)) {
-                   parent._flags |= constants.SAMPLED_MASK
-                } else {
-                    parent._flags &= ~constants.SAMPLED_MASK
-                }
-                parent.finalizeSampling()
-            }
-
             ctx.traceId = parent.traceId;
             ctx.spanId = Utils.getRandom64();
             ctx.parentId = parent.spanId;

diff --git a/test/span.js b/test/span.js
@@ -195,26 +195,24 @@ describe('span should', () => {
 
         it('should make a call to the underlying sampler and use the sampling decision when true', () => {
             let mockSampler = sinon.mock(tracer._sampler);
-            mockSampler.expects('isSampled').withExactArgs('goodOperation', {}).returns(true);
+            mockSampler.expects('isSampled').withExactArgs('op-name', []).returns(true);
             let child = tracer.startSpan('goodOperation', {childOf: span.context()});
             mockSampler.verify();
             assert.isOk(child.context().isSampled())
         });
 
         it('should make a call to the underlying sampler and use the sampling decision when false', () => {
             let mockSampler = sinon.mock(tracer._sampler);
-            mockSampler.expects('isSampled').withExactArgs('horridOperation', {}).returns(false);
+            mockSampler.expects('isSampled').withExactArgs('op-name', []).returns(false);
             let child = tracer.startSpan('horridOperation', {childOf: span.context()});
             mockSampler.verify();
             assert.isNotOk(child.context().isSampled())
         });
 
         it('should make the same sampling decision for all children', () => {
             let mockSampler = sinon.mock(tracer._sampler);
+            mockSampler.expects('isSampled').withExactArgs('op-name', []).returns(false);
             let parent = span.context();
-            mockSampler.expects('isSampled').withExactArgs('op1', {}).returns(false);
-            mockSampler.expects('isSampled').withExactArgs('op2', {}).throws();
-            mockSampler.expects('isSampled').withExactArgs('op3', {}).throws();
 
             let child1 = tracer.startSpan('op1', {childOf: parent});
             let child2 = tracer.startSpan('op2', {childOf: parent});
@@ -261,18 +259,18 @@ describe('span should', () => {
         });
 
         describe('span sampling finalizer', () => {
+            it ('should not finalize span unless triggered', () => {
+                assert.equal(span._spanContext.samplingFinalized, false);
+            });
+
             it ('should trigger when it inherits a sampling decision', () => {
-                assert.equal(span.context().samplingFinalized, false, 'Span created in before each is not finalized');
 
                 let childSpan = tracer.startSpan('child-span', {childOf: span});
                 assert.isOk(span.context().samplingFinalized);
                 assert.isOk(childSpan.context().samplingFinalized);
             });
 
             it ('should trigger when it sets the sampling priority', () => {
-                // Span created in before each is not finalized.
-                assert.equal(span.context().samplingFinalized, false);
-
                 span.setTag(opentracing.Tags.SAMPLING_PRIORITY, 1);
                 assert.isOk(span.context().samplingFinalized);
 
@@ -282,25 +280,16 @@ describe('span should', () => {
             });
 
             it ('should trigger on a finish()-ed span', () => {
-                // Span created in before each is not finalized.
-                assert.equal(span.context().samplingFinalized, false);
-
                 span.finish();
                 assert.isOk(span.context().samplingFinalized);
             });
 
             it ('should trigger after calling setOperationName', () => {
-                // Span created in before each is not finalized.
-                assert.equal(span.context().samplingFinalized, false);
-
                 span.setOperationName('fry');
                 assert.isOk(span.context().samplingFinalized);
             });
 
             it ('should trigger when its context is injected into headers', () => {
-                // Span created in before each is not finalized.
-                assert.equal(span.context().samplingFinalized, false);
-
                 let headers = {};
                 tracer.inject(span.context(), opentracing.FORMAT_HTTP_HEADERS, headers);
 
@@ -316,7 +305,7 @@ describe('span should', () => {
                 { logger: new MockLogger() }
             );
             let unFinalizedSpan = tracer.startSpan('unFinalizedSpan');
-            assert.equal(unFinalizedSpan.context().samplingFinalized, false);
+            assert.equal(unFinalizedSpan._spanContext.samplingFinalized, false);
             assert.isOk(unFinalizedSpan._isWriteable());
 
             tracer._sampler = new ConstSampler(true);

diff --git a/test/tracer.js b/test/tracer.js
@@ -62,7 +62,7 @@ describe('tracer should', () => {
 
         assert.isOk(rootSpan.context().traceId);
         assert.isNotOk(rootSpan.context().parentId);
-        assert.equal(rootSpan.context().flags, constants.SAMPLED_MASK | constants.DEFERRED_SAMPLING_MASK);
+        assert.equal(rootSpan.context().flags, constants.SAMPLED_MASK);
         assert.equal('Bender', rootSpan.getBaggageItem('robot'));
         assert.equal('Leela', rootSpan.getBaggageItem('female'));
         assert.equal('Fry', rootSpan.getBaggageItem('male'));