-
Notifications
You must be signed in to change notification settings - Fork 131
Conversation
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@PaulMiami Any news on resolving the Travis errors? |
@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 <paul.biccherai@chronwell.com>
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 |
.travis.yml
Outdated
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for, and it is indented correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes total sense, thanks
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
src/span_context.js
Outdated
@@ -124,8 +155,13 @@ export default class SpanContext { | |||
return this._samplingFinalized; | |||
} | |||
|
|||
set traceId(traceId: Buffer): void { | |||
this._traceId = traceId; | |||
set traceIdLow(traceIdLow: Buffer): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll work on that
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Hi, I'm sorry for bothering once again. |
This update is critical to our usage w/ Istio @dowjones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR LGTM aside from using the deprecated Buffer constructor syntax.
The other changes are super minor.
src/tracer.js
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a boolean based on the config and a test, so it should be @param {boolean} ...
instead
src/span_context.js
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
src/tracer.js
Outdated
@@ -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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can just drop the new
to avoid the deprecated constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use of new
is unnecessary, regardless of the constructor being deprecated.
test/span_context.js
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is the same as the previous line.
src/span_context.js
Outdated
@@ -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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Buffer.from(safeTraceIdStr, 'hex');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/span_context.js
Outdated
@@ -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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 <paul.biccherai@chronwell.com>
No problem! |
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: jaeger-client-node/test/http_sender.js Lines 241 to 256 in 8d0b1f7
This is passing on master (I reran it Friday), so it seems like it's related to the diff. I think revising the Do you know what the issue might be? @yurishkuro Any thoughts on what the issue might be? |
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
@tiffon apparently travis is defaulting to ubuntu xenial now and master is building of trusty, I changed the travis file to force trusty. |
@PaulMiami Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/_flow/tracer.js
Outdated
@@ -20,4 +20,5 @@ declare type startSpanOptions = { | |||
references?: Array<Reference>, | |||
tags?: any, | |||
startTime?: number, | |||
traceId128bit?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know that, I'll move it
src/span_context.js
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to allocate two buffers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/tracer.js
Outdated
@@ -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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/util.js
Outdated
| 'base64' | ||
| 'latin1' | ||
| 'binary' | ||
| 'hex'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need all of these?
src/util.js
Outdated
@@ -148,4 +159,28 @@ export default class Utils { | |||
error(err); | |||
}); | |||
} | |||
|
|||
/** | |||
* @param {string|number} input - a string to store in the buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll split the code
test/tracer.js
Outdated
it('start a root span with 128 bit traceId', () => { | ||
let span = tracer.startSpan('test-name', { | ||
traceId128bit: true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not OpenTracing-compliant code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry didn't know that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro Great points.
Requesting changes per Yuri's comments.
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
@tiffon I made the changes. The code coverage went down a little because the deprecated buffer code doesn't get hit |
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
@yurishkuro I made the changes but I am trying to figure out what's wrong with the build
|
I am not sure. Are they repeatable locally, or just in a one-off Travis run? |
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
@yurishkuro it didn't fail initially on my local but after running it repeatedly it did once in a while.
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. |
@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 |
if it's a flaky test we shouldn't block this PR. I restarted the build, let's see if it passes. |
@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. |
I will look at it soon, since we're doing a lot of work in Node client right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been changes to the core files that now conflict with this, we need to do a merge
src/span_context.js
Outdated
this._traceId = Utils.newBufferFromHex(this._traceIdStr); | ||
} else { | ||
const paddings = ['0000000000000000', '00000000000000000000000000000000']; | ||
const paddingIndex = traceIdExactLength === 16 ? 0 : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can combine this into a single assignment with traceIdExactLength === 16 ?
and avoid allocating array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
Signed-off-by: Paul Biccherai <paul.biccherai@chronwell.com>
@yurishkuro I merged master |
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Signed-off-by: Paul Biccherai pbiccherai@hotmail.com
Which problem is this PR solving?
Resolves #295
Short description of the changes
Adding 128 bit support to the traceId