-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
d955dba
f70e334
b4e8b8a
628d33a
2124f20
1df7bfb
1ee03f3
29a8e02
dd45062
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 = {}; | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :< There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like that https://lodash.com/docs/4.17.15#get There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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 }; | ||
} | ||
|
||
/** | ||
|
@@ -118,8 +119,6 @@ export class Transaction extends SpanClass implements TransactionInterface { | |
}).endTimestamp; | ||
} | ||
|
||
this._metadata.tracestate = this.tracestate; | ||
|
||
const transaction: Event = { | ||
contexts: { | ||
trace: this.getTraceContext(), | ||
|
@@ -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; | ||
|
@@ -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! }); | ||
} | ||
} |
There was a problem hiding this comment.
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 notracestate.sentry
which might be confusing to the user.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 notracestate.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
!
.