Adding 128 bit traceId support #361

PaulMiami · 2019-04-26T22:24:13Z

Signed-off-by: Paul Biccherai [email protected]

Which problem is this PR solving?

Resolves #295

Short description of the changes

Adding 128 bit support to the traceId

Signed-off-by: Paul Biccherai <[email protected]>

codecov · 2019-04-26T23:38:24Z

Codecov Report

Merging #361 into master will decrease coverage by 0.17%.
The diff coverage is 91.11%.

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   98.82%   98.65%   -0.18%     
==========================================
  Files          50       50              
  Lines        1965     2003      +38     
  Branches      366      374       +8     
==========================================
+ Hits         1942     1976      +34     
- Misses         23       27       +4

Impacted Files	Coverage Δ
src/reporters/udp_sender.js	`98.7% <100%> (+0.01%)`	⬆️
src/span_context.js	`93.28% <100%> (+0.26%)`	⬆️
src/tracer.js	`100% <100%> (ø)`	⬆️
src/thrift.js	`100% <100%> (ø)`	⬆️
src/util.js	`95.12% <80.95%> (-4.88%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4aa902...fb29d69. Read the comment docs.

mwieczorek · 2019-05-09T12:55:57Z

@PaulMiami Any news on resolving the Travis errors?
Sorry for bothering but I'm also interested in this feature.

PaulMiami · 2019-05-09T15:12:19Z

@PaulMiami Any news on resolving the Travis errors?
Sorry for bothering but I'm also interested in this feature.

@mwieczorek I am not sure if I can fix that one on my own, travis only fails trying to run the tests with node 12. I haven't looked at it extensively but it seems that it won't work until node 12 is more stable.

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-05-09T15:42:07Z

I am not sure if it's acceptable but I have changed the travis config to build node lts instead of latest, which is unstable at this time

yurishkuro · 2019-05-09T16:04:55Z

.travis.yml

@@ -21,6 +21,8 @@ matrix:
      - docker
  - env: TEST_NODE_VERSION=6 LINT=1 COVER=1
  - env: TEST_NODE_VERSION=4
+  - env: TEST_NODE_VERSION=--lts
+  allow_failures:


what is this for, and it is indented correctly?

The latest version of node seems to be unstable at this time, so I added the lts version to the build and move the latest to the 'allow_failures' sections which will still build it but won't fail the entire build if it fails

makes total sense, thanks

Signed-off-by: Paul Biccherai <[email protected]>

yurishkuro · 2019-05-09T20:50:23Z

src/span_context.js

@@ -124,8 +155,13 @@ export default class SpanContext {
    return this._samplingFinalized;
  }

-  set traceId(traceId: Buffer): void {
-    this._traceId = traceId;
+  set traceIdLow(traceIdLow: Buffer): void {


why do we need to expose low/high fields explicitly? It seems like the Buffer type can represent the full ID as a single buffer, and do it transparently to the user.

ok, I'll work on that

Signed-off-by: Paul Biccherai <[email protected]>

mwieczorek · 2019-05-29T19:31:53Z

Hi, I'm sorry for bothering once again.
But I guess @PaulMiami addressed comments from review.
@yurishkuro - could you take a look?

scott2449 · 2019-06-07T14:08:25Z

This update is critical to our usage w/ Istio @dowjones

everett980

This PR LGTM aside from using the deprecated Buffer constructor syntax.
The other changes are super minor.

everett980 · 2019-06-11T14:47:33Z

src/tracer.js

@@ -188,6 +188,8 @@ export default class Tracer {
   *        the created Span object. The time should be specified in
   *        milliseconds as Unix timestamp. Decimal value are supported
   *        to represent time values with sub-millisecond accuracy.
+   * @param {number} [options.traceId128bit] - generate root span with a


I believe this is a boolean based on the config and a test, so it should be @param {boolean} ... instead

everett980 · 2019-06-11T14:47:40Z

src/span_context.js

+      const safeTraceIdStr = this._traceIdStr.length % 2 == 0 ? this._traceIdStr : '0' + this._traceIdStr;
+      const tmpBuffer = new Buffer(safeTraceIdStr, 'hex');
+      const size = tmpBuffer.length > 8 ? 16 : 8;
+      this._traceId = new Buffer(size);


It seems as new Buffer is deprecated. We should probably use the new API. So this line and the subsequent line can jointed into:

this._traceId = Buffer.alloc(size)

Then we can rely on babel to provide backwards compatibility.

@everett980 ~~I don't think babel will provide backwards compatibility for the ~~Buffer.from~~ Buffer.alloc because it's not syntax related.

Buffer.alloc was added in 5.10. Seems like guarding on the presence of Buffer.alloc and falling back to the deprecated constructor might be preferred.

everett980 · 2019-06-11T14:48:01Z

src/tracer.js

@@ -239,7 +241,11 @@ export default class Tracer {
        ctx.baggage = parent.baggage;
      }

-      ctx.traceId = randomId;
+      if (options.traceId128bit) {
+        ctx.traceId = new Buffer.concat([Utils.getRandom64(), randomId]);


I believe you can just drop the new to avoid the deprecated constructor

I think the use of new is unnecessary, regardless of the constructor being deprecated.

everett980 · 2019-06-11T14:48:07Z

test/span_context.js

+    assert.equal('ffffffffffffffff', context.spanIdStr);
+    assert.deepEqual(Buffer.concat([LARGEST_64_BUFFER, LARGEST_64_BUFFER]), context.traceId);
+    assert.deepEqual(LARGEST_64_BUFFER, context.spanId);
+    assert.deepEqual(LARGEST_64_BUFFER, context.spanId);


This line is the same as the previous line.

everett980 · 2019-06-11T14:55:16Z

src/span_context.js

@@ -68,7 +68,12 @@ export default class SpanContext {

  get traceId(): any {
    if (this._traceId == null && this._traceIdStr != null) {
-      this._traceId = Utils.encodeInt64(this._traceIdStr);
+      const safeTraceIdStr = this._traceIdStr.length % 2 == 0 ? this._traceIdStr : '0' + this._traceIdStr;
+      const tmpBuffer = new Buffer(safeTraceIdStr, 'hex');


This should be Buffer.from(safeTraceIdStr, 'hex');

@everett980 Buffer.from is not available in node 0.10.x. It was added in 5.10.

I suggest using a factory which checks for the presence of the 5.10 factory functions and falls back to the constructor if they're not available.

tiffon

Thanks for the PR!

I believe the Buffer constructor is deprecated for security reasons. We should aim to use the preferred APIs, when they're available.

What do you think of using a factory function, in place of new Buffer(...), which will prefer the non-deprecated APIs and fallback to the constructor?

tiffon · 2019-06-12T18:27:10Z

src/span_context.js

@@ -68,7 +68,12 @@ export default class SpanContext {

  get traceId(): any {
    if (this._traceId == null && this._traceIdStr != null) {
-      this._traceId = Utils.encodeInt64(this._traceIdStr);
+      const safeTraceIdStr = this._traceIdStr.length % 2 == 0 ? this._traceIdStr : '0' + this._traceIdStr;
+      const tmpBuffer = new Buffer(safeTraceIdStr, 'hex');


@everett980 Buffer.from is not available in node 0.10.x. It was added in 5.10.

I suggest using a factory which checks for the presence of the 5.10 factory functions and falls back to the constructor if they're not available.

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-06-13T00:03:43Z

No problem!
Please @tiffon check out the changes and if you could also re-run the build because it failed with a socket error.
Thanks

tiffon · 2019-06-17T15:26:31Z

Thanks for the changes!

The build for 0.10 keeps hitting a test failure. A test which covers an error scenario is failing due to an emitted error having a different underlying OS error: ESRCH instead of ENOTFOUND.

jaeger-client-node/test/http_sender.js

Lines 241 to 256 in 8d0b1f7

    
           it('should gracefully handle errors emitted by socket.send', done => { 
        
             sender = new HTTPSender({ 
        
               endpoint: 'http://foo.bar.xyz', 
        
               maxSpanBatchSize: batchSize, 
        
             }); 
        
             sender.setProcess(reporter._process); 
        
             let tracer = new Tracer('test-service-name', new RemoteReporter(sender), new ConstSampler(true)); 
        
             tracer.startSpan('testSpan').finish(); 
        
             sender.flush((numSpans, err) => { 
        
               assert.equal(numSpans, 1); 
        
               expect(err).to.have.string('error sending spans over HTTP: Error: getaddrinfo ENOTFOUND'); 
        
               tracer.close(done); 
        
             }); 
        
           });

  1) http sender should gracefully handle errors emitted by socket.send:
     Uncaught AssertionError: expected 'error sending spans over HTTP: Error: getaddrinfo ESRCH' to contain 'error sending spans over HTTP: Error: getaddrinfo ENOTFOUND'
      at dist/test/http_sender.js:305:38
      at Function.invokeCallback (dist/src/reporters/sender_utils.js:33:9)
      at ClientRequest.<anonymous> (dist/src/reporters/http_sender.js:148:32)
      at Socket.socketErrorListener (http.js:1610:9)
      at net.js:834:16

This is passing on master (I reran it Friday), so it seems like it's related to the diff.

I think revising the expect(...) to be more generic would entail a breaking change (via a different error message) and expose folks to different error messages based on the version of node they're running.

Do you know what the issue might be?

@yurishkuro Any thoughts on what the issue might be?

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-06-17T20:44:35Z

@tiffon apparently travis is defaulting to ubuntu xenial now and master is building of trusty, I changed the travis file to force trusty.

tiffon · 2019-06-17T22:10:46Z

@PaulMiami Thanks!

tiffon

👍

yurishkuro · 2019-06-18T04:06:24Z

src/_flow/tracer.js

@@ -20,4 +20,5 @@ declare type startSpanOptions = {
  references?: Array<Reference>,
  tags?: any,
  startTime?: number,
+  traceId128bit?: boolean,


why is this needed? startSpan() is an OpenTracing API function, we cannot change its signature in Jaeger. This flag is only needed as tracer-level flag, not span-level.

didn't know that, I'll move it

src/span_context.js

yurishkuro · 2019-06-18T04:10:03Z

src/span_context.js

+      const safeTraceIdStr = this._traceIdStr.length % 2 == 0 ? this._traceIdStr : '0' + this._traceIdStr;
+      const tmpBuffer = Utils.newBuffer(safeTraceIdStr, 'hex');
+      const size = tmpBuffer.length > 8 ? 16 : 8;
+      this._traceId = Utils.newBuffer(size);


why do we need to allocate two buffers?

I need to create a 64 or 128 bits buffer and decode the hex string at the same, I don't think that can be done in one shot

It was unfortunate oversight that Jaeger clients allowed shortened string representation of trace/span IDs by dropping leading zeroes (an optimization that only affects ~1/16th of all traces, wasn't worth it), instead of always insisting on exactly 16 or 32 characters. I booked a meta-issue for this: jaegertracing/jaeger#1657

To maintain backwards compatibility we do need to be able to accept strings <16 or <32 chars, but always output strings of the exact length.

The overall implementation would probably be simpler if internal representation always uses a buffer of exact length, as you're doing above (an alternative would be use use the buffer of whatever length the input string is). But we should still avoid double-allocating the buffer. For example, in L73 instead of padding with a single zero, you could pad with as many zeros as needed to reach 16/32 length, and then your Utils.newBufferFromHex() function will return the right-sized buffer in one allocation. We still lose an allocation due to padding, but that will only affect about 1/16th of all traces, and once jaegertracing/jaeger#1657 is fixed for all languages padding won't be happening in practice.

yurishkuro · 2019-06-18T04:15:33Z

src/tracer.js

@@ -239,7 +241,11 @@ export default class Tracer {
        ctx.baggage = parent.baggage;
      }

-      ctx.traceId = randomId;
+      if (options.traceId128bit) {
+        ctx.traceId = Buffer.concat([Utils.getRandom64(), randomId]);


this can probably made more efficient by allocating 16-bytes buffer, copying randomId, and filling high bytes with more random values. Saves at least one allocation.

yurishkuro · 2019-06-18T04:16:04Z

src/util.js

+  | 'base64'
+  | 'latin1'
+  | 'binary'
+  | 'hex';


why do we need all of these?

yurishkuro · 2019-06-18T04:19:19Z

src/util.js

@@ -148,4 +159,28 @@ export default class Utils {
        error(err);
      });
  }
+
+  /**
+   * @param {string|number} input - a string to store in the buffer


There are no common code paths based on this param, and encoding is only used for string. I think this should be split into separate functions.

I'll split the code

yurishkuro · 2019-06-18T04:21:36Z

test/tracer.js

+  it('start a root span with 128 bit traceId', () => {
+    let span = tracer.startSpan('test-name', {
+      traceId128bit: true,
+    });


this is not OpenTracing-compliant code.

sorry didn't know that

tiffon

@yurishkuro Great points.

Requesting changes per Yuri's comments.

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-06-25T18:37:32Z

@tiffon I made the changes. The code coverage went down a little because the deprecated buffer code doesn't get hit

https://github.com/PaulMiami/jaeger-client-node/blob/a00710ef336e049a85daf0f9861b854cfa1c37c1/src/util.js#L178

https://github.com/PaulMiami/jaeger-client-node/blob/a00710ef336e049a85daf0f9861b854cfa1c37c1/src/util.js#L189-L191

src/util.js

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-07-09T01:24:59Z

@yurishkuro I made the changes but I am trying to figure out what's wrong with the build

  1) http sender should gracefully handle errors emitted by socket.send:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
  
  2) udp sender should gracefully handle errors emitted by socket.send:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

yurishkuro · 2019-07-11T11:52:42Z

I am not sure. Are they repeatable locally, or just in a one-off Travis run?

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-07-12T00:14:07Z

@yurishkuro it didn't fail initially on my local but after running it repeatedly it did once in a while.
I started to run mocha only for 'udp sender should gracefully handle errors emitted by socket.send' with no timeout and once in a while the time took over 2 seconds to complete.

...
✓ should gracefully handle errors emitted by socket.send
1 passing (39ms)
✓ should gracefully handle errors emitted by socket.send
1 passing (37ms)
✓ should gracefully handle errors emitted by socket.send (3020ms)
1 passing (3s)

I have increase the timeout on those 2 test to 5 seconds. It still failed for the cross docker test but yet seems random as the previous run worked for that test.

PaulMiami · 2019-07-12T16:02:54Z

@yurishkuro I did the same experiment from master locally and the same thing happens, once in a while those 2 tests (http and udp 'sender should gracefully handle errors emitted by socket.send') take a little over 3 seconds to complete

yurishkuro · 2019-07-15T20:46:41Z

if it's a flaky test we shouldn't block this PR. I restarted the build, let's see if it passes.

jmoney · 2019-07-31T01:16:31Z

@yurishkuro did the build pass? Any way I can help with this PR. I'm very interested in seeing this be approved and merged.

rafabene · 2019-09-03T15:56:14Z

@yurishkuro did the build pass? Any way I can help with this PR. I'm very interested in seeing this be approved and merged.

I'm also waiting for this PR so I can use this library together with Istio.
What is it needed to be merged?

yurishkuro · 2019-09-03T17:34:55Z

I will look at it soon, since we're doing a lot of work in Node client right now.

yurishkuro

There have been changes to the core files that now conflict with this, we need to do a merge

yurishkuro · 2019-08-05T23:04:01Z

src/span_context.js

+        this._traceId = Utils.newBufferFromHex(this._traceIdStr);
+      } else {
+        const paddings = ['0000000000000000', '00000000000000000000000000000000'];
+        const paddingIndex = traceIdExactLength === 16 ? 0 : 1;


can combine this into a single assignment with traceIdExactLength === 16 ? and avoid allocating array

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami · 2019-09-04T15:13:10Z

@yurishkuro I merged master

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro

Thank you for the PR!

* docs: docs and example for basic-tracer * chore: rename to node-basic-tracer for clarity * chore: rename again to basic-tracer-node * docs: update docs and example

Adding 128 bit traceId support

82f5a97

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami force-pushed the tmp-fix4 branch from 337f64f to 82f5a97 Compare April 26, 2019 22:26

fix node 0.10 issue

678561f

Signed-off-by: Paul Biccherai <[email protected]>

Replace node latest to lts in the travis config

cd7cf05

Signed-off-by: Paul Biccherai <[email protected]>

yurishkuro reviewed May 9, 2019

View reviewed changes

Fix travis file, adding back build for node latest

0c23d6f

Signed-off-by: Paul Biccherai <[email protected]>

yurishkuro reviewed May 9, 2019

View reviewed changes

PaulMiami added 3 commits May 15, 2019 15:14

Merge conflic

945237a

Signed-off-by: Paul Biccherai <[email protected]>

Refactoring traceId

16e1d91

Signed-off-by: Paul Biccherai <[email protected]>

Adding unit test

d5aa139

Signed-off-by: Paul Biccherai <[email protected]>

yurishkuro requested a review from tiffon June 10, 2019 22:53

everett980 suggested changes Jun 11, 2019

View reviewed changes

tiffon suggested changes Jun 12, 2019

View reviewed changes

Fixed issues

eb918d3

Signed-off-by: Paul Biccherai <[email protected]>

Force travis to use ubuntu trusty

4ec48e6

Signed-off-by: Paul Biccherai <[email protected]>

tiffon approved these changes Jun 17, 2019

View reviewed changes

yurishkuro reviewed Jun 18, 2019

View reviewed changes

tiffon suggested changes Jun 21, 2019

View reviewed changes

Fix issues

a00710e

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami force-pushed the tmp-fix4 branch from 91505b6 to a00710e Compare June 24, 2019 23:41

yurishkuro reviewed Jul 6, 2019

View reviewed changes

src/util.js Show resolved Hide resolved

PaulMiami added 2 commits July 8, 2019 18:46

Refactoring

5059f19

Signed-off-by: Paul Biccherai <[email protected]>

Fix node 0.10 compatibility

69ac16c

Signed-off-by: Paul Biccherai <[email protected]>

PaulMiami added 2 commits July 11, 2019 19:33

Increase timeout on some unit tests

50b49fe

Signed-off-by: Paul Biccherai <[email protected]>

Increase timeout on some unit tests

cfd324c

Signed-off-by: Paul Biccherai <[email protected]>

yurishkuro mentioned this pull request Aug 3, 2019

SpanContext cannot handle 128-bit traceId #295

Closed

yurishkuro reviewed Sep 3, 2019

View reviewed changes

PaulMiami added 2 commits September 4, 2019 10:52

Merge conflic

4d4a573

Signed-off-by: Paul Biccherai <[email protected]>

Small refactor

1f02e9c

Signed-off-by: Paul Biccherai <[email protected]>

yurishkuro and others added 4 commits September 4, 2019 16:36

Merge branch 'master' into tmp-fix4

df66094

Merge branch 'master' into tmp-fix4

ec0374a

Add one more test; create Buffer via Utils

eee5ab6

Signed-off-by: Yuri Shkuro <[email protected]>

Avoid Buffer.fill() which is void on Node 0.10

fb29d69

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Sep 9, 2019

View reviewed changes

yurishkuro merged commit 13935c7 into jaegertracing:master Sep 9, 2019

doochik mentioned this pull request Sep 16, 2019

initTracer should pass options.traceId128bit #397

Merged

Adding 128 bit traceId support #361

Adding 128 bit traceId support #361

Conversation

PaulMiami commented Apr 26, 2019

Which problem is this PR solving?

Short description of the changes

codecov bot commented Apr 26, 2019 • edited Loading

Codecov Report

mwieczorek commented May 9, 2019

PaulMiami commented May 9, 2019

PaulMiami commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwieczorek commented May 29, 2019

scott2449 commented Jun 7, 2019

everett980 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulMiami commented Jun 13, 2019 • edited Loading

tiffon commented Jun 17, 2019

PaulMiami commented Jun 17, 2019

tiffon commented Jun 17, 2019

tiffon left a comment

Choose a reason for hiding this comment

👍

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon left a comment

Choose a reason for hiding this comment

PaulMiami commented Jun 25, 2019 • edited Loading

PaulMiami commented Jul 9, 2019

yurishkuro commented Jul 11, 2019

PaulMiami commented Jul 12, 2019

PaulMiami commented Jul 12, 2019

yurishkuro commented Jul 15, 2019

jmoney commented Jul 31, 2019

rafabene commented Sep 3, 2019

yurishkuro commented Sep 3, 2019

yurishkuro left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulMiami commented Sep 4, 2019

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 26, 2019 •

edited

Loading

tiffon Jun 12, 2019 •

edited

Loading

tiffon Jun 12, 2019 •

edited

Loading

PaulMiami commented Jun 13, 2019 •

edited

Loading

PaulMiami commented Jun 25, 2019 •

edited

Loading

yurishkuro left a comment •

edited

Loading