Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow recording links only at Span creation time #449

Merged
merged 2 commits into from
Oct 23, 2019
Merged

fix: allow recording links only at Span creation time #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2b91e7
fix: allow recording links only at Span creation time
mayurkale22 Oct 21, 2019
6dde832
fix: build
mayurkale22 Oct 23, 2019
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
12 changes: 4 additions & 8 deletions packages/opentelemetry-shim-opentracing/src/shim.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ function translateSpanOptions(
startTime: options.startTime,
};

// because there's no `Links` in SpanOptions, we set them in `TracerShim.startSpan()`
Copy link
Member Author

Choose a reason for hiding this comment

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

@bg451 this change will make you (and everyone) very happy :)

if (options.references) {
opts.links = translateReferences(options.references);
}

if (options.childOf) {
if (options.childOf instanceof SpanShim) {
opts.parent = (options.childOf as SpanShim).getSpan();
Expand Down Expand Up @@ -111,13 +114,6 @@ export class TracerShim extends opentracing.Tracer {
span.setAttributes(options.tags);
}

if (options.references) {
const links = translateReferences(options.references);
for (const link of links) {
span.addLink(link.spanContext, link.attributes);
}
}

return new SpanShim(this, span);
}

Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry-tracing/src/BasicTracer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export class BasicTracer implements types.Tracer {
spanContext,
options.kind || types.SpanKind.INTERNAL,
parentContext ? parentContext.spanId : undefined,
options.links || [],
options.startTime
);
// Set default attributes
Expand Down
13 changes: 2 additions & 11 deletions packages/opentelemetry-tracing/src/Span.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,14 @@ export class Span implements types.Span, ReadableSpan {
spanContext: types.SpanContext,
kind: types.SpanKind,
parentSpanId?: string,
links: types.Link[] = [],
startTime: types.TimeInput = hrTime()
) {
this.name = spanName;
this.spanContext = spanContext;
this.parentSpanId = parentSpanId;
this.kind = kind;
this.links = links;
this.startTime = timeInputToHrTime(startTime);
this._logger = parentTracer.logger;
this._traceParams = parentTracer.getActiveTraceParams();
Expand Down Expand Up @@ -134,17 +136,6 @@ export class Span implements types.Span, ReadableSpan {
return this;
}

addLink(spanContext: types.SpanContext, attributes?: types.Attributes): this {
if (this._isSpanEnded()) return this;

if (this.links.length >= this._traceParams.numberOfLinksPerSpan!) {
this._logger.warn('Dropping extra links.');
this.links.shift();
}
this.links.push({ spanContext, attributes });
return this;
}

setStatus(status: types.Status): this {
if (this._isSpanEnded()) return this;
this.status = status;
Expand Down
47 changes: 21 additions & 26 deletions packages/opentelemetry-tracing/test/Span.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ describe('Span', () => {
spanContext,
SpanKind.SERVER,
undefined,
[],
0
);
const timeMS = 123;
Expand All @@ -114,6 +115,7 @@ describe('Span', () => {
spanContext,
SpanKind.SERVER,
undefined,
[],
0
);
const timeMS = 123;
Expand Down Expand Up @@ -167,22 +169,22 @@ describe('Span', () => {
spanId: '5e0c63257de34c92',
traceFlags: TraceFlags.SAMPLED,
};
const span = new Span(tracer, name, spanContext, SpanKind.CLIENT);
span.addLink(spanContext);
span.addLink(spanContext, { attr1: 'value', attr2: 123, attr3: true });
const attributes = { attr1: 'value', attr2: 123, attr3: true };
const span = new Span(tracer, name, spanContext, SpanKind.CLIENT, '12345', [
{ spanContext },
{ spanContext, attributes },
]);
span.end();
});

it('should drop extra links, attributes and events', () => {
const span = new Span(tracer, name, spanContext, SpanKind.CLIENT);
for (let i = 0; i < 150; i++) {
span.addLink(spanContext);
span.setAttribute('foo' + i, 'bar' + i);
span.addEvent('sent' + i);
}
span.end();

assert.strictEqual(span.links.length, 32);
assert.strictEqual(span.events.length, 128);
assert.strictEqual(Object.keys(span.attributes).length, 32);
assert.strictEqual(span.events[span.events.length - 1].name, 'sent149');
Expand Down Expand Up @@ -246,27 +248,24 @@ describe('Span', () => {
});

it('should return ReadableSpan with links', () => {
const span = new Span(tracer, 'my-span', spanContext, SpanKind.CLIENT);
span.addLink(spanContext);
let readableSpan = span.toReadableSpan();
assert.strictEqual(readableSpan.links.length, 1);
assert.deepStrictEqual(readableSpan.links, [
{
attributes: undefined,
spanContext: {
spanId: '6e0c63257de34c92',
traceId: 'd4cda95b652f4a1592b449d5929fda1b',
traceFlags: 1,
const span = new Span(
tracer,
'my-span',
spanContext,
SpanKind.CLIENT,
undefined,
[
{ spanContext },
{
spanContext,
attributes: { attr1: 'value', attr2: 123, attr3: true },
},
},
]);

span.addLink(spanContext, { attr1: 'value', attr2: 123, attr3: true });
readableSpan = span.toReadableSpan();
]
);
const readableSpan = span.toReadableSpan();
assert.strictEqual(readableSpan.links.length, 2);
assert.deepStrictEqual(readableSpan.links, [
{
attributes: undefined,
spanContext,
},
{
Expand All @@ -276,10 +275,6 @@ describe('Span', () => {
]);

span.end();
// shouldn't add new link
span.addLink(spanContext);
readableSpan = span.toReadableSpan();
assert.strictEqual(readableSpan.links.length, 2);
});

it('should return ReadableSpan with events', () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/opentelemetry-types/src/trace/SpanOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { Span } from './span';
import { Attributes } from './attributes';
import { SpanKind } from './span_kind';
import { SpanContext } from './span_context';
import { Link } from './link';

/**
* Options needed for span creation
Expand All @@ -32,6 +33,9 @@ export interface SpanOptions {
/** Indicates that if this Span is active and recording information like events with the `AddEvent` operation and attributes using `setAttributes`. */
isRecording?: boolean;

/** A spans links */
links?: Link[];

/**
* A parent SpanContext (or Span, for convenience) that the newly-started
* span will be the child of.
Expand Down
9 changes: 0 additions & 9 deletions packages/opentelemetry-types/src/trace/span.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,6 @@ export interface Span {
startTime?: TimeInput
): this;

/**
* Adds a link to the Span.
*
* @param spanContext the context of the linked span.
* @param [attributes] the attributes that will be added; these are
* associated with this link.
*/
addLink(spanContext: SpanContext, attributes?: Attributes): this;

/**
* Sets a status to the span. If used, this will override the default Span
* status. Default is {@link CanonicalCode.OK}.
Expand Down