diff --git a/domains/observability/knowledge/span-sub-sampling.md b/domains/observability/knowledge/span-sub-sampling.md index 31d29d27..80fac4f4 100644 --- a/domains/observability/knowledge/span-sub-sampling.md +++ b/domains/observability/knowledge/span-sub-sampling.md @@ -77,4 +77,6 @@ Removing ~90% of volume before the sample multiplies headroom — a higher sub-r ## Kill Switch -Ship every always-on span family with an env disable flag (PR #39891: `SENTRY_DISTRIBUTED_TRACING_DISABLED` returns the messenger un-wrapped). It turns a future emergency cut into a config flip instead of a cherry-pick. +**For anything the sampler sees, the per-name rate is the kill switch** — a remote name-to-rate map pins a transaction to zero at the next Sentry init, on every build that reads the flag, with no release. Do not add a bespoke env flag to get that. + +An env flag still earns its place for what the rate cannot reach. A span handed an explicit parent is a child span, and no `tracesSampler` call is made for it. And a flag can remove the wrapping itself rather than sample its output: PR #39891's `SENTRY_DISTRIBUTED_TRACING_DISABLED` returns the messenger un-wrapped, so the hot path pays nothing at all — a different thing from emitting fewer spans. diff --git a/domains/observability/skills/sentry-quota/repos/metamask-extension.md b/domains/observability/skills/sentry-quota/repos/metamask-extension.md index 79b13a2b..219ca540 100644 --- a/domains/observability/skills/sentry-quota/repos/metamask-extension.md +++ b/domains/observability/skills/sentry-quota/repos/metamask-extension.md @@ -52,6 +52,9 @@ gh pr diff --repo MetaMask/metamask-extension \ - **Gate location differs by repo.** Extension spans go through `trace()` — gate at the call site or in the wrapper. Core controller spans go through the injected callback — gate in the package's trace util or the callback so every consumer (extension, mobile) inherits the cap. - **`BackgroundRpc` / `MessengerCall`** (the `TraceName` tail) are the already-gated wrapper spans from [PR #39891](https://github.com/MetaMask/metamask-extension/pull/39891) — the reference implementation of the Tier-2 sub-sample pattern and the `SENTRY_DISTRIBUTED_TRACING_DISABLED` kill-switch. - **Tier-0 fix path is a core PR + a patch on the extension release branch.** Controller instrumentation originates in `MetaMask/core`; the release branch is where the cherry-pick lands. The sev-1 blocker goes on the in-flight release milestone — e.g. [issue #43211](https://github.com/MetaMask/metamask-extension/issues/43211) ("Assets Controller Sentry Instrumentation exceeding quota"). +- **Which control the per-name rate is.** `sentry-traces-sampler.ts` resolves a rate in this order: the remote `sentry.transactionSampleRates` map, then `DEFAULT_TRANSACTION_SAMPLE_RATES`, then the parent's decision, then the default — and every non-zero result is capped by the remote `sentry.tracesSampleRate`, which doubles as the ceiling. A per-name entry pins its rate regardless of the parent, so a throttled name cannot ride in on a sampled parent. `AssetsDataSourceTiming` and `AssetsUpdatePipeline` are seeded at `0` and are the worked example. +- **What the remote map does not reach.** `sentry-remote-rates.ts` reads the flag from persisted `RemoteFeatureFlagController` state (merged with `_flags.remoteFeatureFlags.sentry` manifest overrides) **once at Sentry init**, so a published rate takes effect at the next init, not mid-session — and only in a build that ships this module. Adding a *name* inside `transactionSampleRates` is a value change; adding a new top-level rate key needs the LaunchDarkly `variationJsonSchema` updated first, since it is written with `additionalProperties: false`. +- **Which spans the sampler sees.** `trace()` sets `forceTransaction` only when the request carries no `parentContext` and a span is already active, so a `trace()` call given an explicit parent stays a child span and no `tracesSampler` call is made for it. Those are the spans for which the call-site gate is the only control. [MetaMask-planning#7546](https://github.com/MetaMask/MetaMask-planning/issues/7546) (make `parentContext` explicit) removes that ambient fallback, so a span declaring no parent will root its own trace instead of grafting onto whatever was active — still a transaction the sampler sees, but drawing its own decision at its own rate rather than inheriting a parent's. Re-read this bullet when it lands. `shouldSampleWrappers` also reads a remote rate (`sentry.wrapperSampleRate`), so the gate is tunable without a release as well; what it buys over a per-name rate is per-trace coherence and never building the span. - **Spotting the culprit first:** `sentry-mcp-queries` → Volume Estimation (`span.op` aggregate `count()`) ranks span contributors by span count; this skill takes over once you have the offending span name. A span-count ranking is not a ranking by billed volume: retention differs per name, so no single factor rescales it, and on a plan metered in transactions it can invert. ## Requests Already Recorded as `http.client` Spans diff --git a/domains/observability/skills/sentry-quota/skill.md b/domains/observability/skills/sentry-quota/skill.md index 8ec3363d..d2eb467e 100644 --- a/domains/observability/skills/sentry-quota/skill.md +++ b/domains/observability/skills/sentry-quota/skill.md @@ -1,7 +1,7 @@ --- maturity: experimental name: sentry-quota -description: Catch quota-risky Sentry span instrumentation in code and PRs — fan-out × ungated × no-kill-switch — before it blows the span budget +description: Catch quota-risky Sentry span instrumentation in code and PRs — fan-out × ungated × not in the sampler's rate table — before it blows the span budget --- # Sentry Span Quota Guard @@ -19,7 +19,7 @@ Find and fix custom Sentry span instrumentation that blows the project span budg - Reading the live span counts themselves — that's `sentry-mcp-queries` (Volume Estimation). - Product-analytics events (Segment / `trackEvent`) — that's `instrumentation`, with data domain `knowledge/segment-governance.md` for Segment governance. -- The span is already behind a per-trace sample gate **and** a kill-switch — already mitigated. +- The span is already named in the sampler's rate table and, where it is a span family on a hot path, gated at the call site — already mitigated. - Error volume. Errors are metered separately from spans and transactions, so no change here moves the error quota. ## Breach Triad @@ -30,7 +30,7 @@ A custom span is a quota risk when these stack. The first three together are the |---|---|---| | **Fan-out** | span created in a loop / `.map` / `.forEach` / per-asset / per-account / per-chain / poller, or a `trace()` callback that awaits requests | N spans per trace, not 1. Each request inside an active span is an automatic `http.client` child | | **Always-on** | no `tracesSampleRate` sub-rate, no hash gate before the span | every qualifying call emits | -| **No kill-switch** | not guarded by an env flag | disabling needs a release, not a config flip | +| **No per-name rate** | the transaction name is absent from the sampler's rate table | cutting it needs a release, because a runtime throttle can only act on a name it holds | | Hot path | data-source / update-pipeline / network callback, not a discrete user action | high call frequency | Low fan-out + discrete user action + already gated = fine. Don't flag healthy spans. @@ -60,11 +60,60 @@ Low fan-out + discrete user action + already gated = fine. Don't flag healthy sp ### Mitigate Pick the lowest tier that stops the bleed. +## PR Report + +What the review emits, not a form the author fills in. Every field but the last is derivable from the diff plus one Sentry query; the last is the author's, and it is the term the estimate is most sensitive to. + +| # | Field | Source | +|---|---|---| +| 1 | Names this diff adds or changes | diff | +| 2 | What starts each one, and what stops it | diff + repo | +| 3 | Every span that will be a child of it | diff + SDK config | +| 4 | What this measures that those spans do not | field 3 | +| 5 | Whether the names already emit | one query, 90 days | +| 6 | Which rate resolves, and where it lives | sampler source | +| 7 | Whether a call-site gate is warranted | fields 2 and 3 | +| 8 | Attributes, and the cardinality of each dynamic key | diff | +| 9 | Reach | **the author** | + +1. **Names.** Every new `TraceName`, every new `trace(` site, and any `trace` passed as an *argument* into an existing call — the third has no `trace(` in the diff to find. +2. **Triggers.** What starts it and what stops it: a discrete user action, a page load, a poller, a selector, a loop. Follow mounts, subscriptions and inits, not only `trace(` sites, because a change can start a traced process with no span anywhere in its diff. "Nothing stops it" is a finding, not a blank. +3. **Children, enumerated rather than described.** A `trace()` callback holds its span active, so every request inside becomes an `http.client` child except those `shouldCreateSpanForRequest` drops, plus any nested traced call (`knowledge/auto-instrumentation.md`). This list is what makes one span more than one span, and it is what field 4 is answered against. +4. **What it adds.** Where the new span wraps a request that already carries an `http.client` span with a duration, say what the new name measures that the existing one does not. A timing span over an interval already recorded is a rename of existing data. +5. **Already emitting** — workflow step 2. Lead the verdict with the current count. +6. **Rate resolution**, in order: a remote per-name rate, then a build-time per-name rate, then the enclosing trace's decision, then the global default. **A span started inside a sampled trace inherits that decision and the global default never applies.** Say whether the per-name entry is in *this* PR; a rate that ships in a later ref does not cap this one. +7. **Gate** — see the next section. It is not a question about a kill switch. +8. **Attributes.** Span `data`, not scope tags. For every dynamic key, the number of distinct values it can take. +9. **Reach.** The share of sessions that reach the trigger. No static analysis reaches it, so the estimate ships as a range across it with the assumption written down, never folded into a single number. + +The report closes on + +``` +spans = occurrences x spans per occurrence x the rate that applies +occurrences = reach x frequency x duration +``` + +with spans per occurrence from field 3 and the rate from field 6. + +## Which Control To Ask For + +**The per-name rate is the kill switch.** A remote name-to-rate map pins any transaction to zero at runtime, with no release, and it wins over the build-time table. So do not ask an author to build an env flag — ask that the name ship in the rate table alongside the code that emits it. Two limits belong in the report: + +- **It reaches only builds that read it.** A rate published today does nothing for an installed build shipped before the sampler, which is why the release inbound filter (Tier 1) is a separate instrument rather than a redundant one. +- **It only sees transactions.** A span handed an explicit parent is a child span and the sampler is never called for it, so there the call-site gate is the only control. + +**A call-site gate is for the two things a rate cannot do**, and asking for both by reflex is how a review spends its credit on redundancy: + +- **Per-trace coherence.** The sampler draws independently per name, so a per-name rate leaves a trace showing some of its spans and not others. A deterministic hash on the trace id keeps a whole family together, which is what makes the waterfall readable (`knowledge/span-sub-sampling.md`). Never `Math.random()` per span. +- **Not creating the span at all**, rather than dropping it at send — the difference that matters on a hot path where building the span is itself the cost. + +Warranted for a span family on a hot path. Not warranted for a single named transaction the rate table can cap. + ## Mitigation Ladder | Tier | When | Action | |---|---|---| -| **0 — Immediate** | a span fans out and is actively breaching on the live release | disable the `trace()` call at source (or env-guard it) + **cherry-pick to the release branch** + file a sev-1 release blocker on the in-flight release milestone | +| **0 — Immediate** | a span fans out and is actively breaching on the live release | publish the name at rate `0` in the remote per-name rate map — no release, effective at the next Sentry init on every build that reads the flag. Falls back to disabling the `trace()` call at source and **cherry-picking to the release branch** for a child span the sampler never sees, or for builds predating the sampler. Either way, file a sev-1 release blocker on the in-flight release milestone | | **1 — Release containment** | spike concentrated in an old, already-patched release with lingering users. A sampler fix in a newer build does not change that release's rates unless it reads its rate remotely | Sentry **inbound filter** dropping `release:` spans + force-update. The only dashboard action. Filters target a whole release, not one span — don't filter a release you still want data from. There is no inbound filter by transaction name, only a fixed health-check one. Filtered events do not consume quota, so confirm the drop in the filtered outcomes (`stats_v2` grouped by `reason`), not in Explore. It is not instant: one recorded release filter took 3.8 days from filing to taking effect | | **2 — Durable** | the span is justified long-term but ungated | deterministic `traceId`-hash sub-sample gate before the span (`span-sub-sampling`) | | **3 — Wrong tool** | the metric needs full fidelity; sampling loses the signal | move the metric off trace spans — they are the wrong substrate for always-on high-cardinality metrics. Segment is the usual target, but its events can ship unregistered, with no CI check and no billing review (data domain `knowledge/segment-governance.md`), so it is not a free lunch | @@ -83,7 +132,8 @@ Tier 0 + 1 stop the bleed; Tier 2 is the follow-up so the metric returns. | "No grep hits, so it's safe" | The culprit may be on a release ref not checked out — verify the version/ref | | Disable the span on `main` only | Cherry-pick to the active release branch — `main` alone leaves the live release breaching | | Treat "move to Segment" as free | Segment events ship without CI governance or billing review (data domain `knowledge/segment-governance.md`) | -| Ship new always-on instrumentation with no kill-switch | Add an env disable flag on day one — turns a future cut into a config flip, not a cherry-pick | +| Ship a new transaction whose name is not in the sampler's rate table | The name is the unit a runtime throttle acts on. In the table on day one, a later cut is a flag value; absent from it, a cut is a release | +| Ask the author for a bespoke env kill-switch | The per-name rate is the kill switch. Ask for a call-site gate only where per-trace coherence, or not building the span at all, is the point | | An optional `trace?` param passes review because it emits nothing | It is a dormant fan-out — it detonates when any caller supplies the argument. Remove the *param*, not just the argument, so one line can't re-arm it. | | Disable one entry point of a multi-path change | One change can reach the backend by more than one path (a controller callback *and* a selector param). Audit every entry point it added, not just the one that fired. | | Read a span that "fires N million times" as one triggered too often | A total is transactions × spans per transaction. Check spans per trace before blaming the trigger: fan-out multiplies the count with no change in how often the trigger fires |