Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Adding 128 bit traceId support #361

Merged
merged 20 commits into from
Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ npm-debug.log
dist/

.idea/
.vscode/
*.iml

crossdock/jaeger-docker-compose.yml
1 change: 1 addition & 0 deletions src/_flow/tracer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ declare type startSpanOptions = {
references?: Array<Reference>,
tags?: any,
startTime?: number,
traceId128bit?: boolean,
Copy link
Member

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.

Copy link
Contributor Author

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

};
7 changes: 6 additions & 1 deletion src/span_context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

const size = tmpBuffer.length > 8 ? 16 : 8;
this._traceId = new Buffer(size);
Copy link
Collaborator

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.

Copy link
Member

@tiffon tiffon Jun 12, 2019

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.

this._traceId.fill(0);
tmpBuffer.copy(this._traceId, size - tmpBuffer.length);
}
return this._traceId;
}
Expand Down
24 changes: 20 additions & 4 deletions src/thrift.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,23 +104,39 @@ export default class ThriftUtils {

thriftRefs.push({
refType: refEnum,
traceIdLow: context.traceId,
traceIdHigh: ThriftUtils.emptyBuffer,
traceIdLow: ThriftUtils.getTraceIdLow(context.traceId),
traceIdHigh: ThriftUtils.getTraceIdHigh(context.traceId),
spanId: context.spanId,
});
}

return thriftRefs;
}

static getTraceIdLow(traceId: Buffer) {
if (traceId != null) {
return traceId.slice(-8);
}

return ThriftUtils.emptyBuffer;
}

static getTraceIdHigh(traceId: Buffer) {
if (traceId != null && traceId.length > 8) {
return traceId.slice(-16, -8);
}

return ThriftUtils.emptyBuffer;
}

static spanToThrift(span: Span): any {
let tags = ThriftUtils.getThriftTags(span._tags);
let logs = ThriftUtils.getThriftLogs(span._logs);
let unsigned = true;

return {
traceIdLow: span._spanContext.traceId,
traceIdHigh: ThriftUtils.emptyBuffer, // TODO(oibe) implement 128 bit ids
traceIdLow: ThriftUtils.getTraceIdLow(span._spanContext.traceId),
traceIdHigh: ThriftUtils.getTraceIdHigh(span._spanContext.traceId),
spanId: span._spanContext.spanId,
parentSpanId: span._spanContext.parentId || ThriftUtils.emptyBuffer,
operationName: span._operationName,
Expand Down
8 changes: 7 additions & 1 deletion src/tracer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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

* 128bit traceId.
* @return {Span} - a new Span object.
**/
startSpan(operationName: string, options: ?startSpanOptions): Span {
Expand Down Expand Up @@ -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]);
Copy link
Collaborator

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

Copy link
Member

@tiffon tiffon Jun 12, 2019

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.

} else {
ctx.traceId = randomId;
}
ctx.spanId = randomId;
ctx.parentId = null;
ctx.flags = flags;
Expand Down
36 changes: 36 additions & 0 deletions test/span_context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Copy link
Collaborator

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.

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');
Expand Down
31 changes: 31 additions & 0 deletions test/thrift.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
// the License.

import { assert } from 'chai';
import opentracing from 'opentracing';
import ConstSampler from '../src/samplers/const_sampler.js';
import InMemoryReporter from '../src/reporters/in_memory_reporter.js';
import Tracer from '../src/tracer.js';
Expand Down Expand Up @@ -64,4 +65,34 @@ describe('ThriftUtils', () => {
assert.deepEqual(tSpan.duration, Utils.encodeInt64((123.678 - 123.456) * 1000));
assert.deepEqual(tSpan.logs[0].timestamp, Utils.encodeInt64(123567));
});

it('should convert span with 128 bit traceId', () => {
let reporter = new InMemoryReporter();
let tracer = new Tracer('test-service-name', reporter, new ConstSampler(true));
let span = tracer.startSpan('some operation', { traceId128bit: true });
let childOfRef = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, span.context());
let childSpan = tracer.startSpan('some child operation', { references: [childOfRef] });
childSpan.finish();
span.finish();
tracer.close();
let tSpan = ThriftUtils.spanToThrift(childSpan);
assert.deepEqual(tSpan.traceIdLow, childSpan.context().traceId.slice(-8));
assert.deepEqual(tSpan.traceIdHigh, childSpan.context().traceId.slice(-16, -8));
assert.deepEqual(tSpan.spanId, childSpan.context().spanId);
assert.deepEqual(tSpan.references[0].traceIdLow, span.context().traceId.slice(-8));
assert.deepEqual(tSpan.references[0].traceIdHigh, span.context().traceId.slice(-16, -8));
assert.deepEqual(tSpan.references[0].spanId, span.context().spanId);
});

it('should convert extract traceIdLow and traceIdHigh', () => {
let traceIdLow = Utils.encodeInt64(1);
let traceIdHigh = Utils.encodeInt64(2);
let traceId = Buffer.concat([traceIdHigh, traceIdLow]);

assert.deepEqual(ThriftUtils.getTraceIdLow(traceId), traceIdLow);
assert.deepEqual(ThriftUtils.getTraceIdHigh(traceId), traceIdHigh);

assert.deepEqual(ThriftUtils.getTraceIdLow(null), ThriftUtils.emptyBuffer);
assert.deepEqual(ThriftUtils.getTraceIdHigh(null), ThriftUtils.emptyBuffer);
});
});
43 changes: 43 additions & 0 deletions test/tracer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -418,6 +450,17 @@ describe('tracer should', () => {
});
});

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

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.

Copy link
Contributor Author

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


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',
Expand Down