Skip to content

ref(tracing): Rework tracestate internals to allow for third-party data #3266

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

13 changes: 9 additions & 4 deletions packages/core/src/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export function eventToSentryRequest(event: Event, api: API): SentryRequest {
export function transactionToSentryRequest(event: Event, api: API): SentryRequest {
const sdkInfo = getSdkMetadataForEnvelopeHeader(api);

const { transactionSampling, tracestate: encodedTracestate, ...metadata } = event.debug_meta || {};
const { transactionSampling, tracestate, ...metadata } = event.debug_meta || {};
const { method: samplingMethod, rate: sampleRate } = transactionSampling || {};
if (Object.keys(metadata).length === 0) {
delete event.debug_meta;
Expand All @@ -77,9 +77,14 @@ export function transactionToSentryRequest(event: Event, api: API): SentryReques

// the tracestate is stored in bas64-encoded JSON, but envelope header values are expected to be full JS values,
// so we have to decode and reinflate it
let tracestate;
let reinflatedTracestate;
try {
tracestate = JSON.parse(base64ToUnicode(encodedTracestate as string));
// Because transaction metadata passes through a number of locations (transactionContext, transaction, event during
// processing, event as sent), each with different requirements, all of the parts are typed as optional. That said,
// if we get to this point and either `tracestate` or `tracestate.sentry` are undefined, something's gone very wrong.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const encodedSentryValue = tracestate!.sentry!.replace('sentry=', '');
reinflatedTracestate = JSON.parse(base64ToUnicode(encodedSentryValue));
} catch (err) {
logger.warn(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

This would log Cannot read property 'replace' of undefined for situations when there's simply no tracestate.sentry which might be confusing to the user.

Copy link
Member Author

@lobsterkatie lobsterkatie Feb 17, 2021

Choose a reason for hiding this comment

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

True. Originally I had it as const encodedSentryValue = tracestate!.sentry!.replace('sentry=', '');, since if we get to that point and there's no tracestate.sentry, things have gone real wrong, but then it was yelling at me about the !. But I think maybe I should change it back, to better communicate to the reader that it's not optional.

Or maybe I can write my types in a smarter way... In any case, will fix it one way or another.

UPDATE: Okay, tried with the types, but there are too many stages at which this or that property may (legitimately) not be defined, so I'm going with the !.

}
Expand All @@ -88,7 +93,7 @@ export function transactionToSentryRequest(event: Event, api: API): SentryReques
event_id: event.event_id,
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
...(tracestate && { trace: tracestate }), // trace context for dynamic sampling on relay
...(reinflatedTracestate && { trace: reinflatedTracestate }), // trace context for dynamic sampling on relay
});

const itemHeaders = JSON.stringify({
Expand Down
8 changes: 5 additions & 3 deletions packages/core/test/lib/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ describe('eventToSentryRequest', () => {
// release: 'off.leash.park',
// public_key: 'dogsarebadatkeepingsecrets',
// }),
tracestate:
'eyJ0cmFjZV9pZCI6IjEyMzEyMDEyMTEyMTIwMTIiLCJlbnZpcm9ubWVudCI6ImRvZ3BhcmsiLCJyZWxlYXNlIjoib2ZmLmxlYXNo' +
'LnBhcmsiLCJwdWJsaWNfa2V5IjoiZG9nc2FyZWJhZGF0a2VlcGluZ3NlY3JldHMifQ',
tracestate: {
sentry:
'sentry=eyJ0cmFjZV9pZCI6IjEyMzEyMDEyMTEyMTIwMTIiLCJlbnZpcm9ubWVudCI6ImRvZ3BhcmsiLCJyZWxlYXNlIjoib2ZmLmxlYXNo' +
'LnBhcmsiLCJwdWJsaWNfa2V5IjoiZG9nc2FyZWJhZGF0a2VlcGluZ3NlY3JldHMifQ',
},
},
spans: [],
transaction: '/dogs/are/great/',
Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/src/browser/request.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { getCurrentHub } from '@sentry/hub';
import { Span } from '@sentry/types';
import { addInstrumentationHandler, isInstanceOf, isMatchingPattern } from '@sentry/utils';

import { Span } from '../span';
import { getActiveTransaction, hasTracingEnabled } from '../utils';

export const DEFAULT_TRACING_ORIGINS = ['localhost', /^\//];
Expand Down
39 changes: 37 additions & 2 deletions packages/tracing/src/span.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
/* eslint-disable max-lines */
import { Primitive, Span as SpanInterface, SpanContext, TraceHeaders, Transaction } from '@sentry/types';
import { getCurrentHub } from '@sentry/hub';
import { Hub, Primitive, Span as SpanInterface, SpanContext, TraceHeaders, Transaction } from '@sentry/types';
import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils';

import { SpanStatus } from './spanstatus';
import { computeTracestateValue } from './utils';

/**
* Keeps track of finished spans for a given transaction
Expand Down Expand Up @@ -289,7 +291,12 @@ export class Span implements SpanInterface {
*/
public getTraceHeaders(): TraceHeaders {
// tracestates live on the transaction, so if this is a free-floating span, there won't be one
const tracestate = this.transaction && `sentry=${this.transaction.tracestate}`; // TODO kmclb
let tracestate;
if (this.transaction) {
tracestate = this.transaction.metadata?.tracestate?.sentry;
} else {
tracestate = this._getNewTracestate();
}

return {
'sentry-trace': this.toTraceparent(),
Expand Down Expand Up @@ -352,4 +359,32 @@ export class Span implements SpanInterface {
trace_id: this.traceId,
});
}

/**
* Create a new Sentry tracestate header entry (i.e. `sentry=xxxxxx`)
*
* @returns The new Sentry tracestate entry, or undefined if there's no client or no dsn
*/
protected _getNewTracestate(hub: Hub = getCurrentHub()): string | undefined {
const client = hub.getClient();
const dsn = client?.getDsn();

if (!client || !dsn) {
return;
}

const { environment, release } = client.getOptions() || {};

// TODO - the only reason we need the non-null assertion on `dsn.publicKey` (below) is because `dsn.publicKey` has
// to be optional while we transition from `dsn.user` -> `dsn.publicKey`. Once `dsn.user` is removed, we can make
// `dsn.publicKey` required and remove the `!`.

return `sentry=${computeTracestateValue({
trace_id: this.traceId,
environment,
release,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
public_key: dsn.publicKey!,
})}`;
}
}
50 changes: 13 additions & 37 deletions packages/tracing/src/transaction.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
import { getCurrentHub, Hub } from '@sentry/hub';
import { DebugMeta, Event, Measurements, Transaction as TransactionInterface, TransactionContext } from '@sentry/types';
import {
Event,
Measurements,
Transaction as TransactionInterface,
TransactionContext,
TransactionMetadata,
} from '@sentry/types';
import { dropUndefinedKeys, isInstanceOf, logger } from '@sentry/utils';

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

type TransactionMetadata = Pick<DebugMeta, 'transactionSampling' | 'tracestate'>;

/** JSDoc */
export class Transaction extends SpanClass implements TransactionInterface {
public name: string;

public readonly tracestate: string;

private _metadata: TransactionMetadata = {};
public metadata: TransactionMetadata;

private _measurements: Measurements = {};

Expand Down Expand Up @@ -42,9 +43,9 @@ export class Transaction extends SpanClass implements TransactionInterface {

this._trimEnd = transactionContext.trimEnd;

// _getNewTracestate only returns undefined in the absence of a client or dsn, in which case it doesn't matter what
// the header values are - nothing can be sent anyway - so the third alternative here is just to make TS happy
this.tracestate = transactionContext.tracestate || this._getNewTracestate() || 'things are broken';
this.metadata = transactionContext.metadata || {};
this.metadata.tracestate = this.metadata.tracestate || {};
this.metadata.tracestate.sentry = this.metadata.tracestate.sentry || this._getNewTracestate(this._hub);
Comment on lines +46 to +48
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a nicer/more compact way to do this? This feels like a lot of code, but I don't know of a setter version of the ?. getter syntax.

Copy link
Contributor

@kamilogorek kamilogorek Feb 17, 2021

Choose a reason for hiding this comment

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

No, not really, but we do this in so many places, that I might actually write

getOrDefault(this, 'metadata.tracestate.sentry', this._getNewTracestate(this._hub))

helper for this :<

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, except we also need to set the data. I think a helper isn't a bad idea, but I'll save that for a separate PR.


// this is because transactions are also spans, and spans have a transaction pointer
this.transaction = this;
Expand Down Expand Up @@ -81,7 +82,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
* @hidden
*/
public setMetadata(newMetadata: TransactionMetadata): void {
this._metadata = { ...this._metadata, ...newMetadata };
this.metadata = { ...this.metadata, ...newMetadata };
}

/**
Expand Down Expand Up @@ -118,8 +119,6 @@ export class Transaction extends SpanClass implements TransactionInterface {
}).endTimestamp;
}

this._metadata.tracestate = this.tracestate;

const transaction: Event = {
contexts: {
trace: this.getTraceContext(),
Expand All @@ -130,7 +129,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
timestamp: this.endTimestamp,
transaction: this.name,
type: 'transaction',
debug_meta: this._metadata,
debug_meta: this.metadata,
};

const hasMeasurements = Object.keys(this._measurements).length > 0;
Expand Down Expand Up @@ -168,27 +167,4 @@ export class Transaction extends SpanClass implements TransactionInterface {

return this;
}

/**
* Create a new tracestate header value
*
* @returns The new tracestate value, or undefined if there's no client or no dsn
*/
private _getNewTracestate(): string | undefined {
const client = this._hub.getClient();
const dsn = client?.getDsn();

if (!client || !dsn) {
return;
}

const { environment, release } = client.getOptions() || {};

// TODO - the only reason we need the non-null assertion on `dsn.publicKey` (below) is because `dsn.publicKey` has
// to be optional while we transition from `dsn.user` -> `dsn.publicKey`. Once `dsn.user` is removed, we can make
// `dsn.publicKey` required and remove the `!`.

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return computeTracestateValue({ trace_id: this.traceId, environment, release, public_key: dsn.publicKey! });
}
}
25 changes: 13 additions & 12 deletions packages/tracing/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,23 @@ export function secToMs(time: number): number {
// so it can be used in manual instrumentation without necessitating a hard dependency on @sentry/utils
export { stripUrlQueryAndFragment } from '@sentry/utils';

type TracestateData = {
trace_id: string;
environment: string | undefined | null;
release: string | undefined | null;
public_key: string;
};

/**
* Compute the value of a tracestate header.
* Compute the value of a Sentry tracestate header.
*
* @throws SentryError (because using the logger creates a circular dependency)
* @returns the base64-encoded header value
*/
// Note: this is here instead of in the tracing package since @sentry/core tests rely on it
export function computeTracestateValue(tracestateData: {
trace_id: string;
environment: string | undefined | null;
release: string | undefined | null;
public_key: string;
}): string {
export function computeTracestateValue(data: TracestateData): string {
// `JSON.stringify` will drop keys with undefined values, but not ones with null values
tracestateData.environment = tracestateData.environment || null;
tracestateData.release = tracestateData.release || null;
data.environment = data.environment || null;
data.release = data.release || null;

// See https://www.w3.org/TR/trace-context/#tracestate-header-field-values
// The spec for tracestate header values calls for a string of the form
Expand All @@ -94,8 +95,8 @@ export function computeTracestateValue(tracestateData: {
// used to pad the end of base64 values though, so to avoid confusion, we strip them off. (Most languages' base64
// decoding functions (including those in JS) are able to function without the padding.)
try {
return unicodeToBase64(JSON.stringify(tracestateData)).replace(/={1,2}$/, '');
return unicodeToBase64(JSON.stringify(data)).replace(/={1,2}$/, '');
} catch (err) {
throw new SentryError(`[Tracing] Error creating tracestate header: ${err}`);
throw new SentryError(`[Tracing] Error computing tracestate value from data: ${err}\nData: ${data}`);
}
}
15 changes: 11 additions & 4 deletions packages/tracing/test/hub.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,19 @@ describe('Hub', () => {
name: 'dogpark',
traceId: '12312012123120121231201212312012',
parentSpanId: '1121201211212012',
tracestate: 'doGsaREgReaT',
metadata: { tracestate: { sentry: 'sentry=doGsaREgReaT' } },
};
const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
const transaction = hub.startTransaction(transactionContext);

expect(transaction).toEqual(expect.objectContaining(transactionContext));
expect(transaction).toEqual(
expect.objectContaining({
name: 'dogpark',
traceId: '12312012123120121231201212312012',
parentSpanId: '1121201211212012',
metadata: expect.objectContaining({ tracestate: { sentry: 'sentry=doGsaREgReaT' } }),
}),
);
});

it('creates a new tracestate value if not given one in transaction context', () => {
Expand All @@ -62,7 +69,7 @@ describe('Hub', () => {
public_key: 'dogsarebadatkeepingsecrets',
});

expect(transaction.tracestate).toEqual(b64Value);
expect(transaction.metadata?.tracestate?.sentry).toEqual(`sentry=${b64Value}`);
});

it('uses default environment if none given', () => {
Expand All @@ -80,7 +87,7 @@ describe('Hub', () => {
public_key: 'dogsarebadatkeepingsecrets',
});

expect(transaction.tracestate).toEqual(b64Value);
expect(transaction.metadata?.tracestate?.sentry).toEqual(`sentry=${b64Value}`);
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/test/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe('Span', () => {
const headers = span.getTraceHeaders();

expect(headers['sentry-trace']).toEqual(span.toTraceparent());
expect(headers.tracestate).toEqual(`sentry=${transaction.tracestate}`);
expect(headers.tracestate).toEqual(transaction.metadata?.tracestate?.sentry);
});
});

Expand Down
10 changes: 4 additions & 6 deletions packages/types/src/debugMeta.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import { TransactionMetadata } from './transaction';

/**
* Holds meta information to customize the behavior of sentry's event processing.
**/
export interface DebugMeta {
export type DebugMeta = {
images?: Array<DebugImage>;
transactionSampling?: { rate?: number; method?: string };

/** The Sentry portion of a transaction's tracestate header, used for dynamic sampling */
tracestate?: string;
}
} & TransactionMetadata;

/**
* Possible choices for debug images.
Expand Down
1 change: 1 addition & 0 deletions packages/types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export {
TraceparentData,
Transaction,
TransactionContext,
TransactionMetadata,
TransactionSamplingMethod,
} from './transaction';
export { Thread } from './thread';
Expand Down
14 changes: 11 additions & 3 deletions packages/types/src/transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ export interface TransactionContext extends SpanContext {
parentSampled?: boolean;

/**
* The tracestate header value associated with this transaction, potentially inherited from a parent transaction,
* which will be propagated across services to all child transactions
* Metadata associated with the transaction, for internal SDK use.
*/
tracestate?: string;
metadata?: TransactionMetadata;
}

/**
Expand Down Expand Up @@ -119,3 +118,12 @@ export enum TransactionSamplingMethod {
Rate = 'client_rate',
Inheritance = 'inheritance',
}

export interface TransactionMetadata {
transactionSampling?: { rate?: number; method?: string };

/** The sentry half of a transaction's tracestate header, used for dynamic sampling */
tracestate?: {
sentry?: string;
};
}