-
Notifications
You must be signed in to change notification settings - Fork 131
Adding 128 bit traceId support #361
Changes from 7 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 |
---|---|---|
|
@@ -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; | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const tmpBuffer = new Buffer(safeTraceIdStr, '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. This should be 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. @everett980 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. |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. It seems as
Then we can rely on babel to provide backwards compatibility. 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. @everett980 ~~I don't think babel will provide backwards compatibility for the
|
||
this._traceId.fill(0); | ||
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 {number} [options.traceId128bit] - generate root span with a | ||
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 believe this is a boolean based on the config and a test, so it should be |
||
* 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 = new 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. I believe you can just drop the 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 think the use of |
||
} else { | ||
ctx.traceId = randomId; | ||
} | ||
ctx.spanId = randomId; | ||
ctx.parentId = null; | ||
ctx.flags = flags; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,21 @@ describe('SpanContext should', () => { | |
assert.equal(flags, context.flags); | ||
}); | ||
|
||
it('return given values as they were set 128 bit', () => { | ||
let traceId = Buffer.concat([Utils.encodeInt64(2), Utils.encodeInt64(1)]); | ||
let spanId = Utils.encodeInt64(3); | ||
let parentId = Utils.encodeInt64(4); | ||
let flags = 1; | ||
|
||
let context = SpanContext.withBinaryIds(traceId, spanId, parentId, flags); | ||
|
||
assert.deepEqual(traceId, context.traceId); | ||
assert.deepEqual('20000000000000001', context.traceIdStr); | ||
assert.deepEqual(spanId, context.spanId); | ||
assert.deepEqual(parentId, context.parentId); | ||
assert.equal(flags, context.flags); | ||
}); | ||
|
||
it('return IsSampled properly', () => { | ||
let context = SpanContext.withBinaryIds( | ||
Utils.encodeInt64(1), | ||
|
@@ -76,6 +91,7 @@ describe('SpanContext should', () => { | |
let context = SpanContext.fromString('100:7f:0:1'); | ||
|
||
assert.deepEqual('100', context.traceIdStr); | ||
assert.deepEqual(Utils.encodeInt64(0x100), context.traceId); | ||
assert.deepEqual(Utils.encodeInt64(0x7f), context.spanId); | ||
assert.equal(null, context.parentId); | ||
assert.equal(1, context.flags); | ||
|
@@ -90,6 +106,26 @@ describe('SpanContext should', () => { | |
assert.equal(context.flags, 0x1); | ||
}); | ||
|
||
it('turn properly formatted strings into correct span contexts 128 bit', () => { | ||
let context = SpanContext.fromString('20000000000000100:7f:0:1'); | ||
|
||
assert.deepEqual('20000000000000100', context.traceIdStr); | ||
assert.deepEqual(Buffer.concat([Utils.encodeInt64(0x2), Utils.encodeInt64(0x100)]), context.traceId); | ||
assert.deepEqual(Utils.encodeInt64(0x7f), context.spanId); | ||
assert.equal(null, context.parentId); | ||
assert.equal(1, context.flags); | ||
|
||
// test large numbers | ||
context = SpanContext.fromString('ffffffffffffffffffffffffffffffff:ffffffffffffffff:5:1'); | ||
assert.equal('ffffffffffffffffffffffffffffffff', context.traceIdStr); | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This line is the same as the previous line. |
||
assert.deepEqual(Utils.encodeInt64(0x5), context.parentId); | ||
assert.equal(context.flags, 0x1); | ||
}); | ||
|
||
it('return null on malformed traces', () => { | ||
assert.equal(SpanContext.fromString('bad value'), null); | ||
assert.equal(SpanContext.fromString('1:1:1:1:1'), null, 'Too many colons'); | ||
|
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