fix: change span ID to use random bytes (googleapis#654)

PR-URL: googleapis#654

While running a test application that was composed of multiple Node.js apps, each with tracing enabled, we found that span IDs would conflict when the multiple apps were simultaneously started, because each started their span IDs at index 1. This switches to use secure randomness for span IDs to help ensure uniqueness even when multiple Node apps are started at the same time.

src/span-data.ts

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import * as crypto from 'crypto';
 import * as util from 'util';
 
 import {Constants} from './constants';
@@ -42,8 +43,22 @@ interface StackFrame {
   column_number?: number;
 }
 
-// Auto-incrementing integer
-let uid = 1;
+// Use 6 bytes of randomness only as JS numbers are doubles not 64-bit ints.
+const SPAN_ID_RANDOM_BYTES = 6;
+
+// Use the faster crypto.randomFillSync when available (Node 7+) falling back to
+// using crypto.randomBytes.
+const spanIdBuffer = Buffer.alloc(SPAN_ID_RANDOM_BYTES);
+const randomFillSync = crypto.randomFillSync;
+const randomBytes = crypto.randomBytes;
+const spanRandomBuffer = randomFillSync ?
+    () => randomFillSync(spanIdBuffer) :
+    () => randomBytes(SPAN_ID_RANDOM_BYTES);
+
+function randomSpanId() {
+  // tslint:disable-next-line:ban Needed to parse hexadecimal.
+  return parseInt(spanRandomBuffer().toString('hex'), 16).toString();
+}
 
 export class SpanData implements SpanDataInterface {
   readonly span: TraceSpan;
@@ -60,10 +75,9 @@ export class SpanData implements SpanDataInterface {
   constructor(
       readonly trace: Trace, spanName: string, parentSpanId: string,
       private readonly isRoot: boolean, skipFrames: number) {
-    const spanId = '' + (uid++);
     spanName =
         traceUtil.truncate(spanName, Constants.TRACE_SERVICE_SPAN_NAME_LIMIT);
-    this.span = new TraceSpan(spanName, spanId, parentSpanId);
+    this.span = new TraceSpan(spanName, randomSpanId(), parentSpanId);
     trace.spans.push(this.span);
     if (traceWriter.get().getConfig().stackTraceLimit > 0) {
       // This is a mechanism to get the structured stack trace out of V8.

test/test-span-data.ts

@@ -56,6 +56,26 @@ describe('SpanData', function() {
     });
   });
 
+  it('generates unique numeric span ID strings', function() {
+    cls.getNamespace().run(function() {
+      var spans = [];
+      var spanIds = new Set<string>();
+      var rootSpanData = createRootSpanData('hi');
+      var numSpanIdsToCheck = 5;
+      for (var i = 0; i < numSpanIdsToCheck; i++) {
+        var spanData = new SpanData(
+            rootSpanData.trace, 'child', rootSpanData.span.spanId, false, 0);
+        var spanId = spanData.span.spanId;
+        spanIds.add(spanId);
+        // Check that the span IDs are numeric positive number strings
+        assert.ok(typeof spanId === 'string');
+        assert.ok(spanId.match(/\d+/));
+        assert.ok(Number(spanId) > 0);
+        assert.strictEqual(Number(spanId).toString(), spanId);
+      }
+    });
+  });
+
   it('converts label values to strings', function() {
     cls.getNamespace().run(function() {
       var spanData = createRootSpanData('name', 1, 2);

test/test-trace-header-context.ts

@@ -101,34 +101,44 @@ describe('test-trace-header-context', function() {
   it('should parse incoming header', function(done) {
     var app = express();
     var server;
-    var context = '123456/2;o=1';
-    app.get('/', function (req, res) {
-      http.get({ port: common.serverPort, path: '/self'});
+    var sentTraceId = '0000000000000000000000000000000a';
+    var sentSpanId = '2';
+    var sentTraceOptions = 'o=1';
+    var sentTraceContext = `${sentTraceId}/${sentSpanId};${sentTraceOptions}`;
+    app.get('/', function(req, res) {
+      http.get({port: common.serverPort, path: '/self'});
       res.send(common.serverRes);
     });
     app.get('/self', function(req, res) {
-      assert.equal(
-        req.headers[Constants.TRACE_CONTEXT_HEADER_NAME].slice(0, 6),
-        context.slice(0, 6));
-      assert.equal(
-        req.headers[Constants.TRACE_CONTEXT_HEADER_NAME].slice(8),
-        ';o=1');
+      var receivedTraceContext =
+          req.headers[Constants.TRACE_CONTEXT_HEADER_NAME];
+      var receivedTraceId = receivedTraceContext.split('/')[0];
+      var receivedSpanIdAndOptions =
+          receivedTraceContext.split('/')[1].split(';');
+      var receivedSpanId = receivedSpanIdAndOptions[0];
+      var receivedTraceOptions = receivedSpanIdAndOptions[1];
+      // Trace ID and trace options should be the same in sender and receiver.
+      assert.strictEqual(receivedTraceId, sentTraceId);
+      assert.strictEqual(receivedTraceOptions, sentTraceOptions);
+      // Span ID should be different as receiver generates a new span ID.
+      assert.notStrictEqual(receivedSpanId, sentSpanId);
+
       res.send(common.serverRes);
       var traces = common.getTraces();
-      assert.equal(traces.length, 2);
-      assert.equal(traces[0].spans.length, 2);
-      assert.equal(traces[1].spans.length, 1);
-      assert.equal(traces[0].spans[0].name, '/');
-      assert.equal(traces[0].spans[1].name, 'localhost');
-      assert.equal(traces[1].spans[0].name, '/self');
+      assert.strictEqual(traces.length, 2);
+      assert.strictEqual(traces[0].spans.length, 2);
+      assert.strictEqual(traces[1].spans.length, 1);
+      assert.strictEqual(traces[0].spans[0].name, '/');
+      assert.strictEqual(traces[0].spans[1].name, 'localhost');
+      assert.strictEqual(traces[1].spans[0].name, '/self');
       common.cleanTraces();
       server.close();
       done();
     });
     server = app.listen(common.serverPort, function() {
       var headers = {};
-      headers[Constants.TRACE_CONTEXT_HEADER_NAME] = context;
-      http.get({ port: common.serverPort, headers: headers });
+      headers[Constants.TRACE_CONTEXT_HEADER_NAME] = sentTraceContext;
+      http.get({port: common.serverPort, headers: headers});
     });
   });
 });