Do not apply adaptive sampler to child spans (#410)

* Do not apply adaptive sampler to child spans Signed-off-by: Yuri Shkuro <[email protected]> * Add call to setOperation Signed-off-by: Yuri Shkuro <[email protected]> * Few more tests Signed-off-by: Yuri Shkuro <[email protected]> * Rename vars Signed-off-by: Yuri Shkuro <[email protected]> * Fix tests Signed-off-by: Yuri Shkuro <[email protected]>
jaegertracing · Oct 22, 2019 · f94a31d · f94a31d
1 parent 865ac99
commit f94a31d
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 3 deletions.
diff --git a/src/samplers/per_operation_sampler.js b/src/samplers/per_operation_sampler.js
@@ -92,14 +92,21 @@ export default class PerOperationSampler implements Sampler {
 
   onCreateSpan(span: Span): SamplingDecision {
     const outTags = {};
-    const isSampled = this.isSampled(span.operationName, outTags);
-    // NB: return retryable=true here since we can change decision after setOperationName().
+    let isSampled = false;
+    if (span.context()._samplingState.isLocalRootSpan(span.context())) {
+      isSampled = this.isSampled(span.operationName, outTags);
+    }
+    // returning retryable=true since we can change the sampling decision
+    // after the first call to setOperationName()
     return { sample: isSampled, retryable: true, tags: outTags };
   }
 
   onSetOperationName(span: Span, operationName: string): SamplingDecision {
     const outTags = {};
-    const isSampled = this.isSampled(span.operationName, outTags);
+    let isSampled = false;
+    if (span.context()._samplingState.isLocalRootSpan(span.context())) {
+      isSampled = this.isSampled(span.operationName, outTags);
+    }
     return { sample: isSampled, retryable: false, tags: outTags };
   }
 

diff --git a/test/samplers/remote_sampler.js b/test/samplers/remote_sampler.js
@@ -21,6 +21,8 @@ import MockLogger from '../lib/mock_logger';
 import ConfigServer from '../lib/config_server';
 import LocalMetricFactory from '../lib/metrics/local/metric_factory.js';
 import LocalBackend from '../lib/metrics/local/backend.js';
+import Tracer from '../../src/tracer.js';
+import NoopReporter from '../../src/reporters/noop_reporter.js';
 
 describe('RemoteSampler', () => {
   let server: ConfigServer;
@@ -192,6 +194,57 @@ describe('RemoteSampler', () => {
     remoteSampler._refreshSamplingStrategy();
   });
 
+  it('should not use per-operation sampler on child spans', done => {
+    server.addStrategy('service1', {
+      strategyType: 'PROBABILISTIC',
+      probabilisticSampling: {
+        samplingRate: 0.0,
+      },
+      operationSampling: {
+        defaultSamplingProbability: 0.05,
+        defaultLowerBoundTracesPerSecond: 0.1,
+        perOperationStrategies: [
+          {
+            operation: 'op1',
+            probabilisticSampling: { samplingRate: 0.0 },
+          },
+          {
+            operation: 'op2',
+            probabilisticSampling: { samplingRate: 1.0 },
+          },
+        ],
+      },
+    });
+    remoteSampler._onSamplerUpdate = s => {
+      let tracer = new Tracer('service', new NoopReporter(), s);
+
+      let sp0 = tracer.startSpan('op2');
+      assert.isTrue(sp0.context().isSampled(), 'op2 should be sampled on the root span');
+
+      let sp1 = tracer.startSpan('op1');
+      assert.isFalse(sp1.context().isSampled(), 'op1 should not be sampled');
+      sp1.setOperationName('op2');
+      assert.isTrue(sp1.context().isSampled(), 'op2 should be sampled on the root span');
+
+      let parent = tracer.startSpan('op1');
+      assert.isFalse(parent.context().isSampled(), 'parent span should not be sampled');
+      assert.isFalse(parent.context().samplingFinalized, 'parent span should not be finalized');
+
+      let child = tracer.startSpan('op2', { childOf: parent });
+      assert.isFalse(child.context().isSampled(), 'child span should not be sampled even with op2');
+      assert.isFalse(child.context().samplingFinalized, 'child span should not be finalized');
+      child.setOperationName('op2');
+      assert.isFalse(child.context().isSampled(), 'op2 should not be sampled on the child span');
+      assert.isTrue(
+        child.context().samplingFinalized,
+        'child span should be finalized after setOperationName'
+      );
+
+      done();
+    };
+    remoteSampler._refreshSamplingStrategy();
+  });
+
   it('should refresh periodically', done => {
     server.addStrategy('service1', {
       strategyType: 'PROBABILISTIC',