Skip to content

feat(tracing): Add user data to tracestate header #3343

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

Merged
merged 7 commits into from
Apr 2, 2021
Merged

feat(tracing): Add user data to tracestate header #3343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ba466c9
add metadata to Transaction type and segment to User type
lobsterkatie Mar 24, 2021
86ac6a3
move to lazy creation of tracestate value
lobsterkatie Mar 24, 2021
a2ee549
include user data in tracestate
lobsterkatie Mar 24, 2021
2572ff4
when creating tracestate, get hub from transaction if possible
lobsterkatie Mar 24, 2021
ba14834
add tests
lobsterkatie Mar 24, 2021
2a7b830
change schema to match spec
lobsterkatie Mar 24, 2021
7cc1d39
fix typo
lobsterkatie Mar 24, 2021
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
Prev Previous commit
Next Next commit
move to lazy creation of tracestate value
  • Loading branch information
@lobsterkatie
lobsterkatie committed Mar 24, 2021
commit 86ac6a3506acde5f2d227611a7992343797acf2b
12 changes: 11 additions & 1 deletion packages/tracing/src/span.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,14 @@ export class Span implements SpanInterface {
* @inheritDoc
*/
public getTraceHeaders(): TraceHeaders {
// if this span is part of a transaction, but that transaction doesn't yet have a tracestate value, create one
if (this.transaction && !this.transaction?.metadata.tracestate?.sentry) {
this.transaction.metadata.tracestate = {
...this.transaction.metadata.tracestate,
sentry: this._getNewTracestate(),
};
}

const tracestate = this._toTracestate();

return {
Expand Down Expand Up @@ -392,9 +400,11 @@ export class Span implements SpanInterface {
}

/**
* Return a tracestate-compatible header string. Returns undefined if there is no client or no DSN.
* Return a tracestate-compatible header string, including both sentry and third-party data (if any). Returns
* undefined if there is no client or no DSN.
*/
private _toTracestate(): string | undefined {
// if this is an orphan span, crate a new tracestate value
const sentryTracestate = this.transaction?.metadata?.tracestate?.sentry || this._getNewTracestate();
let thirdpartyTracestate = this.transaction?.metadata?.tracestate?.thirdparty;

Expand Down
10 changes: 5 additions & 5 deletions packages/tracing/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@ export class Transaction extends SpanClass implements TransactionInterface {
this._trimEnd = transactionContext.trimEnd;
this._hub = hub || getCurrentHub();

// create a new sentry tracestate value if we didn't inherit one
if (!this.metadata.tracestate?.sentry) {
this.metadata.tracestate = { ...this.metadata.tracestate, sentry: this._getNewTracestate(this._hub) };
}

// this is because transactions are also spans, and spans have a transaction pointer
this.transaction = this;
}
Expand Down Expand Up @@ -117,6 +112,11 @@ export class Transaction extends SpanClass implements TransactionInterface {
}).endTimestamp;
}

// ensure that we have a tracestate to attach to the envelope header
if (!this.metadata.tracestate?.sentry) {
this.metadata.tracestate = { ...this.metadata.tracestate, sentry: this._getNewTracestate() };
}

Comment on lines +115 to +119
Copy link

@RaduW RaduW Mar 24, 2021

Choose a reason for hiding this comment

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

Nit: I saw that this is not a new thing (it predates this PR) but why do we push tracestate in debug_meta ?
Nobody uses it in Relay from debug_meata (I'm looking for it in the http headers).

If we didn't _getNewTracestate() could become private.

Copy link
Member Author

@lobsterkatie lobsterkatie Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

why do we push tracestate in debug_meta ?
Nobody uses it in Relay from debug_meata (I'm looking for it in the http headers).

Because it needs to get through to here somehow, and putting it in debug_meta means that even if it slips through* and makes it to sentry, it won't show up for the user. (Previously the information hitched a ride on a custom tag, and a bug in the code meant that it was showing up for users.)

*If you look a few lines into the linked function, you'll see that in fact it gets pulled out of the eventual event payload. Same with the sampling data.

If we didn't _getNewTracestate() could become private.

Is there any advantage to that over it being protected (which it is currently)?

Copy link

@RaduW RaduW Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Again it was just a nit, but in general I find protected members to be a sign of a messy abstraction. It is something that you think you shouldn't expose ( otherwise you would make it public) but have to expose it "a little" because derived classes need to mess with it.

const transaction: Event = {
contexts: {
trace: this.getTraceContext(),
Expand Down
68 changes: 3 additions & 65 deletions packages/tracing/test/hub.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable @typescript-eslint/unbound-method */
import { BrowserClient, init as initSDK } from '@sentry/browser';
import { getCurrentHub, Hub } from '@sentry/hub';
import { BrowserClient } from '@sentry/browser';
import { Hub } from '@sentry/hub';
import * as hubModule from '@sentry/hub';
import { TransactionSamplingMethod } from '@sentry/types';
import * as utilsModule from '@sentry/utils'; // for mocking
Expand All @@ -9,7 +9,7 @@ import { logger } from '@sentry/utils';
import { BrowserTracing } from '../src/browser/browsertracing';
import { addExtensionMethods } from '../src/hubextensions';
import { Transaction } from '../src/transaction';
import { computeTracestateValue, extractSentrytraceData, SENTRY_TRACE_REGEX } from '../src/utils';
import { extractSentrytraceData, SENTRY_TRACE_REGEX } from '../src/utils';
import { addDOMPropertiesToGlobal, getSymbolObjectKeyByName, testOnlyIfNodeVersionAtLeast } from './testutils';

addExtensionMethods();
Expand Down Expand Up @@ -50,68 +50,6 @@ describe('Hub', () => {

expect(transaction).toEqual(expect.objectContaining(transactionContext));
});

it('creates a new tracestate value if no tracestate data in transaction context', () => {
const transaction = hub.startTransaction({ name: 'FETCH /ball' });

const b64Value = computeTracestateValue({
traceId: transaction.traceId,
environment: 'dogpark',
release: 'off.leash.trail',
publicKey: 'dogsarebadatkeepingsecrets',
});

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

it('creates a new tracestate value if tracestate data in transaction context only contains third party data', () => {
const transactionContext = {
name: 'FETCH /ball',
traceId: '12312012123120121231201212312012',
parentSpanId: '1121201211212012',
metadata: { tracestate: { thirdparty: 'maisey=silly;charlie=goofy' } },
};

const transaction = hub.startTransaction(transactionContext);

const b64Value = computeTracestateValue({
traceId: transaction.traceId,
environment: 'dogpark',
release: 'off.leash.trail',
publicKey: 'dogsarebadatkeepingsecrets',
});

expect(transaction).toEqual(
expect.objectContaining({
metadata: {
tracestate: {
// a new value for `sentry` is created
sentry: `sentry=${b64Value}`,
// the third-party value isn't lost
thirdparty: 'maisey=silly;charlie=goofy',
},
},
}),
);
});

it('uses default environment if none given', () => {
const release = 'off.leash.park';
initSDK({
dsn: 'https://[email protected]/12312012',
release,
});
const transaction = getCurrentHub().startTransaction({ name: 'FETCH /ball' });

const b64Value = computeTracestateValue({
traceId: transaction.traceId,
environment: 'production',
release,
publicKey: 'dogsarebadatkeepingsecrets',
});

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

describe('getTransaction()', () => {
Expand Down