-
Notifications
You must be signed in to change notification settings - Fork 142
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
✨ [RUM-3542] Add trace context injection control in rum configuration #2656
Conversation
Bundles Sizes Evolution
|
if (configuration.traceContextInjection === TraceContextInjection.All) { | ||
inject(makeTracingHeaders(context.traceId, context.spanId, context.traceSampled, tracingOption.propagatorTypes)) | ||
} |
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.
FMU, the check should more look like this
if (
(context.traceSampled && configuration.traceContextInjection === TraceContextInjection.Sampled) ||
configuration.traceContextInjection === TraceContextInjection.All
) {
inject(makeTracingHeaders(context.traceId, context.spanId, context.traceSampled, tracingOption.propagatorTypes))
}
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.
Thank you! I updated this and added tests accordingly.
export const enum TraceContextInjection { | ||
/* | ||
Default. Inject trace context to all network requests as per the sampling rate. | ||
*/ | ||
All, | ||
|
||
/* | ||
Inject trace context only if the trace is sampled. | ||
*/ | ||
Sampled, | ||
} |
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.
💬 suggestion: user-facing enums should be defined as a plain object, like so:
browser-sdk/packages/core/src/domain/configuration/configuration.ts
Lines 17 to 22 in c52216b
export const DefaultPrivacyLevel = { | |
ALLOW: 'allow', | |
MASK: 'mask', | |
MASK_USER_INPUT: 'mask-user-input', | |
} as const | |
export type DefaultPrivacyLevel = (typeof DefaultPrivacyLevel)[keyof typeof DefaultPrivacyLevel] |
This allows using either the object (ex: TraceContextInjection.All
) or the value directly (ex: "all"
) without facing typing issues, from JS or TS.
In any case, const enum
should never be customer-facing because non-TS users won't have access to it.
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.
Got it. I updated it with an object like this and moved it to the same configuration file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2656 +/- ##
==========================================
- Coverage 92.91% 92.90% -0.01%
==========================================
Files 239 239
Lines 6934 6939 +5
Branches 1517 1520 +3
==========================================
+ Hits 6443 6447 +4
- Misses 491 492 +1 ☔ View full report in Codecov by Sentry. |
/to-staging |
🚂 Branch Integration: starting soon, merge in < 10m Commit ec7d3c5825 will soon be integrated into staging-12. This build is going to start soon! (estimated merge in less than 10m) Use |
…l into staging-12 Co-authored-by: cy-moi <[email protected]>
🚂 Branch Integration: This commit was successfully integrated Commit ec7d3c5825 has been merged into staging-12 in merge commit a155b4d01e. Check out the triggered pipeline on Gitlab 🦊 |
Co-authored-by: Aymeric <[email protected]>
Co-authored-by: Aymeric <[email protected]>
Co-authored-by: Aymeric <[email protected]>
Co-authored-by: Aymeric <[email protected]>
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.
Nice!
@cy-moi Any idea if this would support when only using W3C propagatorTypes?
From what I can tell, it might not be. Can you confirm? |
Motivation
APM is working on building more granular sampling control on the backend side which can't be applied if the sampling decision is already taken on the client side.
Changes
Added a field
traceContextInjection
in the configuration to control context injection.This new configuration allows more granular sampling at the backend and does not bring any break changes. It is available along with sampling rate configuration.
traceContextInjection
is an opt-in configuration defaults to "all" which means no behavior changes. If opt-in for the "sampled" option, you would see only sampled requests having trace context attached to them.Testing
I have gone over the contributing documentation.