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.

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

feat(tracing): Add context update methods to Span and Transaction #3192

Merged
merged 3 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
38 changes: 38 additions & 0 deletions packages/tracing/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,44 @@ export class Span implements SpanInterface {
return `${this.traceId}-${this.spanId}${sampledString}`;
}

/**
* @inheritDoc
*/
public toContext(): SpanContext {
return dropUndefinedKeys({
data: this.data,
description: this.description,
endTimestamp: this.endTimestamp,
op: this.op,
parentSpanId: this.parentSpanId,
sampled: this.sampled,
spanId: this.spanId,
startTimestamp: this.startTimestamp,
status: this.status,
tags: this.tags,
traceId: this.traceId,
});
}

/**
* @inheritDoc
*/
public updateWithContext(spanContext: SpanContext): this {
this.data = spanContext.data ?? {};
this.description = spanContext.description;
this.endTimestamp = spanContext.endTimestamp;
this.op = spanContext.op;
this.parentSpanId = spanContext.parentSpanId;
this.sampled = spanContext.sampled;
this.spanId = spanContext.spanId ?? this.spanId;
this.startTimestamp = spanContext.startTimestamp ?? this.startTimestamp;
this.status = spanContext.status;
this.tags = spanContext.tags ?? {};
this.traceId = spanContext.traceId ?? this.traceId;

return this;
}

/**
* @inheritDoc
*/
Expand Down
30 changes: 28 additions & 2 deletions packages/tracing/src/transaction.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { getCurrentHub, Hub } from '@sentry/hub';
import { Event, Measurements, Transaction as TransactionInterface, TransactionContext } from '@sentry/types';
import { isInstanceOf, logger } from '@sentry/utils';
import { dropUndefinedKeys, isInstanceOf, logger } from '@sentry/utils';

import { Span as SpanClass, SpanRecorder } from './span';

Expand All @@ -14,7 +14,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
*/
private readonly _hub: Hub = (getCurrentHub() as unknown) as Hub;

private readonly _trimEnd?: boolean;
private _trimEnd?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is kind of an internal bit, is there a reason we'd want people to be able to muck with it? Same goes for traceId, parentSpanId, and the two timestamps. It seems like we could dump/update a subset of the properties rather than all of them, and then we wouldn't need this change.

(To be fair, this same question applies to the original beforeNavigate - perhaps we should only pass a partial context there.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that a user shouldn't mess with these but I think the reason they are exposed, and that we should expose them (but not document them) for downstream SDKs like React Native and Capacitor to use.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it depends on if there's a use case for toContext beyond user-generated functions like beforeNavigate. If wrapper SDKs use them themselves, then yeah, okay, but there's also no reason they can't modify the object directly, is there? I thought the only reason to do this was because we're trying to match beforeNavigate's signature. No?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true, the initial reason that I came to this was to mimic beforeNavigate and the RN SDK could not set _trimEnd as it was private.

Copy link
Member

@lobsterkatie lobsterkatie Jan 21, 2021

Choose a reason for hiding this comment

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

Then I'd vote for:

  1. Do whatever you need to to _trimEnd to make the RN SDK able to do its job.
  2. Separately, have the methods above dump and read in only a subset of the data (whatever it makes sense to have a user touch in beforeNavigate). It occurs to me this also maybe could be used in what's passed to tracesSampler.

None of this is that critical, though, so your call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm so I think the updateWithContext methods in this PR should be left as is, and the dumping should be done by beforeNavigate and tracesSampler separately


/**
* This constructor should never be called manually. Those instrumenting tracing should use
Expand Down Expand Up @@ -119,4 +119,30 @@ export class Transaction extends SpanClass implements TransactionInterface {

return this._hub.captureEvent(transaction);
}

/**
* @inheritDoc
*/
public toContext(): TransactionContext {
const spanContext = super.toContext();

return dropUndefinedKeys({
...spanContext,
name: this.name,
trimEnd: this._trimEnd,
});
}

/**
* @inheritDoc
*/
public updateWithContext(transactionContext: TransactionContext): this {
super.updateWithContext(transactionContext);

this.name = transactionContext.name ?? '';

this._trimEnd = transactionContext.trimEnd;

return this;
}
}
82 changes: 82 additions & 0 deletions packages/tracing/test/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,4 +287,86 @@ describe('Span', () => {
});
});
});

describe('toContext and updateWithContext', () => {
test('toContext should return correct context', () => {
const originalContext = { traceId: 'a', spanId: 'b', sampled: false, description: 'test', op: 'op' };
const span = new Span(originalContext);

const newContext = span.toContext();

expect(newContext).toStrictEqual({
...originalContext,
data: {},
spanId: expect.any(String),
startTimestamp: expect.any(Number),
tags: {},
traceId: expect.any(String),
});
});

test('updateWithContext should completely change span properties', () => {
const originalContext = {
traceId: 'a',
spanId: 'b',
sampled: false,
description: 'test',
op: 'op',
tags: {
tag0: 'hello',
},
};
const span = new Span(originalContext);

span.updateWithContext({
traceId: 'c',
spanId: 'd',
sampled: true,
});

expect(span.traceId).toBe('c');
expect(span.spanId).toBe('d');
expect(span.sampled).toBe(true);
expect(span.description).toBe(undefined);
expect(span.op).toBe(undefined);
expect(span.tags).toStrictEqual({});
});

test('using toContext and updateWithContext together should update only changed properties', () => {
const originalContext = {
traceId: 'a',
spanId: 'b',
sampled: false,
description: 'test',
op: 'op',
tags: { tag0: 'hello' },
data: { data0: 'foo' },
};
const span = new Span(originalContext);

const newContext = {
...span.toContext(),
description: 'new',
endTimestamp: 1,
op: 'new-op',
sampled: true,
tags: {
tag1: 'bye',
},
};

if (newContext.data) newContext.data.data1 = 'bar';

span.updateWithContext(newContext);

expect(span.traceId).toBe('a');
expect(span.spanId).toBe('b');
expect(span.description).toBe('new');
expect(span.endTimestamp).toBe(1);
expect(span.op).toBe('new-op');
expect(span.sampled).toBe(true);
expect(span.tags).toStrictEqual({ tag1: 'bye' });
expect(span.data).toStrictEqual({ data0: 'foo', data1: 'bar' });
});
});
});
6 changes: 6 additions & 0 deletions packages/types/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ export interface Span extends SpanContext {
/** Return a traceparent compatible header string */
toTraceparent(): string;

/** Returns the current span properties as a `SpanContext` */
toContext(): SpanContext;

/** Updates the current span with a new `SpanContext` */
updateWithContext(spanContext: SpanContext): this;

/** Convert the object to JSON for w. spans array info only */
getTraceContext(): {
data?: { [key: string]: any };
Expand Down
6 changes: 6 additions & 0 deletions packages/types/src/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ export interface Transaction extends TransactionContext, Span {
* Set the name of the transaction
*/
setName(name: string): void;

/** Returns the current transaction properties as a `TransactionContext` */
toContext(): TransactionContext;

/** Updates the current transaction with a new `TransactionContext` */
updateWithContext(transactionContext: TransactionContext): this;
}

/**
Expand Down