Use node-uuid.v4()

[suryatech]

As of now, the traceIds and spanIds are generated using the function,

Math.floor(Math.random() * largestRandom);

Can we move to UUID (v4) instead?

[cyli]

I certainly wouldn't mind, and I don't see any issues with it - it shouldn't break anything. Just want to check that I'm not missing anything, and that others would be ok with this too? @wadey and @dustyburwell maybe?

[cyli]

Ack, wait, sorry, I spoke too soon. Looks like zipkin expects the IDs to be a 64-bits. A uuid4 would be 124-bit - unless you just meant generate a UUID v4, convert to an int, and then truncate to 64-bits?

[suryatech]

@cyli We can truncate it to 64 bits. The overall aim is to minimise the collisions that the above function results in.

The other issue we are going to have is the number limitation in JavaScript if we convert the 16 character (64 bit) hex to an int (loses precision). To overcome this, we should completely eliminate this conversion process and use strings everywhere.

What do you think?

[wadey]

I would recommend passing strings around and use a simple method like this to generate the IDs:

var b = Buffer(8)
function random64Hex() {
    b.writeUInt32BE(Math.random() * 0x100000000, 0)
    b.writeUInt32BE(Math.random() * 0x100000000, 4)
    return b.toString('hex')
}

[cyli]

@wadey That seems to work - they get sent as hex-encoded header values anyway... Would that be ok @suryatech?

[suryatech]

@wadey The provided method works great with 0 collisions in 10 million hex strings.

@cyli I need to verify if zipkin receives the same traceId that this function generates. Will update soon.

[suryatech]

@cyli I tried using the generated hex by overriding the fromHeaders method of the Trace object. Everything works fine until the serialisation process begins.

I believe the serialisation process expects the trace/span/parent - IDs to be integers and if a string is passed, it fails to serialise correctly.

The trace information is sent to Zipkin but with a traceId of (0x0).

[suryatech]

@cyli

encodeInt function in binary_parser.js inside the thrift library look like so,

p.encodeInt = function( data, bits, signed, forceBigEndian ){
  var max = Math.pow( 2, bits );
  ( data >= max || data < -( max / 2 ) ) && this.warn( "encodeInt::overflow" ) && ( data = 0 );
  data < 0 && ( data += max );
  for( var r = []; data; r[r.length] = data % 256, data = Math.floor( data / 256 ) );
  for( bits = -( -bits >> 3 ) - r.length; bits--; r[r.length] = 0 );
        return new Buffer((this.bigEndian||forceBigEndian) ? r.reverse() : r );
};

So the data argument needs to be an integer for this to work.

What are your thoughts on this?

[wadey]

I forgot this had to go over thrift, and I wrote node-thrift so I can give some help here. node-thrift 0.9.2 (which node-tryfer master depends on) uses the node-int64 module, so we can pass the IDs around as Int64 objects instead of strings, like so:

var Int64 = require('node-int64')

function randomInt64() {
    return new Int64(Math.random() * 0x100000000, Math.random() * 0x100000000)
}

Example:

> i = randomInt64()
[Int64 value:-Infinity octets:94 54 bb a7 f2 56 dc db]
> i.buffer
<Buffer 94 54 bb a7 f2 56 dc db>
> i.toOctetString()
'9454bba7f256dcdb'

To construct an Int64 from a string, you just pass the string to the constructor:

> i = new Int64('9454bba7f256dcdb')
[Int64 value:-Infinity octets:94 54 bb a7 f2 56 dc db]

node-thrift checks for the .buffer attribute and serializes it correctly: https://github.com/apache/thrift/blob/0.9.2/lib/nodejs/lib/thrift/protocol.js#L130-L131

So as long as we give thrift the Int64 object, all should work well.

[suryatech]

@wadey Thanks for the explanation. I had the older version of node-tryfer (0.2.8) which was using the 0.7.0 version of node-thrift.

Upgraded the tryfer module to the latest version (0.2.10) and modified the random generator to randomInt64 and everything works as expected.

[suryatech]

@cyli Can we modify the default behaviour of the module and change the getUniqueId function in trace.js ? I am willing to send in a PR btw.

https://github.com/racker/node-tryfer/blob/master/lib/trace.js#L29-L31

[cyli]

@suryatech That seems fine to me :) Thanks!