-
Notifications
You must be signed in to change notification settings - Fork 131
Adding 128 bit traceId support #361
Changes from 9 commits
82f5a97
678561f
cd7cf05
0c23d6f
945237a
16e1d91
d5aa139
eb918d3
4ec48e6
a00710e
5059f19
69ac16c
50b49fe
cfd324c
4d4a573
1f02e9c
df66094
ec0374a
eee5ab6
fb29d69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ npm-debug.log | |
dist/ | ||
|
||
.idea/ | ||
.vscode/ | ||
*.iml | ||
|
||
crossdock/jaeger-docker-compose.yml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
sudo: required | ||
dist: trusty | ||
|
||
# Travis will use .nvmrc, by default | ||
language: node_js | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,11 @@ 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; | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 commentThe 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 commentThe 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 commentThe 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 |
||
tmpBuffer.copy(this._traceId, size - tmpBuffer.length); | ||
} | ||
return this._traceId; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 {boolean} [options.traceId128bit] - generate root span with a | ||
* 128bit traceId. | ||
* @return {Span} - a new Span object. | ||
**/ | ||
startSpan(operationName: string, options: ?startSpanOptions): Span { | ||
|
@@ -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 commentThe 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. |
||
} else { | ||
ctx.traceId = randomId; | ||
} | ||
ctx.spanId = randomId; | ||
ctx.parentId = null; | ||
ctx.flags = flags; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,17 @@ import Int64 from 'node-int64'; | |
import os from 'os'; | ||
import http from 'http'; | ||
|
||
type BufferEncoding = | ||
| 'ascii' | ||
| 'utf8' | ||
| 'utf-8' | ||
| 'utf16le' | ||
| 'ucs2' | ||
| 'ucs-2' | ||
| 'base64' | ||
| 'latin1' | ||
| 'binary' | ||
| 'hex'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need all of these? |
||
export default class Utils { | ||
/** | ||
* Determines whether a string contains a given prefix. | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. There are no common code paths based on this param, and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll split the code |
||
* or a number of octets to allocate. | ||
* @param {string} encoding - identifies the character encoding. Default is 'utf8'. | ||
* @return {Buffer} - returns a buffer representing the encoded string, or an empty buffer. | ||
**/ | ||
static newBuffer(input: string | number, encoding?: BufferEncoding): Buffer { | ||
if (typeof input === 'string') { | ||
if (Buffer.from && Buffer.from !== Uint8Array.from) { | ||
return Buffer.from(input, encoding); | ||
} | ||
return new Buffer(input, encoding); | ||
} else if (typeof input === 'number') { | ||
if (Buffer.alloc) { | ||
return Buffer.alloc(input); | ||
} | ||
const buffer = new Buffer(input); | ||
buffer.fill(0); | ||
return buffer; | ||
} else { | ||
throw new Error('The "input" argument must be a number or a string'); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -287,6 +287,38 @@ describe('tracer should', () => { | |
assertByFormat(opentracing.FORMAT_HTTP_HEADERS); | ||
}); | ||
|
||
it('inject plain text headers into carrier, and extract span context with the same value 128bits', () => { | ||
let keyOne = 'keyOne'; | ||
let keyTwo = 'keyTwo'; | ||
let baggage = { | ||
keyOne: 'leela', | ||
keyTwo: 'bender', | ||
}; | ||
let savedContext = SpanContext.withBinaryIds( | ||
Buffer.concat([Utils.encodeInt64(1), Utils.encodeInt64(2)]), | ||
Utils.encodeInt64(2), | ||
Utils.encodeInt64(3), | ||
constants.SAMPLED_MASK, | ||
baggage | ||
); | ||
|
||
let assertByFormat = format => { | ||
let carrier = {}; | ||
tracer.inject(savedContext, format, carrier); | ||
let extractedContext = tracer.extract(format, carrier); | ||
|
||
assert.deepEqual(savedContext.traceId, extractedContext.traceId); | ||
assert.deepEqual(savedContext.spanId, extractedContext.spanId); | ||
assert.deepEqual(savedContext.parentId, extractedContext.parentId); | ||
assert.equal(savedContext.flags, extractedContext.flags); | ||
assert.equal(savedContext.baggage[keyOne], extractedContext.baggage[keyOne]); | ||
assert.equal(savedContext.baggage[keyTwo], extractedContext.baggage[keyTwo]); | ||
}; | ||
|
||
assertByFormat(opentracing.FORMAT_TEXT_MAP); | ||
assertByFormat(opentracing.FORMAT_HTTP_HEADERS); | ||
}); | ||
|
||
it('inject url encoded values into headers', () => { | ||
let baggage = { | ||
keyOne: 'Leela vs. Bender', | ||
|
@@ -418,6 +450,17 @@ describe('tracer should', () => { | |
}); | ||
}); | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. sorry didn't know that |
||
|
||
assert.deepEqual(span.context().traceId.slice(-8), span.context().spanId); | ||
assert.isOk(span.context().traceId); | ||
assert.isNotOk(span.context().parentId); | ||
assert.isOk(span.context().isSampled()); | ||
}); | ||
|
||
it('should NOT mutate tags', () => { | ||
const tags = { | ||
robot: 'bender', | ||
|
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