feat(controlplane): always sample batch subgraph publish traces - #2957
Conversation
Batch publishes (PublishFederatedSubgraphs) are rare, slow, and frequently hit the request timeout, but the flat SENTRY_TRACES_SAMPLE_RATE drops most of the successful ones so their end-to-end composition cost can't be tracked. Add a tracesSampler that forces 100% sampling for the batch publish RPC and falls back to the configured rate for everything else. Child composition spans inherit the parent decision, and upstream sampling decisions are preserved.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSentry distributed-tracing sampling is now customized via a new ChangesSentry tracing sampling customization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/src/core/sentry.config.ts`:
- Around line 28-30: The current check using ctx.parentSampled wrongly returns
SENTRY_TRACES_SAMPLE_RATE when ctx.parentSampled is false; update the sampling
decision so that if typeof ctx.parentSampled === 'boolean' you return 1 when
ctx.parentSampled is true and 0 when false (do not fall back to
SENTRY_TRACES_SAMPLE_RATE), i.e. honor ctx.parentSampled exactly; update the
branch that references ctx.parentSampled and SENTRY_TRACES_SAMPLE_RATE
accordingly so child spans are not sampled when parentSampled === false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 117d1f6e-006b-4eee-8e49-b21addcf17ec
📒 Files selected for processing (1)
controlplane/src/core/sentry.config.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2957 +/- ##
===========================================
+ Coverage 41.64% 65.63% +23.99%
===========================================
Files 1053 329 -724
Lines 133865 47321 -86544
Branches 6472 5300 -1172
===========================================
- Hits 55742 31059 -24683
+ Misses 76332 16238 -60094
+ Partials 1791 24 -1767
🚀 New features to boost your workflow:
|
When an upstream span opted out of sampling (parentSampled === false), the sampler fell back to SENTRY_TRACES_SAMPLE_RATE instead of 0, which can leave orphaned child spans in a trace whose root was not sampled. Check the always-sample paths first, then honor parentSampled exactly (true -> 1, false -> 0), and only fall back to the configured rate when there is no upstream decision.
comatory
left a comment
There was a problem hiding this comment.
I am worried that this code is in the hot path and I'd try to optimize here. Maybe I'm paranoid? I know we can't be sure if we don't benchmark but still... wDYT?
Problem
Batch subgraph publishes go through the
PublishFederatedSubgraphsRPC, which is rare, slow, and frequently hits the ~90s request timeout. In prod,SENTRY_TRACES_SAMPLE_RATEis below 1.0, so the flattracesSampleRatedrops most successful batch publishes — only the timed-out ones reliably show up in Sentry. That makes it hard to track the end-to-end composition cost of a publish over time.Change
Replace the flat
tracesSampleRatewith atracesSamplerthat:/wg.cosmo.platform.v1.PlatformService/PublishFederatedSubgraphs), matched against the span name /http.route/http.target/url.path/url.full;SENTRY_TRACES_SAMPLE_RATEfor everything else (no change for other transactions);parentSampled) so distributed traces stay intact.Child composition spans (e.g.
ComposeGraphsWorker.composeGraphsInWorker) inherit the parent's sampling decision, so the full trace — including per-federated-graph compose timings — is captured.The forced-sample list is a single constant (
ALWAYS_SAMPLE_PATHS), easy to extend to other operations later.Notes
tracesSamplertakes precedence overtracesSampleRate; the sampler returns the configured rate as its default, so overall sampling behaviour is unchanged except for the batch publish path.Summary by CodeRabbit