Faster trace id generation

[dyladan]

Replaces #786

While working on #786 we found that most of the performance benefit came from a more efficient method of generating trace ids. This PR implements the more efficient method while allowing us to continue using the simpler string implementation for trace and span ids.

/cc @Flarna

[codecov-io]

Codecov Report

Merging #824 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #824      +/-   ##
==========================================
+ Coverage   94.11%   94.13%   +0.01%     
==========================================
  Files         247      240       -7     
  Lines       10762    10142     -620     
  Branches     1049      963      -86     
==========================================
- Hits        10129     9547     -582     
+ Misses        633      595      -38

Impacted Files	Coverage Δ
packages/opentelemetry-plugin-dns/src/version.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-resources/src/constants.ts	100% <0%> (ø)	⬆️
...ges/opentelemetry-exporter-zipkin/src/transform.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-plugin-redis/src/version.ts	100% <0%> (ø)	⬆️
...lemetry-resources/test/resource-assertions.test.ts	100% <0%> (ø)	⬆️
...ages/opentelemetry-resources/test/Resource.test.ts	100% <0%> (ø)	⬆️
packages/opentelemetry-core/src/utils/url.ts	100% <0%> (ø)	⬆️
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts	100% <0%> (ø)	⬆️
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts	100% <0%> (ø)	⬆️
...in-postgres/opentelemetry-plugin-pg/src/version.ts	100% <0%> (ø)	⬆️
... and 23 more

[OlivierAlbertini]

It would be nice to have some benchmarks (numbers)
on the "crypto.randomBytes(TRACE_ID_BYTES).toString('hex')" vs uuid() vs "original implementation"
just for communicating those changes in a better way

[dyladan]

It would be nice to have some benchmarks (numbers)
on the "crypto.randomBytes(TRACE_ID_BYTES).toString('hex')" vs uuid() vs "original implementation"
just for communicating those changes in a better way

UUID only generate 16 byte random values, introduces a dependency, and returns strings containing dashes which need to be stripped, but I have included it here for completeness.

'use strict';

const benchmark = require('./benchmark');
const crypto = require('crypto');
const uuid = require("uuid");


const suite = benchmark(100)
    .add('return start.toString() + end.toString()', function () {
        const start = crypto.randomBytes(8);
        const end = crypto.randomBytes(8);
        return start.toString() + end.toString()
    })
    .add('return input.toString("hex")', function () {
        const input = crypto.randomBytes(16);
        return input.toString('hex')
    })
    .add('uuid', function () {
        return uuid.v4().replace(/-/g, '')
    })

// run async
suite.run({ async: false });

  return start.toString() + end.toString() x 231,749 ops/sec ±0.96% (100 runs sampled)
  return input.toString("hex")             x 442,410 ops/sec ±0.94% (100 runs sampled)
  uuid                                     x 381,162 ops/sec ±0.69% (100 runs sampled)

method	time (µs)	slowdown (µs)
8 byte to hex + 8 byte to hex	4.315	2.055 (91%)
16 byte to hex	2.26
uuid	2.624	0.364 (16%)

[obecny]

I think the function for browser is still slow and this might be improved
I have did some research and I have compared this solution with 3 others
here is jsperf

https://jsperf.com/convert-numeric-array-to-hex-string/11

the function toHex2 seems to be the fastest (for our purposes) and it is really visible difference ( more then 7 times faster)

function toHex2 (byteArray) {
    const chars = new Uint8Array(byteArray.length * 2);
    const alpha = 'a'.charCodeAt(0) - 10;
    const digit = '0'.charCodeAt(0);
  
    let p = 0;
    for (let i = 0; i < byteArray.length; i++) {
      let nibble = (byteArray[i] >>> 4) & 0xF;
      chars[p++] = nibble > 9 ? nibble + alpha : nibble + digit;
      nibble = byteArray[i] & 0xF;
      chars[p++] = nibble > 9 ? nibble + alpha : nibble + digit;    
    }
    
    return String.fromCharCode.apply(null, chars);
  }

the mentioned jsperf is modification and the origin comes from this discussion
https://stackoverflow.com/questions/34309988/byte-array-to-hex-string-conversion-in-javascript

[dyladan]

Wow @obecny that is a big difference. The only concern I have is that this is lowercase only. I know the trace context spec states that the ids will be lowercase, but I think it is not a bad idea to be resilient to bad inputs where the case is the only problem.

edit: just realized i'm being stupid. We're generating the characters of course we can make them lowercase only. i'll apply this today

[dyladan]

@obecny applied your change

[obecny]

lgtm, if you can please add just simple unit test for function toHex.

[dyladan]

lgtm, if you can please add just simple unit test for function toHex.

That would require us to export the toHex function, but I don't think we should as it is an internal detail. As long as this test passes, we know ids are being generated correctly: https://github.com/open-telemetry/opentelemetry-js/blob/cf93906a9b736bd15d1f094ec803eae67c17bdde/packages/opentelemetry-core/test/platform/id.test.ts

I expanded it in f539c99 in order to make sure the ids we generate are robust, which you can see here: https://github.com/open-telemetry/opentelemetry-js/blob/f539c9939d19d67a494fe62a81872c9572179838/packages/opentelemetry-core/test/platform/id.test.ts