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
lobsterkatie 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 all commits
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
13 changes: 8 additions & 5 deletions packages/core/test/lib/request.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,7 +28,7 @@ describe('eventToSentryRequest', () => {
environment: 'dogpark',
event_id: '0908201304152013',
release: 'off.leash.park',
user: { id: '1121', username: 'CharlieDog', ip_address: '11.21.20.12' },
user: { id: '1121', username: 'CharlieDog', ip_address: '11.21.20.12', segment: 'bigs' },
};

describe('error/message events', () => {
Expand DownExpand Up@@ -65,13 +65,15 @@ describe('eventToSentryRequest', () => {
// computeTracestateValue({
// trace_id: '1231201211212012',
// environment: 'dogpark',
// release: 'off.leash.park',
// public_key: 'dogsarebadatkeepingsecrets',
// release: 'off.leash.park',
// user: { id: '1121', segment: 'bigs' },
// }),
tracestate: {
sentry:
'sentry=eyJ0cmFjZV9pZCI6IjEyMzEyMDEyMTEyMTIwMTIiLCJlbnZpcm9ubWVudCI6ImRvZ3BhcmsiLCJyZWxlYXNlIjoib2ZmLmxlYXNo' +
'LnBhcmsiLCJwdWJsaWNfa2V5IjoiZG9nc2FyZWJhZGF0a2VlcGluZ3NlY3JldHMifQ',
'sentry=eyJ0cmFjZV9pZCI6IjEyMzEyMDEyMTEyMTIwMTIiLCJlbnZpcm9ubWVudCI6ImRvZ3BhcmsiLCJwdWJsaWNfa2V5Ijo' +
'iZG9nc2FyZWJhZGF0a2VlcGluZ3NlY3JldHMiLCJyZWxlYXNlIjoib2ZmLmxlYXNoLnBhcmsiLCJ1c2VyIjp7ImlkIjoiMTEyM' +
'SIsInNlZ21lbnQiOiJiaWdzIn19',
},
},
spans: [],
Expand DownExpand Up@@ -160,10 +162,11 @@ describe('eventToSentryRequest', () => {

expect(envelope.envelopeHeader.trace).toBeDefined();
expect(envelope.envelopeHeader.trace).toEqual({
trace_id: '1231201211212012',
environment: 'dogpark',
public_key: 'dogsarebadatkeepingsecrets',
release: 'off.leash.park',
trace_id: '1231201211212012',
user: { id: '1121', segment: 'bigs' },
});
});
});
Expand Down
26 changes: 20 additions & 6 deletions packages/tracing/src/span.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
/* eslint-disable max-lines */
import { getCurrentHub } from '@sentry/hub';
import { Hub, Primitive, Span as SpanInterface, SpanContext, TraceHeaders, Transaction } from '@sentry/types';
import { getCurrentHub, Hub } from '@sentry/hub';
import { Primitive, Span as SpanInterface, SpanContext, TraceHeaders, Transaction } from '@sentry/types';
Comment on lines +2 to +3

@RaduWRaduWMar 24, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What's going on here ? Doesn't seem to be related to the PR.

@lobsterkatielobsterkatieMar 24, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It actually is, though. The change here is from importing the type to importing the class. Later on in the file I'm calling Hub.getScope(), a method which exists on the class but isn't in the type, so if I don't make this change, it won't compile. The alternative to doing this is to add the method to the type, which I'm happy to do instead if you think that's better.

@RaduWRaduWMar 26, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Understood, it's probably me (I'm not so familiar with this code) but it further raises the question why we don't have getScope in the interface ( since we do have pushScope, popScope, withScope).

Anyway, not the right place to discuss general architecture of the API.

import { dropUndefinedKeys, logger, timestampWithMs, uuid4 } from '@sentry/utils';

import { SpanStatus } from './spanstatus';
Expand DownExpand 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 DownExpand Up@@ -357,8 +365,11 @@ export class Span implements SpanInterface {
*
* @returns The new Sentry tracestate entry, or undefined if there's no client or no dsn
*/
protected _getNewTracestate(hub: Hub = getCurrentHub()): string | undefined {
protected _getNewTracestate(): string | undefined {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
const hub = ((this.transaction as any)?._hub as Hub) || getCurrentHub();
const client = hub.getClient();
const { id: userId, segment: userSegment } = hub.getScope()?.getUser() || {};
const dsn = client?.getDsn();

if (!client || !dsn) {
Expand All@@ -372,11 +383,12 @@ export class Span implements SpanInterface {
// `dsn.publicKey` required and remove the `!`.

return `sentry=${computeTracestateValue({
traceId: this.traceId,
trace_id: this.traceId,
environment,
release,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
publicKey: dsn.publicKey!,
public_key: dsn.publicKey!,
user: { id: userId, segment: userSegment },
})}`;
}

Expand All@@ -392,9 +404,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, create 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 numberDiff line numberDiff 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 DownExpand 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

@RaduWRaduWMar 24, 2021

Copy link
Copy Markdown

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.

@lobsterkatielobsterkatieMar 24, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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)?

@RaduWRaduWMar 26, 2021•
edited
Loading

Copy link
Copy Markdown

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
9 changes: 6 additions & 3 deletions packages/tracing/src/utils.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -145,10 +145,11 @@ export function secToMs(time: number): number {
export { stripUrlQueryAndFragment } from '@sentry/utils';

type SentryTracestateData = {
traceId: string;
trace_id: string;
environment: string | undefined | null;
release: string | undefined | null;
publicKey: string;
public_key: string;
user: { id: string | undefined | null; segment: string | undefined | null };
Comment thread
RaduW marked this conversation as resolved.
};

/**
Expand All@@ -159,9 +160,11 @@ type SentryTracestateData = {
*/
export function computeTracestateValue(data: SentryTracestateData): string {
// `JSON.stringify` will drop keys with undefined values, but not ones with null values, so this prevents
// `environment` and `release` from being dropped if they haven't been set by `Sentry.init`
// these values from being dropped if they haven't been set by `Sentry.init`
data.environment = data.environment || null;
data.release = data.release || null;
data.user.id = data.user.id || null;
data.user.segment = data.user.segment || 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 Down
Loading