-
-
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
Conversation
size-limit report 📦
|
packages/core/src/tracing/trace.ts
Outdated
const incomingDsc = baggageHeaderToDynamicSamplingContext(baggage); | ||
const baggageOrgId = incomingDsc?.org_id; | ||
|
||
let sdkOrgId: string | undefined; |
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.
m: I think we should use the same logic here as for injecting the org id, which is use the specified one if defined, else extract it from dsn? We may also put this into a reusable utility, I suppose!
packages/core/src/tracing/trace.ts
Outdated
sdkOrgId = extractOrgIdFromDsnHost(dsn.host); | ||
} | ||
|
||
const shouldStartNewTrace = (): boolean => { |
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: Let's maybe make this a separate function that takes arguments instead of inlining this function? No strong feelings, but this is how we usually do this I suppose 🤔
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 no strong opinions about this. Just thought it's not a lot of logic and when inlining this it does not disrupt you from the code-reading flow but it still keeps this part of the code separated :D
# Conflicts: # packages/core/src/index.ts # packages/core/src/tracing/dynamicSamplingContext.ts # packages/core/src/tracing/trace.ts # packages/core/src/utils/dsn.ts # packages/core/src/utils/tracing.ts # packages/core/test/lib/utils/tracing.test.ts
@@ -10,7 +10,7 @@ import { | |||
import type { DynamicSamplingContext } from '../types-hoist/envelope'; | |||
import type { Span } from '../types-hoist/span'; | |||
import { baggageHeaderToDynamicSamplingContext, dynamicSamplingContextToSentryBaggageHeader } from '../utils/baggage'; | |||
import { extractOrgIdFromDsnHost } from '../utils/dsn'; | |||
import { deriveOrgIdFromClient } from '../utils/dsn'; |
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 as extract
...? 😅
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?
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-b6e0f71bb87e888ddb45cd78dcf2cb7efdf9c80b6d596c0224648b1f78bdcb29R148
The one function is doing extractOrIdFromDsnHost
, which is really just looking at the host
string and extracting it from there. And deriveOrgIdFromClient
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
or getXX
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 👍
packages/core/src/utils/dsn.ts
Outdated
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
l: I would re-organize this a bit and either:
- Enforce that
client
is not undefined - imho this is likely the nicer API, and then check at call-site if the client is undefined. - Or else check at the top of the function if client is defined and return
undefined
early, avoiding repeated optional chaining below.
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.
re 1: it is a bit weird to have an API deriveXXFromY
and allow no Y
to be passed in :D
packages/core/src/utils/tracing.ts
Outdated
* 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 comment
The 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?
packages/core/src/utils/dsn.ts
Outdated
|
||
let org_id: string | undefined; | ||
|
||
if (options?.orgId) { |
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.
if (options?.orgId) { | |
if (options.orgId) { |
I think this always exists now?
packages/core/src/utils/tracing.ts
Outdated
return false; | ||
} | ||
|
||
const strictTraceContinuation = client.getOptions()?.strictTraceContinuation || false; // default for `strictTraceContinuation` is `false` |
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.
const strictTraceContinuation = client.getOptions()?.strictTraceContinuation || false; // default for `strictTraceContinuation` is `false` | |
const strictTraceContinuation = client.getOptions().strictTraceContinuation || false; // default for `strictTraceContinuation` is `false` |
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.
small nits but overall looks great! 👍
## DESCRIBE YOUR PR Docs for `strictTraceContinuation` and `orgId` ([related PR](getsentry/sentry-javascript#16313)). I'm not 100% sure where I should put the `orgId` as it does not 100% fit into the "Tracing Options" section but it's currently only used for tracing. closes getsentry/sentry-javascript#16308 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [ ] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs)
## DESCRIBE YOUR PR Docs for `strictTraceContinuation` and `orgId` ([related PR](getsentry/sentry-javascript#16313)). I'm not 100% sure where I should put the `orgId` as it does not 100% fit into the "Tracing Options" section but it's currently only used for tracing. closes getsentry/sentry-javascript#16308 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [ ] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs)
This implements
strictTraceContinuation
based on those docs: https://develop.sentry.dev/sdk/telemetry/traces/#stricttracecontinuationcloses #16291