-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Implement strictTraceContinuation
#16313
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 11 commits
33385ff
e2dd6a6
cd60e47
484aedf
7a60aa3
452c353
08569aa
d4e5cc4
780720e
20b67c6
d8ec911
8e43a3f
78023e3
e70507e
3b51968
19c5afc
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,3 +1,4 @@ | ||||||
import type { Client } from '../client'; | ||||||
import { DEBUG_BUILD } from '../debug-build'; | ||||||
import type { DsnComponents, DsnLike, DsnProtocol } from '../types-hoist/dsn'; | ||||||
import { consoleSandbox, debug } from './debug-logger'; | ||||||
|
@@ -129,6 +130,27 @@ export function extractOrgIdFromDsnHost(host: string): string | undefined { | |||||
return match?.[1]; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Returns the organization ID of the client. | ||||||
* | ||||||
* The organization ID is extracted from the DSN. If the client options include a `orgId`, this will always take precedence. | ||||||
*/ | ||||||
export function deriveOrgIdFromClient(client: Client | undefined): string | undefined { | ||||||
const options = client?.getOptions(); | ||||||
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. l: I would re-organize this a bit and either:
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. re 1: it is a bit weird to have an API |
||||||
|
||||||
const { host } = client?.getDsn() || {}; | ||||||
|
||||||
let org_id: string | undefined; | ||||||
|
||||||
if (options?.orgId) { | ||||||
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.
Suggested change
I think this always exists now? |
||||||
org_id = String(options.orgId); | ||||||
} else if (host) { | ||||||
org_id = extractOrgIdFromDsnHost(host); | ||||||
} | ||||||
|
||||||
return org_id; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Creates a valid Sentry Dsn object, identifying a Sentry instance and project. | ||||||
* @returns a valid DsnComponents object or `undefined` if @param from is an invalid DSN source | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
import type { Client } from '../client'; | ||
import type { DynamicSamplingContext } from '../types-hoist/envelope'; | ||
import type { PropagationContext } from '../types-hoist/tracing'; | ||
import type { TraceparentData } from '../types-hoist/transaction'; | ||
import { debug } from '../utils/debug-logger'; | ||
import { baggageHeaderToDynamicSamplingContext } from './baggage'; | ||
import { deriveOrgIdFromClient } from './dsn'; | ||
import { parseSampleRate } from './parseSampleRate'; | ||
import { generateSpanId, generateTraceId } from './propagationContext'; | ||
|
||
|
@@ -124,3 +127,38 @@ function getSampleRandFromTraceparentAndDsc( | |
return Math.random(); | ||
} | ||
} | ||
|
||
/** | ||
* Determines whether a new trace should be continued based on the provided client and baggage org ID. | ||
* If the trace should not be continued, a new trace will be started. | ||
* | ||
* The result is dependent on the `strictTraceContinuation` option in the client. | ||
* See https://develop.sentry.dev/sdk/telemetry/traces/#stricttracecontinuation | ||
*/ | ||
export function shouldContinueTrace(client: Client | undefined, baggageOrgId?: string): boolean { | ||
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. Also here, do we need to allow empty client? IMHO we never want to continue a trace if we have no client, so we can probably just check this at call site as well? |
||
const sdkOptionOrgId = deriveOrgIdFromClient(client); | ||
|
||
// Case: baggage orgID and SDK orgID don't match - always start new trace | ||
if (baggageOrgId && sdkOptionOrgId && baggageOrgId !== sdkOptionOrgId) { | ||
debug.log( | ||
`Starting a new trace because org IDs don't match (incoming baggage: ${baggageOrgId}, SDK options: ${sdkOptionOrgId})`, | ||
); | ||
return false; | ||
} | ||
|
||
const strictTraceContinuation = client?.getOptions()?.strictTraceContinuation || false; // default for `strictTraceContinuation` is `false` | ||
|
||
if (strictTraceContinuation) { | ||
// With strict continuation enabled, start new trace if: | ||
// - Baggage has orgID, but SDK doesn't have one | ||
// - SDK has orgID, but baggage doesn't have one | ||
if ((baggageOrgId && !sdkOptionOrgId) || (!baggageOrgId && sdkOptionOrgId)) { | ||
debug.log( | ||
`Starting a new trace because strict trace continuation is enabled and one org ID is missing (incoming baggage: ${baggageOrgId}, SDK options: ${sdkOptionOrgId})`, | ||
); | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} |
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.
l: why did we change this? IMHO
derive
is not easier to understand/grasp asextract
...? 😅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.
with change, I mean re-word :D I would just go with
extractOrgIdFromClient
or something like this?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.
It's a different function :D
I extracted the logic of getting the org ID.
extractOrgIdFromDsnHost
is still available: https://github.com/getsentry/sentry-javascript/pull/16313/files#diff-b6e0f71bb87e888ddb45cd78dcf2cb7efdf9c80b6d596c0224648b1f78bdcb29R148The one function is doing
extractOrIdFromDsnHost
, which is really just looking at thehost
string and extracting it from there. AndderiveOrgIdFromClient
is looking at different cues (either the org ID or the host on the client) to get the org ID.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.
yeah, right, that makes sense of course - what I mean is that I think
derive
is a non-ideal term/prefix for the function name :D we do not use it anywhere in the code, rather we always use e.g.extractXX
orgetXX
or something like this, so I'd rather use something along these lines for consistency - it is a different method of course :)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.
ah got you, that makes sense of course 👍