fix: sampler now samples correctly (#100)

Sampler was not adhering to the configured propability when not ALWAYS or
NEVER. Issue was that the Sampler was using Number.MAX_VALUE, which uses
1024-bits. There were two issues with this approach:

1. The MAX_VALUE is NOT considered a "safe integer" and thus there are side
   effects during manipulation.
2. The `shouldSample` method was only comparing the idUpperBound (based on
   1024-bits of data) to the lower 16 characters (64-bits) of a trace, which
   was resulting in the configured probabilty not being respected.

We now us a 52-bit number, which falls within the "safe integer" range (53-bits)
and is able to be fully represented by a 13-diget hex value. This allows the
value to:

1. Be safely algorithmically manipulated without side effects.
2. Be easily compared to the lower 52-bits (13 characters) of a traceId.

The tests have been updated to perform validation of traceIds that should and
should not be sampled given a probability.

Author: justindsmith

packages/opencensus-core/src/trace/config/types.ts

@@ -31,7 +31,7 @@ export interface BufferConfig {
 
 /** Defines tracer configuration parameters */
 export interface TracerConfig {
-  /** Determines the samplin rate. Ranges from 0.0 to 1.0 */
+  /** Determines the sampling rate. Ranges from 0.0 to 1.0 */
   samplingRate?: number;
   /** Determines the ignored (or blacklisted) URLs */
   ignoreUrls?: Array<string|RegExp>;

packages/opencensus-core/src/trace/sampler/sampler.ts

@@ -17,10 +17,11 @@
 import {randomSpanId} from '../../internal/util';
 import {Sampler} from './types';
 
-
-const MIN_NUMBER = Number.MIN_VALUE;
-const MAX_NUMBER = Number.MAX_VALUE;
-
+// We use 52-bits as our max number because it remains a javascript "safe
+// integer" for arithmetic and parsing while using the full hex range for
+// comparison to the lower order bytes on a traceId.
+const MAX_NUMBER = 0xfffffffffffff;
+const LOWER_BYTE_COUNT = 13;
 
 /**  Sampler that samples every trace. */
 export class AlwaysSampler implements Sampler {
@@ -61,7 +62,8 @@ export class ProbabilitySampler implements Sampler {
    * False if the traceId is not in probability.
    */
   shouldSample(traceId: string): boolean {
-    const LOWER_BYTES = traceId ? traceId.substring(16) : '0';
+    const LOWER_BYTES =
+        traceId ? ('0000000000000' + traceId).slice(-LOWER_BYTE_COUNT) : '0';
     // tslint:disable-next-line:ban Needed to parse hexadecimal.
     const LOWER_LONG = parseInt(LOWER_BYTES, 16);
 

packages/opencensus-core/test/test-sampler.ts

@@ -31,16 +31,16 @@ describe('Sampler', () => {
     it('should return a always sampler for 1', () => {
       const root = new RootSpan(tracer);
       const sampler = SamplerBuilder.getSampler(1);
-      const samplerShouldSampler = sampler.shouldSample(root.traceId);
+      const samplerShouldSample = sampler.shouldSample(root.traceId);
       assert.strictEqual(sampler.description, 'always');
-      assert.ok(samplerShouldSampler);
+      assert.ok(samplerShouldSample);
     });
     it('should return a always sampler for >1', () => {
       const root = new RootSpan(tracer);
       const sampler = SamplerBuilder.getSampler(100);
-      const samplerShouldSampler = sampler.shouldSample(root.traceId);
+      const samplerShouldSample = sampler.shouldSample(root.traceId);
       assert.strictEqual(sampler.description, 'always');
-      assert.ok(samplerShouldSampler);
+      assert.ok(samplerShouldSample);
     });
   });
   /**
@@ -50,26 +50,56 @@ describe('Sampler', () => {
     it('should return a never sampler for 0', () => {
       const root = new RootSpan(tracer);
       const sampler = SamplerBuilder.getSampler(0);
-      const samplerShouldSampler = sampler.shouldSample(root.traceId);
+      const samplerShouldSample = sampler.shouldSample(root.traceId);
       assert.strictEqual(sampler.description, 'never');
-      assert.ok(!samplerShouldSampler);
+      assert.ok(!samplerShouldSample);
     });
     it('should return a never sampler for negative value', () => {
       const root = new RootSpan(tracer);
       const sampler = SamplerBuilder.getSampler(-1);
-      const samplerShouldSampler = sampler.shouldSample(root.traceId);
+      const samplerShouldSample = sampler.shouldSample(root.traceId);
       assert.strictEqual(sampler.description, 'never');
-      assert.ok(!samplerShouldSampler);
+      assert.ok(!samplerShouldSample);
     });
   });
 
   describe('shouldSample() probability', () => {
     it('should return a probability sampler', () => {
-      const root = new RootSpan(tracer);
       const sampler = SamplerBuilder.getSampler(0.7);
       assert.ok(sampler.description.indexOf('probability') >= 0);
-      const samplerShouldSampler = sampler.shouldSample(root.traceId);
-      assert.ok(samplerShouldSampler ? samplerShouldSampler : true);
     });
+    it('should sample an empty traceId', () => {
+      const sampler = SamplerBuilder.getSampler(0.5);
+      const samplerShouldSample = sampler.shouldSample(null);
+      assert.ok(samplerShouldSample);
+    });
+    it('should accept and reject traces based on last 26 bytes of traceId',
+       () => {
+         const sampler = SamplerBuilder.getSampler(0.5);
+
+         const shouldSample = [
+           '11111111111111111110000000000000',
+           '1111111111111111111000ffffffffff',
+           '11111111111111111117ffffffffffff',
+         ];
+         shouldSample.forEach(traceId => {
+           const samplerShouldSample = sampler.shouldSample(traceId);
+           assert.ok(
+               samplerShouldSample,
+               `should have sampled but didn't: ${traceId}`);
+         });
+
+         const shouldNotSample = [
+           '11111111111111111118000000000000',
+           '11111111111111111118000fffffffff',
+           '1111111111111111111fffffffffffff',
+         ];
+         shouldNotSample.forEach(traceId => {
+           const samplerShouldSample = sampler.shouldSample(traceId);
+           assert.ok(
+               !samplerShouldSample,
+               `should not have sampled but did: ${traceId}`);
+         });
+       });
   });
 });

packages/opencensus-exporter-zpages/test/test-zpages.ts

@@ -36,7 +36,7 @@ const zpagesServerUrl = 'http://localhost:' + options.port;
 
 /** Default config for traces tests */
 const defaultConfig: TracerConfig = {
-  samplingRate: 0.2
+  samplingRate: 1.0
 };
 
 /**