diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 20217c32..06a7f1e0 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -16,6 +16,9 @@ /domains/assets/ @MetaMask/metamask-assets /domains/coding/ @MetaMask/extension-platform @MetaMask/mobile-platform @MetaMask/core-platform /domains/general/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/observability/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/observability/skills/*/repos/metamask-extension.md @MetaMask/extension-platform +/domains/observability/skills/*/repos/metamask-mobile.md @MetaMask/mobile-platform /domains/performance/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/perps/ @MetaMask/perps /domains/pr-workflow/ @MetaMask/extension-platform @MetaMask/mobile-platform diff --git a/domains/observability/knowledge/span-sub-sampling.md b/domains/observability/knowledge/span-sub-sampling.md new file mode 100644 index 00000000..31d29d27 --- /dev/null +++ b/domains/observability/knowledge/span-sub-sampling.md @@ -0,0 +1,80 @@ +--- +name: span-sub-sampling +domain: observability +description: Deterministic per-trace sub-sampling for high-frequency custom spans — span sub-rate, traceId-hash bucketed +--- + +# Span Sub-Sampling + +Durable fix for a custom span that fans out and eats the span budget. Gate the span with a per-trace sub-rate, keyed on the trace id so every gated span in a trace is kept-or-dropped together. Source: [PR #39891](https://github.com/MetaMask/metamask-extension/pull/39891) (`shared/lib/wrapper-sampling.ts`). + +## Rate Math + +``` +effective rate = sample rate of the trace context the span joins × span sub-rate +``` + +- A span that joins its own realm's head-sampled trace inherits the global `tracesSampleRate` (extension prod: 0.5%), so the sub-rate cuts on top: `0.5% × 1% = 0.005%`. +- A span that joins a context propagated as sampled inherits a rate of 1, not the global rate. The extension background continues every UI context it receives as sampled, so its `rpc.handler` spans are kept at the sub-rate alone unless a per-name override or the remote `sentry.tracesSampleRate` ceiling applies. +- PR #39891 ships a sub-rate of 0.5% (`WRAPPER_SAMPLE_RATE = 0.005`) — a conservative pilot — and names 5% as the step-up once the denylist is confirmed effective in production. + +Pick the sub-rate from how many sampled traces the metric needs to stay useful — not from the quota alone. Too low and the metric goes dark. + +## Pattern + +```ts +const WRAPPER_SAMPLE_RATE = 0.005; + +// Deterministic: same answer for the same traceId, so every gated span in a +// trace is kept or dropped together. The trace's root is sampled separately. +export function shouldSampleWrappers(traceId: string | undefined): boolean { + if (!traceId || traceId.length < 8) { + return false; + } + const hashBucket = parseInt(traceId.slice(0, 8), 16) % 10000; + return hashBucket < WRAPPER_SAMPLE_RATE * 10000; +} +``` + +**Why deterministic, not `Math.random()` per call:** independent per-span sampling shreds a trace into partial waterfalls (some spans present, siblings missing) — useless for attribution. Hashing the trace id makes keep/drop the same for every gated span in the trace. It does not tie them to the trace's root, which head sampling draws on its own: with a 0.5% root rate, about 99.5% of traces that keep wrapper spans are expected to lack their root. + +## Gate Order (cheapest check first) + +```ts +let traceId: string | undefined; +try { + traceId = sentryGetActiveSpan()?.spanContext().traceId; +} catch { + // The span may have ended or be invalid. A tracing failure must not escape + // into the work itself, so fall through with no trace id. +} +if (!traceId || isReadOnlyAction(action) || !shouldSampleWrappers(traceId)) { + return doWorkWithoutSpan(); +} +return trace({ name, op, data }, doWorkWithSpan); +``` + +1. No active trace → no span. +2. Denylist → skip noise (below). +3. Sub-sample miss → skip this trace's spans. + +## Denylist: cut before you sample + +Drop spans with no timing/attribution signal before sub-sampling. In PR #39891, read-only verbs are ~90% of `messenger.call` volume: + +```ts +const READ_ONLY_VERB = /^(?:get|has|find|is|peek)(?:[A-Z]|$)/u; +``` + +The denylist covers `messenger.call` only. `rpc.handler` spans reach the sub-sample gate with no denylist, and 34.9% of them were read-only in one recorded measurement. + +Removing ~90% of volume before the sample multiplies headroom — a higher sub-rate then yields the same span budget, so kept traces are denser and more useful. + +## Where the Gate Goes + +- **Consumer (extension):** spans go through `trace()`. Gate at the call site, or for a whole span family inside the wrapper. `traceId` from `sentryGetActiveSpan()?.spanContext().traceId`. +- **Controller package (core):** controllers call an injected `trace` callback. Gate in the package's trace util or the callback so every consumer inherits the cap. Pull the trace id from the controller's tracing context, not a fresh Sentry import. + +## 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. diff --git a/domains/observability/skills/grafana-tempo-queries/skill.md b/domains/observability/skills/grafana-tempo-queries/skill.md new file mode 100644 index 00000000..b5126de9 --- /dev/null +++ b/domains/observability/skills/grafana-tempo-queries/skill.md @@ -0,0 +1,143 @@ +--- +name: grafana-tempo-queries +description: Query backend traces in Grafana Tempo with TraceQL — find traces by service or span attribute, fetch a trace by id, inspect its span tree, and enumerate tag values. Covers the datasource-proxy access path, the credential-expiry failure that returns empty results indistinguishable from "no data", the negative control that proves a filter actually applied, and the id/kind/base64 decoding quirks in the response. Use when investigating backend latency, checking what the backend recorded for a request, or establishing which infrastructure tiers a trace reaches. Triggers on Tempo, TraceQL, Grafana traces, backend span inspection, "does the backend have this trace", or tracing a request past the API boundary. +maturity: experimental +--- + +# grafana-tempo-queries + +Tempo holds **backend** spans. Client spans from the extension and mobile go to Sentry via the SDK's own transport and never appear here — so a Tempo trace normally starts at an inbound server span, and a missing root is expected rather than broken. Mobile sends no trace context to the backend: its `tracePropagationTargets` name no backend host and it does not set `propagateTraceparent`, so a backend trace for a mobile request shares no trace id with the mobile client's spans. To join the two halves, see `sentry-grafana-correlation`. + +## Setup + +Everything goes through Grafana's datasource proxy. Authenticate with a service account token. Keep the host, datasource uid, org id, and token in your environment — this repository is public, so never commit them. + +```bash +# Set these once per shell, from your own Grafana instance: +# GRAFANA_HOST e.g. https://grafana. +# TEMPO_UID the Tempo datasource uid (see discovery below) +# GRAFANA_ORG the numeric org id the datasource belongs to +# GRAFANA_TOKEN a service account token, Viewer role, scoped to the Tempo datasource +BASE="$GRAFANA_HOST/api/datasources/proxy/uid/$TEMPO_UID" +AUTH=(-H "Authorization: Bearer $GRAFANA_TOKEN" -H "X-Grafana-Org-Id: $GRAFANA_ORG") +``` + +Create the token under Administration, Service accounts. A Viewer-role account is +sufficient for every query in this skill, and the token is revocable on its own without +disturbing anything else you have open. + +If your Grafana disallows service accounts and there is genuinely no token path, a browser +`grafana_session` cookie works in the same header slot: + +```bash +AUTH=(-H "Cookie: grafana_session=$GRAFANA_SESSION" -H "X-Grafana-Org-Id: $GRAFANA_ORG") +``` + +Reach for that only after confirming a token cannot be issued. A session cookie carries +your whole Grafana authority rather than one datasource's read access, expires on a +schedule you do not control, and cannot be revoked without ending your own session. It is +also indistinguishable from you in an audit log. + +Discover the datasource uid rather than guessing it: + +```bash +curl -s "$GRAFANA_HOST/api/datasources" "${AUTH[@]}" \ + | node -e 'JSON.parse(require("fs").readFileSync(0)).filter(d=>d.type==="tempo").forEach(d=>console.log(d.uid,d.name))' +``` + +## Check the instrument before believing a result + +**A stale session returns HTTP 401 with an empty body, and a naive parser reports that as zero results** — indistinguishable from "this data does not exist". This is the single most expensive failure mode here: it produces confident negative conclusions about instrumentation coverage. + +```bash +# 1. Prove you are authenticated. Do this first, every session. +curl -s -o /dev/null -w 'grafana auth: HTTP %{http_code}\n' "$GRAFANA_HOST/api/user" "${AUTH[@]}" + +# 2. Prove the filter is actually being applied, with a query that must match nothing. +curl -s -G "$BASE/api/search" "${AUTH[@]}" \ + --data-urlencode 'q={span.db.system = "not-a-real-db-xyz"}' \ + --data-urlencode "start=$START" --data-urlencode "end=$NOW" \ + | node -e 'const j=JSON.parse(require("fs").readFileSync(0));console.log("control traces:",(j.traces||[]).length,"(must be 0)")' +``` + +If several different filters all return exactly your `limit`, the filter is not being applied — treat the results as unfiltered until the negative control returns 0. + +## Core queries + +Every endpoint wants an explicit epoch-seconds window. Omitting it on a by-id lookup makes the request hunt across all blocks and hit a context deadline. A window is not always enough: a by-id lookup of a session-length trace has returned 500 with one. + +```bash +NOW=$(date +%s); START=$((NOW-3600)) +``` + +**Search by TraceQL.** Returns trace summaries plus the spans that matched. + +```bash +curl -s -G "$BASE/api/search" "${AUTH[@]}" \ + --data-urlencode 'q={resource.service.name="my-service"}' \ + --data-urlencode "start=$START" --data-urlencode "end=$NOW" \ + --data-urlencode "limit=20" +``` + +**Fetch one trace in full** (OTLP JSON: resource batches → scope spans → spans). + +```bash +curl -s "$BASE/api/traces/$TRACE_ID?start=$START&end=$NOW" "${AUTH[@]}" +``` + +**Enumerate values for a tag** — useful for inventorying what a fleet emits. Expect a `502` on high-cardinality tags; fall back to inspecting individual traces rather than concluding the tag is unused. + +```bash +curl -s -G "$BASE/api/v2/search/tag/span.db.system/values" "${AUTH[@]}" \ + --data-urlencode "start=$START" --data-urlencode "end=$NOW" +``` + +## TraceQL patterns worth knowing + +| Goal | Query | +| --- | --- | +| One service | `{resource.service.name="svc-name"}` | +| Several services | `{resource.service.name=~"(svc-a|svc-b)-prd"}` | +| Attribute present at all | `{span.db.system != nil}` | +| Attribute absent, which a `!=` comparison skips | `{span.db.system = nil}` | +| Span kind | `{kind=server}`, `{kind=client}` | +| Slow spans | `{duration > 1s}` | +| **Two conditions anywhere in the same trace** | `{resource.service.name="svc-a"} && {span.db.system != nil}` | + +The last one is the important one: `&&` between two brace groups is a **trace-level** conjunction, not a single-span filter. It answers "does a request into this service reach a database at all", which is how you map how deep a trace goes without reading traces one at a time. + +## Reading the response + +- **Span and trace ids are base64**, not hex. Decode before comparing them to anything from a header or from Sentry: `Buffer.from(id,"base64").toString("hex")`. +- **`kind` is a string** (`SPAN_KIND_SERVER`, `SPAN_KIND_CLIENT`, `SPAN_KIND_INTERNAL`), not the numeric enum. Filtering on `sp.kind === 2` silently matches nothing. +- **Search results drop leading zeros from trace ids.** A 31-character id is a 32-character id with a leading zero; zero-pad before using it anywhere else, or the lookup fails for a reason that looks like absence. +- **`rootServiceName: ""`** means the trace's root is not in Tempo. For client-originated requests that is the normal case — the root is a client span living in Sentry — and it is the marker for finding them. +- Resource attributes carry deployment context (`service.name`, kubernetes pod/namespace/cluster, region); span attributes carry the request (`http.*`, `net.*`, `db.*`). + +## Deep links for sharing + +A link is more useful than a pasted id. Build a Grafana Explore URL with the query pre-filled: + +```bash +node -e ' +const left={datasource:process.env.TEMPO_UID, + queries:[{refId:"A",datasource:{type:"tempo",uid:process.env.TEMPO_UID},queryType:"traceql",query:process.argv[1]}], + range:{from:"now-6h",to:"now"}}; +console.log(`${process.env.GRAFANA_HOST}/explore?orgId=${process.env.GRAFANA_ORG}&left=${encodeURIComponent(JSON.stringify(left))}`); +' '' +``` + +Prefer an absolute `from`/`to` when the link needs to outlive the event; a relative window slides off it and the reader opens an empty result. The Explore state in a link can be dropped across an SSO redirect, so a reader who is not yet signed in can land on an empty Explore. + +## Failure modes + +| Symptom | Cause | Response | +| --- | --- | --- | +| All queries return 0 | Session expired (401, empty body) | Check `/api/user` first | +| Every filter returns exactly `limit` | Filter not applied | Run the negative control | +| By-id lookup times out | No time window | Pass `start`/`end` | +| Tag-values returns 502 | High cardinality | Inspect traces directly | +| Cloudflare error 1015 instead of JSON | Parallel requests tripped the rate limit in front of Grafana | Send requests one at a time | +| Id from search not found elsewhere | Leading zeros stripped | Zero-pad to 32 chars | +| Kind filter matches nothing | Comparing to a number | Compare to `SPAN_KIND_*` | +| Trace has no root | Root is a client span | Expected; see `sentry-grafana-correlation` | diff --git a/domains/observability/skills/instrumentation/repos/metamask-extension.md b/domains/observability/skills/instrumentation/repos/metamask-extension.md new file mode 100644 index 00000000..33d06bc3 --- /dev/null +++ b/domains/observability/skills/instrumentation/repos/metamask-extension.md @@ -0,0 +1,68 @@ +--- +repo: metamask-extension +parent: instrumentation +--- + +## Key Files + +| Content | Path | +|---------|------| +| Sentry trace wrapper | `shared/lib/trace.ts` | +| Trace name enum | `shared/lib/trace.ts` → `TraceName` | +| MetaMetrics controller | `app/scripts/controllers/metametrics-controller.ts` | +| Anonymous-event marking (`excludeMetaMetricsId`) | `app/scripts/controllers/analytics/analytics.ts` → `applyAnonymousEventOptions()` | +| Event enum | `shared/constants/metametrics.ts` → `MetaMetricsEventName` | +| Sentry setup + sample rate | `app/scripts/lib/setupSentry.js` → `getTracesSampleRate()` | +| Sentry `user.id` (set to the MetaMetrics `analyticsId`) | `app/scripts/lib/sentry-metametrics.ts` → `metaMetricsIntegration()` | +| Segment tracking plan | `Consensys/segment-schema` → `tracking-plans/metamask-extension.yaml`, which lists event libraries. Event definitions live in `libraries/events//` | + +## Cross-Process Context (UI → Background) + +The extension has two Sentry hubs — one in the UI process and one in the background service worker. A trace starting in UI and continuing in background requires explicit context propagation across the RPC boundary: + +```typescript +// Serialize at UI call site +const context: SerializedTraceContext = { + _name: TraceName.MyOperation, + _traceId: span.spanContext().traceId, + _spanId: span.spanContext().spanId, +} + +// Background receives context, creates child span +trace({ name: TraceName.MyOperation, parentContext: context }, async () => { ... }) +``` + +Without propagation: Sentry shows two disconnected operations. With propagation, the background span joins the UI's trace under whichever UI span was active at the call, which is not necessarily the span that caused it. `submitRequestToBackground` attaches a context only when a UI span is active at call time. Without one, `createMetaRPCHandler` runs the handler with no `rpc.handler` span and outside the UI's trace. + +The serialized context carries no sampled flag, so the background continues every propagated trace as sampled. `tracesSampler` then keeps the background span at rate 1 unless a per-name override or a ceiling applies, so background spans can be stored for a trace whose UI root was sampled out. + +The UI and background timestamps do not share a clock. In 17 of 96 measured traces they disagreed by up to 67 minutes, so a duration computed across the boundary is not reliable. + +## Sentry Sample Rate + +```bash +grep -n "tracesSampleRate" app/scripts/lib/setupSentry.js +# The fallback rate. tracesSampler (app/scripts/lib/sentry-traces-sampler.ts) takes precedence over it +``` + +## Sentry Traces Explorer Query (Volume Estimation) + +``` +Environment: production | Time range: 30 days | Mode: aggregate +Query: span.op:http.client span.description:*{endpoint}* +Group by: span.description, transaction +Sort: -count(span.duration) +``` + +## Detect `excludeMetaMetricsId` Misuse + +```bash +grep -rn "excludeMetaMetricsId: true" app/ ui/ shared/ --include="*.ts" --include="*.tsx" --include="*.js" +# Each hit sends its event under the shared anonymous id. Confirm the event must not carry identity. +# Event names matching /^send|^confirm/iu are anonymous by default: check new event names too. +``` + +## Data Council Contact + +- Slack: `#metamask-metametrics` +- Team: `@consensys/data-council` diff --git a/domains/observability/skills/instrumentation/skill.md b/domains/observability/skills/instrumentation/skill.md new file mode 100644 index 00000000..9c88000e --- /dev/null +++ b/domains/observability/skills/instrumentation/skill.md @@ -0,0 +1,87 @@ +--- +maturity: experimental +name: instrumentation +description: Create and update Sentry spans, MetaMetrics events, and Segment events — methodology, policies, common pitfalls +--- + +# Analytics Instrumentation + +## When To Use + +- Adding or modifying a MetaMetrics (Segment) event +- Adding or modifying a Sentry performance span +- Estimating event or span volume from production data +- Auditing existing instrumentation for correctness + +--- + +## Do Not Use When + +- Adding local debug logging with no telemetry destination +- Investigating an existing Sentry error report (use `sentry-mcp-queries`) +- Internal feature flag evaluation not surfaced as an analytics event + +--- + +## Sentry Spans + +### Creating a Span + +1. **Register a named trace entry** in the repo's trace name enum before writing any span code. Unnamed spans are invisible in Sentry filters. +2. **Use the repo's `trace()` wrapper**, not raw `Sentry.startSpan()`. Wrappers handle cross-process context propagation, active-span inheritance, and consistent tag injection. +3. **Inherit parent automatically** — when no `parentContext` is provided, the wrapper inherits from `Sentry.getActiveSpan()`, making the new span a child of the active parent (e.g., a `pageload` span). That parent is whichever span is active at the call, not necessarily the one that caused the work, which metamask-extension#45527 (stop spans silently attaching to whatever trace happens to be active) proposes to fix. A span started by `trace()` without a callback is active only inside that call, so spans created before its `endTrace()` do not nest under it. + +### Updating a Span + +- Adding a tag: no governance required +- Renaming a trace name enum entry: grep all callsites; update enum and references atomically +- Changing an `op` value: breaks saved queries and dashboards — coordinate with whoever owns them +- Moving a span's start (`trace()`) or end (`endTrace()`): changes what its duration measures, so a release-over-release delta mixes a performance change with a definition change + +--- + +## MetaMetrics / Segment Events + +### Creating an Event + +1. **Check the event name enum** — event may already exist under a different phrasing. +2. **Check the segment tracking plan** — event may be registered under a different name than the enum key. +3. **Add to the enum**, then implement the `trackEvent` call. +4. **Pass `excludeMetaMetricsId: true` only for an event that must not carry the user's identity.** It sends the event under the shared anonymous id and drops the profile ids, for every user, not only those who have not opted in. Event names matching `/^send|^confirm/iu` get it by default unless the caller passes `excludeMetaMetricsId: false` (see data domain `knowledge/metrametrics-identity.md`). +5. **Open a data governance review** before merging. There is usually no CI enforcement on schema registration — this step is easy to skip (see data domain `knowledge/segment-governance.md`). +6. **Register in the team's segment tracking plan** before shipping. + +### Updating an Event + +- Adding a property: requires governance review and schema update +- Renaming an event: deprecate old + add new in tracking plan; coordinate on migration window +- Removing an event: confirm no active dashboards depend on it before removing + +--- + +## Volume Estimation via Sentry + +When direct Segment access is unavailable, estimate from Sentry production span data: + +1. **Find a correlated HTTP endpoint** — one that fires 1:1 with the event. +2. **Query Sentry Traces Explorer** (aggregate mode): + ``` + span.op:http.client span.description:*{endpoint}* + ``` +3. **Read the `count()` aggregate.** Span datasets already extrapolate it by each span's sample weight, so it is the estimate. Do not multiply it by `1 / tracesSampleRate`, which extrapolates twice. +4. **Interpret as upper bound** — endpoint may have callers outside the event path. + +Caveats: sample population is MetaMetrics opted-in users only. The extension's Sentry integration drops every event unless `consentDecisionMade && optedIn`, and Segment gates differently, so attribution across the two pipelines holds at install granularity, not per session. For longer-range (30D+) or release-over-release queries, the sampled count is **not** comparable at face value — older releases are downsampled / retention-truncated and `.0` releases are sample-thin; see `sentry-mcp-queries` (Longer-Range Queries and Percentile Fidelity) and the `performance-attribution` skill. + +--- + +## Common Pitfalls + +| Mistake | Correct Approach | +|---------|-----------------| +| `excludeMetaMetricsId: true` on an event that needs user identity | It sends the event under the shared anonymous id for every user. Reserve it for events that must be anonymous | +| Ship event without tracking-plan registration | No CI gate — add governance review explicitly to PR checklist | +| Raw `Sentry.startSpan()` instead of the repo's `trace()` wrapper | Use the wrapper — handles cross-process context and active-span inheritance | +| New span with no trace name enum entry | Register enum entry first; unnamed spans are invisible in Sentry filters | +| Multiply a span `count()` by `1 / tracesSampleRate` | `count()` is already extrapolated, so read it as the estimate | +| Treat Sentry estimates as exact counts | Probabilistic sample — state sample size and confidence | diff --git a/domains/observability/skills/performance-attribution/repos/metamask-extension.md b/domains/observability/skills/performance-attribution/repos/metamask-extension.md new file mode 100644 index 00000000..32744604 --- /dev/null +++ b/domains/observability/skills/performance-attribution/repos/metamask-extension.md @@ -0,0 +1,97 @@ +--- +repo: metamask-extension +parent: performance-attribution +--- + +## Source & Project + +Primary source is Sentry **Trace Explorer** (not Dashboard 219877): + +- Project `metamask` (ID `273505`), `environment:production` +- Mode `Aggregates`, **Group By** `release`, **Visualize** `p75(span.duration)` and `p95(span.duration)` +- Time `90d` (primary). Dashboard 219877 (30d) is legacy/context only. + +## Key Transactions + +| Transaction | What it measures | +|---|---| +| `UI Startup` | Extension click → interactive UI. Service-worker boot is a separate trace, rooted under `/service-worker.js`, so background startup work is not in it | +| `/home.html` | Home page render | +| `Asset Details` | Token/NFT detail view render | +| `/notification.html` | dApp confirmation popup (approvals/signatures) — high-frequency for power users, compounds with usage | + +## Query Template + +``` +is_transaction:true environment:production transaction:"UI Startup" (release:metamask-extension@13.11.2 OR release:metamask-extension@13.12.2 OR release:metamask-extension@13.13.1 OR release:metamask-extension@13.14.2 OR release:metamask-extension@13.15.0) +``` + +Swap the `transaction:"…"` value per metric; keep `statsPeriod=90d`. + +## Version Selection — Highest-Sample Patch Per Minor + +Anchor each minor line on its highest-sample patch, never the `.0`: + +| Minor | Patch used | Rationale | +|---|---|---| +| 13.11 | 13.11.2 | Highest sample count | +| 13.12 | 13.12.2 | Highest sample count | +| 13.13 | 13.13.1 | Highest sample count | +| 13.14 | 13.14.2 | Highest sample count | +| 13.15 | 13.15.0 | Current release | + +`.0` releases have **10–100× fewer samples** — never anchor a percentile on a `.0` when a higher patch exists in the same minor line. + +## 90d vs 30d — Empirical + +30d baselines ran **~2× higher** than 90d for the same metric (e.g. UI Startup p75 `9.39s → 3.47s` at 30d vs `4.40s → 3.34s` at 90d). Cause unconfirmed — residual-user population and/or sampling of residual traffic; **not** confirmed "power users" (no cohort segmentation). Report 90d; cite 30d only for context. Note: Sentry share links may render 30d in the UI even when the report figure is 90d — verify `statsPeriod=90d`. + +## Hot-Path Files + +| Path | Why it matters | +|---|---| +| `babel.config.js` | Build-time transforms (e.g. React Compiler) — broad scope | +| `ui/selectors/*.js` | Redux selectors — run on every state change | +| `ui/hooks/*.ts` | Hooks — component lifecycle | +| `ui/components/` | Virtualization / render patterns | +| `package.json` | Dependency runtime behavior + core-package bumps | + +## Analysis Commands + +```bash +git log v13.X.X..v13.Y.Y --oneline --no-merges | wc -l # commit count between releases +git diff v13.X.X..v13.Y.Y --stat -- ui/selectors babel.config.js # file-level change summary +git diff v13.X.X..v13.Y.Y -- # detailed diff for one file +git log v13.X.X..v13.Y.Y --oneline -- # commits touching specific paths +``` + +## Core Packages to Monitor + +App-repo diffs miss work shipped as version bumps. Diff `package.json`, then read each package CHANGELOG: + +| Package | Performance relevance | +|---|---| +| `@metamask/assets-controllers` | Token detection, balance fetching, NFT metadata | +| `@metamask/transaction-controller` | Transaction state size, history storage | +| `@metamask/network-controller` | RPC call handling, retry logic | + +```bash +git diff v13.X.X..v13.Y.Y -- package.json | grep -E '^[-+] +"' # every changed dependency, @metamask/* and others +``` + +Example findings: + +- `@metamask/transaction-controller` v62.8.0 — deprecated `history` / `sendFlowHistory` from `TransactionMeta` → significant state-size reduction for power users (consumed in extension [#38665](https://github.com/MetaMask/metamask-extension/pull/38665)). +- `@metamask/assets-controllers` v94.0.0 ([core #7408](https://github.com/MetaMask/core/pull/7408)) — Account API v2 → v4 for token detection → fewer RPC calls, delegated detection. +- `@sentry/browser` 10.x: a CI benchmark ceiling breach was traced to this bump (benchmark harness, not production), so read bumps outside `@metamask/*` too. + +## Worked Example: v13.11 → v13.15 (90d) + +| Metric | p75 (typical) | p95 (tail) | +|---|---|---| +| UI Startup | 4.40s → 3.34s (-24%) | 15.65s → 9.11s (**-42%**, -6.5s) | +| /home.html | 1.69s → 1.19s (-30%) | 4.96s → 3.24s (-35%) | +| Asset Details | 100ms → 47ms (**-53%**) | 287ms → 94ms (**-67%**) | +| /notification.html | 1.36s → 1.05s (-23%) | 4.30s → 4.71s (+9%, **high variance — inconclusive**) | + +Most UI Startup gains and the /home.html p95 gain landed in 13.12 (p95 UI Startup -40% in one release). /home.html p75 moved 1.69s → 1.56s in 13.12, 0.13s of its 0.50s drop, and its larger drops came in 13.14–13.15. Asset Details improved across 13.14 → 13.15. Treat the per-release header deltas as measured totals and attribute individual code changes as likely contributors only. diff --git a/domains/observability/skills/performance-attribution/skill.md b/domains/observability/skills/performance-attribution/skill.md new file mode 100644 index 00000000..ffd54172 --- /dev/null +++ b/domains/observability/skills/performance-attribution/skill.md @@ -0,0 +1,90 @@ +--- +maturity: experimental +name: performance-attribution +description: Attribute release-over-release p75/p95 performance movements to specific code changes via black-box diff analysis +--- + +# Performance Attribution + +Pair a **measured** percentile movement (from Sentry Trace Explorer) with **black-box code-diff analysis** to produce confidence-rated attributions: what changed across releases, how much it moved, and why. + +## When To Use + +- Explaining a confirmed p75/p95 latency change across releases +- Building a per-release attribution catalogue (change → confidence → metric) +- Auditing whether a "performance initiative" actually moved a metric +- Attributing movement that spans the app repo **and** `@metamask/*` core-package bumps + +## Do Not Use When + +- The metric movement isn't yet confirmed reliable — run query hygiene first (see `sentry-mcp-queries`: filter superseded/low-sample releases, normalize, verify stored sample size) +- You need proof of causation — this yields *likely contributors*, not isolated causes (see Limitations) +- Pre-merge perf review of a single PR — there is no production metric to attribute yet + +## Step 1 — Get the Measurement Right First + +Attribution is only as good as the metric. Lock these down before touching code: + +- **Percentile.** p75 = typical user (more stable signal). p95 = slowest 5% — *assumed* large-wallet/power users, but **not cohort-verified** (also slow hardware / poor network). Prioritize p95 when the optimization targets data size (memoization, virtualization) that disproportionately helps the tail; trust p75 as the more reliable number. +- **Time window.** Use the **longer (90d) window as primary** — it includes traffic from when older releases were actively used, so the population is representative and comparable across releases. A 30d window over-weights residual users still lingering on old versions → inflated baselines and bigger-looking deltas between *different* populations. Report 90d; cite 30d only as context. +- **Version selection.** Per minor line, anchor on the **highest-sample patch**, never the `.0`. `.0` releases have 10–100× fewer samples and are rollout-biased. See `sentry-mcp-queries` → *Filtering Unreliable Releases* and *Longer-Range (30D+) Queries and Percentile Fidelity*. + +## Step 2 — Black-Box Code Analysis + +Assess impact on **code content + execution frequency alone**. Deliberately ignore commit messages, PR titles/descriptions, claimed impact, and epic/initiative goals — they bias the read. Base it on: diff content, file location (→ execution frequency), algorithmic complexity, and memoization patterns. + +Hot-path categories — a change here can move a render metric: + +| Category | Why it matters | +|---|---| +| Build config | Build-time transforms (e.g. React Compiler) apply broadly | +| Selectors | Run on every state change — hottest path | +| Hooks | Affect component lifecycle / re-render frequency | +| Components | Virtualization & render patterns | +| Dependencies | Version bumps change runtime behavior | + +Pattern catalogue — what to grep for: + +| Pattern | Signal | Confidence | +|---|---|---| +| `createDeepEqualSelector` → `createSelector` + `EMPTY_ARRAY` sentinel | Removes per-change deep compares | HIGH if foundational selector | +| Identity-function selector `(foo => foo)` → real transform | Broken memoization fixed | HIGH if many consumers | +| In-place mutation `.sort()/.reverse()/.splice()` → spread copy | Mutation had broken all downstream memoization | HIGH | +| Build-plugin addition with broad scope | Build-time optimization | HIGH if scope = all `ui/` | +| O(n) string parse → O(1) lookup | Algorithmic reduction | MEDIUM — depends on call frequency | + +## Step 3 — Score Confidence + +1. **Mechanism** — how does this reduce work? (fewer re-renders / less allocation / better caching) +2. **Frequency** — is the path hot? (selector per state change = hot) +3. **Scope** — how many components/files does it touch? +4. **Match** — does it target what the metric measures? + +| Confidence | Criteria | +|---|---| +| HIGH | Clear mechanism + hot path + timing matches the metric move | +| MEDIUM | Mechanism clear, frequency or scope uncertain | +| LOW | Indirect or infrastructure-only | + +## Step 4 — Don't Forget Core Packages + +App-repo diffs miss work shipped as `@metamask/*` version bumps — it surfaces only as a `package.json` change. For each bump between releases, read the package CHANGELOG "Changed"/"Fixed" sections for state-size reduction, caching, fewer RPC calls, batching, or data-structure/field deprecations. (Commands and the packages to watch live in the repo file.) + +## Reading / Writing an Attribution Catalogue + +- Release-header totals = the **measured** improvement for the whole release +- Table rows = **likely contributors**, not isolated causes +- "High confidence" = mechanism + timing + population align +- Always keep an **Unattributed** section for movement no change explains +- Flag high-variance metrics (a noisy confirmation-popup p95) as inconclusive, not as wins + +## Limitations + +- **Correlation, not causation** — change + improvement in the same release does not prove the change caused it +- **Release totals, not isolated impact** — a "-44%" reflects the entire release, not one change +- **Production variance** — user hardware and network are uncontrolled, and each release's user mix differs (early updaters on a new release, lingering users on an old one) +- **Code analysis, not runtime profiling** — based on structure, not measured execution paths +- **p95 cohort is assumed, not verified** — no power-user segmentation +- **Window choice changes the baseline** — always state which window a number came from + +For more precise attribution: per-optimization feature flags / A-B tests, CI synthetic benchmarks, and verified user-cohort segmentation. Threshold remote feature flags assign each user to a fixed group by hashing a per-user id (the canonical profile ID, or the MetaMetrics ID before a profile exists), so a threshold flag gives a stable A/B split. diff --git a/domains/observability/skills/sentry-grafana-correlation/skill.md b/domains/observability/skills/sentry-grafana-correlation/skill.md new file mode 100644 index 00000000..886a067c --- /dev/null +++ b/domains/observability/skills/sentry-grafana-correlation/skill.md @@ -0,0 +1,105 @@ +--- +name: sentry-grafana-correlation +description: Join one trace across Sentry and Grafana Tempo by trace id to see the whole client-to-backend path, and diagnose why a half is missing. Covers the split-store model (client spans reach Sentry through the SDK and survive only head sampling; backend spans reach Tempo through tail sampling and Sentry through environment routing), the classification of both-halves / client-only / backend-only outcomes with the sampling and routing rule that causes each, and the id-padding, time-window, and query-syntax traps that make a present trace look absent. Use when a trace looks truncated, a backend span has no parent, per-hop latency needs attributing across the seam, or you need to know which store should hold a given span. Triggers on cross-stack trace, orphaned span, trace id lookup, client-backend correlation, split waterfall, or "where did the rest of the trace go". +maturity: experimental +--- + +# sentry-grafana-correlation + +One request produces spans in two stores, joined only by `trace_id`. Reading a trace end to end means querying both and knowing which absences are expected. + +Prerequisite: `grafana-tempo-queries` for the Tempo side, `sentry-mcp-queries` for richer Sentry work. + +## The model — what lands where, and why a half goes missing + +| Span | Reaches | Gated by | +| --- | --- | --- | +| Client (`pageload`, `navigation`, `http.client`, custom) | Sentry, via the SDK transport | the head decision of the context the span joins: the client's `tracesSampleRate`, or a sampled flag propagated from another client realm | +| Backend (`http.server`, internal, db, messaging) | Tempo, via the collector | collector tail-sampling policy | +| Backend, additionally | Sentry, if the collector forwards it | an environment attribute on any span in the trace matching a routing policy | + +Three consequences drive every diagnosis below: + +- **The client's sampled flag and the client's own retention are separate decisions.** The propagated `traceparent` flag tells the backend whether to record; the client's head sampling decides whether the client span is kept. When the flag says record and head sampling drops the client span, the backend records a span whose parent was never stored anywhere — an orphan. This is the normal case at low client sample rates, not an anomaly. The same split happens inside one client with two realms, such as an extension's UI and background: a realm that continues a propagated context as sampled keeps its spans while the originating realm's head sampling drops their parent, so client-side orphans appear in Sentry too. +- **A `-00` (not-sampled) flag can suppress the backend span entirely**, because a parent-respecting sampler delegates to "never record" for an unsampled remote parent. No backend span is created at all — different from one being dropped later. +- **Backend spans reach Sentry only if some span in their trace carries an environment attribute that matches a routing policy.** Tail sampling routes whole traces: one matching span forwards the trace with every service in it. A service that expresses environment under a different attribute name is absent from Sentry only in traces where no other span matches, and is still present in Tempo. + +## Setup + +Keep organisation slugs, project ids, and hosts in your environment — do not commit them. + +```bash +# SENTRY_ORG, SENTRY_PROJECT_ID (numeric), SENTRY_AUTH_TOKEN +# plus the grafana-tempo-queries variables for the Tempo side +``` + +## Query the Sentry half + +```bash +curl -fsS -G "https://sentry.io/api/0/organizations/$SENTRY_ORG/events/" \ + -H "Authorization: Bearer $SENTRY_AUTH_TOKEN" \ + --data-urlencode "dataset=spans" \ + --data-urlencode "field=span.op" --data-urlencode "field=span_id" \ + --data-urlencode "field=parent_span" --data-urlencode "field=span.description" \ + --data-urlencode "field=timestamp" \ + --data-urlencode "query=trace:$TRACE_ID" \ + --data-urlencode "project=$SENTRY_PROJECT_ID" \ + --data-urlencode "statsPeriod=24h" \ + --data-urlencode "sort=-timestamp" +``` + +A syntax trap that produces misleading emptiness: + +- **Any field you sort on must also be selected.** Sorting by `-timestamp` without requesting `timestamp` returns `400 orderby must also be in the selected columns or groupby` — and a script that swallows errors reports it as no results. + +`!has:parent_span` selects root spans in a spans query run through Sentry MCP `search_events`. If a raw `/events/` call rejects the filter, request `parent_span` as a field and filter client-side. + +Use `project=-1` to search every project at once when you do not yet know which one should hold the span — that is how you tell "in the wrong project" apart from "absent". + +## Procedure — Tempo to Sentry + +Use when you have a backend trace and want its client context. + +1. Find client-originated backend traces: search your services in Tempo, then keep the results whose `rootServiceName` reports the root was never received. Those reference a client parent that is not in Tempo. +2. **Zero-pad each trace id to 32 characters** before querying Sentry. Tempo search strips leading zeros, and an unpadded id returns nothing for a reason that looks like absence. +3. Query Sentry for `trace:`, first in the client's project, then with `project=-1`. +4. Classify with the table below. + +## Procedure — Sentry to Tempo + +Use when a Sentry trace looks truncated at the network boundary. + +1. Take the trace id from the Sentry trace view. +2. Look for backend spans of that trace in Sentry itself first. If the collector forwards backend spans for that environment, both halves may already be in one place and no cross-store hop is needed. Check presence with an attribute that marks backend spans (a tenant id, for example), not `span.op:http.server`: server spans were 2.9% of backend spans in one measurement, so keying on them can read as none present. Keep `http.server` for the nesting check below. +3. Otherwise fetch the trace from Tempo by id, with a time window that brackets the client span's timestamp. +4. If Tempo has nothing, the backend either never recorded it (a `-00` flag), or its trace fell outside the tail-sampling policy. + +## Classification + +| What you find | Meaning | Where to look next | +| --- | --- | --- | +| Client and backend spans, backend parented on the client's request span | Healthy join; per-hop latency is attributable | — | +| Client and backend spans, backend parented on an enclosing operation root | Propagation is attaching the wrong parent, so the backend span sits beside its caller instead of beneath it | The client's header-injection path | +| Backend spans only, client parent referenced but nowhere | Orphan: the flag instructed recording, head sampling discarded the client span. Or no client span was active when the request went out, so the SDK sent a parent id from its scope's propagation context that no span carries | Client sample rate, or decoupling the flag from head sampling, or whether a span is active at the call site | +| Client spans only, no backend span anywhere | Either no header was propagated to that host, or the flag was `-00` so the backend never created a span | Propagation targets (`tracePropagationTargets`), then whether `traceparent` is sent at all (`propagateTraceparent`, off by default in the Sentry SDK, since an OpenTelemetry backend does not read Sentry's own `sentry-trace` header), then the flag | +| Backend in Tempo but not in Sentry when it should be | Environment attribute does not match a forwarding policy | The service's environment tagging | +| Nothing in either store | Head-sampled out end to end | Expected at low sample rates | + +## Checking whether a backend span nests correctly + +Nesting takes the parent identity and the times, not the picture. Take the backend `http.server` span's `parentSpanId` (hex-decode it from Tempo's base64), then look that id up among the client's spans in Sentry: + +- Resolves to an `http.client` span whose description matches the same URL, and the backend span starts before that span ended → correctly nested beneath the request that caused it. A backend span that starts after the `http.client` span ended cannot be its child, whatever the id says (one recorded case started 318 seconds after). +- Resolves to a transaction root or custom operation span → the backend span is a sibling of its caller; hop latency cannot be read off the waterfall. +- Resolves to nothing in either store → orphan. + +Node services truncate span starts to whole milliseconds, because the OpenTelemetry JS SDK stamps a start from `Date.now()`. A start and an end within the same millisecond do not establish which came first. + +## Traps + +- **Time windows differ per store.** Tempo retention is typically much shorter than Sentry's, so an older trace legitimately exists in one and not the other. Confirm the window before concluding a half is missing. +- **Window a parent lookup on the parent, not the child.** A parent starts before its child, so a window that opens at the child's start excludes the parent by construction and manufactures a false orphan. +- **Client-observed duration includes time no span records.** An `http.client` span also holds time in any tier that emits no spans, such as a CDN in front of the backend, and time queued for a connection once Chrome's six-connections-per-origin limit is full. +- **Verify credentials on both sides first.** A Grafana token without datasource scope and an out-of-scope Sentry token both present as empty results, which read as "no data" rather than as "not allowed". Confirm each side returns something before concluding a half is missing. +- **A relative time window on a shared link expires.** Pin absolute ranges when the link needs to outlive the incident. +- **One sampling decision can be shared across a long-lived trace id.** If a client reuses a trace id across many operations, the proportion of spans marked sampled will not match the nominal client rate; do not read that ratio as an effective sample rate. diff --git a/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md b/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md new file mode 100644 index 00000000..b745ef0f --- /dev/null +++ b/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md @@ -0,0 +1,60 @@ +--- +repo: metamask-extension +parent: sentry-mcp-queries +--- + +## Organization and Projects + +``` +mcp__sentry__find_organizations → confirm org slug +mcp__sentry__find_projects → metamask (the extension: Chrome/MV3 + Firefox/MV2) +``` + +## Standard Filter Set for Extension Errors + +``` +environment:production +installType:normal +``` + +Then add `dist:mv3` or `dist:mv2` to isolate by manifest. Production releases are named `metamask-extension@`, so filter `release:metamask-extension@13.47.0`, not `release:13.47.0`. + +## Sample Rate + +Production `tracesSampleRate` = `0.005` (0.5%), the fallback in `getTracesSampleRate()`. `tracesSampler` (`app/scripts/lib/sentry-traces-sampler.ts`) takes precedence over it: a per-name override, the remote `sentry.tracesSampleRate` flag and a sampled parent each set a span's effective rate, so no single multiplier converts stored spans to volume. + +```bash +# Verify current value before using +grep "tracesSampleRate" app/scripts/lib/setupSentry.js +``` + +## Volume Estimation — Worked Example + +`AssetsFirstInitFetchCompleted` correlates 1:1 with `accounts.api.cx.metamask.io/v1/supportedNetworks` (fires once per init) — **not** `/v4/multiaccount/balances` (fires per account): + +``` +/v1/supportedNetworks: 2.6M sampled (30d) once per init: tracks the event rate +/v4/multiaccount/balances: ~26M sampled per account: 10× the init count, NOT the event rate +``` + +Lesson: pick the once-per-event endpoint or you over-count by the fan-out factor. + +## Common Issue Searches + +| What you're looking for | Query | +|---|---| +| Background connection errors | `is:unresolved background connection` | +| MV3-only errors | `is:unresolved dist:mv3` | +| Errors spiking in recent release | `is:unresolved times_seen:>100` | +| Performance issues | `issue.category:performance` | + +## Tag: `dist` Values + +| Value | Meaning | +|-------|---------| +| `mv3` | Chrome (Manifest V3 — service worker) | +| `mv2` | Firefox (Manifest V2 — background page) | + +## Seer Analysis Notes + +Seer has access to the Sentry issue, stack traces, and recent events. It does not have access to the codebase. Validate its hypothesis against the actual handler chain in the source — especially for keepalive, lifecycle, and concurrency conclusions. diff --git a/domains/observability/skills/sentry-mcp-queries/skill.md b/domains/observability/skills/sentry-mcp-queries/skill.md new file mode 100644 index 00000000..7262b65b --- /dev/null +++ b/domains/observability/skills/sentry-mcp-queries/skill.md @@ -0,0 +1,135 @@ +--- +maturity: experimental +name: sentry-mcp-queries +description: Query Sentry via MCP — error triage, tag distribution, volume estimation, replay retrieval +--- + +# Sentry MCP Queries + +## When To Use + +- Investigating a production error before attributing root cause +- Checking dist (MV3 vs MV2) error distribution +- Estimating event or span volume from production data +- Comparing error rates release-over-release for regression detection +- Retrieving session replay or profiling data + +## Do Not Use When + +- The error reproduces locally with a full stack trace +- Reading product analytics (Segment events, not Sentry errors/spans) +- Pre-merge investigation — Sentry data is post-merge only + +## Setup + +Run once per session: + +``` +mcp__sentry__whoami → confirm auth +mcp__sentry__find_organizations → org slug +mcp__sentry__find_projects → project slug(s) +``` + +All subsequent tools require `organization_slug` and usually `project_slug`. Slug mismatch causes silent empty results. A filter on an attribute the dataset does not carry also returns an empty result with no error, so run the query without that filter as a control before reading a zero. `mcp__sentry__search_events` returns at most 100 rows per call, has no cursor, and accepts only a relative `period` such as `90d`. + +## Workflow: Error Triage + +1. `mcp__sentry__search_issues` — find by title, fingerprint, or keyword. An issue is a grouping bucket, not a fault: Sentry groups by stack trace, which the issue list does not return, so two issues with matching titles are not shown to be one fault. +2. `mcp__sentry__get_issue_tag_values` — check `dist` distribution **before** attributing root cause. An error's tag distribution does not reflect the user population's, so also compare `count_unique(user.id)` by that tag. +3. If 99%+ one dist → platform lifecycle root cause (see `extension-errors-debugging`) +4. `mcp__sentry__search_issue_events` — individual events for stack trace detail +5. `mcp__sentry__analyze_issue_with_seer` — AI-assisted hypothesis (validate against code) + +## Workflow: Volume Estimation + +Segment event volume is invisible from Sentry, but a correlated `http.client` span is not. Anchor estimation on an HTTP endpoint the event's controller calls **1:1** with the event firing. + +1. Identify the correlated endpoint — the one that fires **once per event**, not per sub-call (e.g. a per-init call, not a per-account call). Picking a per-sub-call endpoint over-counts. +2. `mcp__sentry__search_events` aggregate mode, filter `span.op:http.client` + endpoint +3. Read the `count()` aggregate. Span datasets already extrapolate it by each span's sample weight (see *Longer-Range (30D+) Queries and Percentile Fidelity*), so it is the volume estimate. +4. Do not multiply it by `1 / tracesSampleRate`. That extrapolates twice, and the weight Sentry applies can differ from the configured rate (a per-name sampler rate, a remote override, a trace continued as sampled). +5. Treat as an **upper bound** — the endpoint may have callers beyond the event path. Sample population = MetaMetrics-opted-in users only (Sentry opt-in is tied to MetaMetrics). + +## Workflow: Release Comparison + +Compare error rates or metrics across releases for regression detection: + +1. `mcp__sentry__find_releases` — list releases sorted by date +2. **Filter out unreliable releases** (see below) before comparing +3. `mcp__sentry__search_events` with `release:12.5.0` for baseline +4. `mcp__sentry__search_events` with `release:12.6.0` for comparison +5. **Normalize by sessions or users** — raw counts conflate traffic changes with error rate changes: + ``` + rate = events / sessions_for_that_release + ``` +6. Report delta against baseline with sample-size caveat + +Sentry's Endpoint Regression detector checks p95 transaction duration only for server operations (`http.server`, `serverless.function`, `asgi.server`, `rails.request`, `function.aws`, `function.aws.lambda`), so a browser transaction such as `pageload` gets no automatic regression issue and needs this comparison. + +## Filtering Unreliable Releases + +Patch releases have uneven adoption — comparing raw counts against them produces false signal. Skip a release before comparing if: + +| Filter | Threshold | Reason | +|---|---|---| +| Age since publish | < 48–72h | Browser auto-update rollout still ramping (Chrome/Firefox/Edge) | +| Session count | < ~50% of previous stable release | Sample too small for meaningful rates | +| Stored span count | < ~few hundred for p75, < ~few thousand for p95+ | Tail percentiles are computed over the *stored* sample — extrapolated counts hide how few events back them | +| Superseded patch | a higher patch in the same `X.Y.*` line exists **and** the active window (`first_seen`→`last_seen`) is short | Hotfixed-past releases collect few spans, biased to early-updaters during the rollout/migration window | +| Release stage | `dev`, `canary`, `nightly` | Non-production build — different error profile | +| Environment | not `production` | Development / staging noise | +| Manifest split | compare only within same `dist` | MV3 and MV2 populations have different error distributions | + +**Rule of thumb:** use the newest release that has ≥ 3 days of production adoption **and** session volume comparable to the previous stable release. Everything in between is hotfix noise — skip it for regression comparisons unless investigating that specific patch. + +## Longer-Range (30D+) Queries and Percentile Fidelity + +Widening the window past ~30 days to gain sample size trades it back for **fidelity loss on older releases**. Three effects compound: + +- **Sample-rate drift** — `tracesSampleRate` changes between releases, so absolute span counts across a 30D+ window mix different capture rates. Normalize each release by *its own* sample rate (or by sessions/users), never a single global rate. +- **Extrapolation hides thin samples** — span datasets report sample-rate-weighted (extrapolated) counts. A release with 40 stored spans at 0.75% extrapolates to ~5,300 — a real-looking number backed by 40 events. Always check the **stored** sample count (`count_sample()`), not the extrapolated total, before trusting a release. A group backed by a single stored span reports that span's extrapolation weight, not 1, so a per-entity ranking at high cardinality (`groupBy` trace, user or session) or a ratio such as `count()` / `count_unique(trace)` measures sample weight rather than volume. For one trace's stored span count, read `GET /api/0/organizations/{org}/trace-meta/{trace_id}/?project=-1&statsPeriod=`, which does not extrapolate. +- **Retention downsampling** — spans near the retention boundary are partially evicted, so an old release's count is truncated, not representative. Treat the oldest releases in a 30D+ window as lower bounds only. Span listings over a long window undercount as well, not only aggregate counts. + +**For p75+ analysis** (any tail percentile — p75/p90/p95/p99), sample size *and* quality both matter: + +- **Size** — percentiles are computed over stored events. p50 stabilizes in the low hundreds; p75 needs more; p95/p99 need thousands of stored spans. Below that, a handful of outliers move the number — don't report a tail percentile you can't back with stored count. +- **Quality** — rollout-window spans (first-launch, cold cache, state migration) skew the tail high. A superseded patch release's spans are disproportionately these, so its p75+ reads worse than its steady state would. In the extension project, 84.8% of `span.op:pageload` transactions were measured as not user-visible page loads, and the service worker's `pageload` adopts every request made before `finalTimeout` as a child. Scope a pageload percentile by transaction name. + +**Resolving the size-vs-fidelity tension:** when a single release lacks the sample to support p75+, **collapse the patch chain** — aggregate `release:X.Y.*` across the minor line, or compare against the last *widely-adopted* patch — rather than extending the window into aged, downsampled, sample-rate-drifted territory. Reach for sample size *across adjacent stable patches inside the retention-safe window*, not by going further back in time. Use a longer (90d) window as the **primary, comparable-across-releases** source for p75/p95 and a 30d window only as **secondary context** — 30d over-weights the users still lingering on old versions and inflates baselines. A 90d query has returned `meta.dataScanned: partial` from the events API, meaning Sentry scanned only part of the window, and the `mcp__sentry__search_events` output does not show that field. + +For attributing a confirmed p75/p95 movement to specific code changes, see the `performance-attribution` skill. + +## Workflow: Replay and Profile + +1. `mcp__sentry__search_issue_events` — find an event ID with replay/profile +2. `mcp__sentry__get_replay_details` / `mcp__sentry__get_profile_details` for that event ID + +The extension's `setupSentry.js` configures no profiling integration or `profilesSampleRate`, and browser profiling needs a `Document-Policy` response header that an MV3 extension cannot set, so the extension has no profiles to retrieve. + +## Tag Filters + +| Tag | Values | Use | +|-----|--------|-----| +| `dist` | `mv3`, `mv2` | Isolate by manifest version | +| `environment` | `production`, `staging` | Exclude non-prod noise | +| `installType` | `normal`, `development`, `sideload`, `admin` | Exclude developer-loaded builds | + +**Do not conflate `environment` and `installType`** — a production build can have `installType:development` if loaded unpacked. + +## Common Pitfalls + +| Mistake | Correct Approach | +|---------|-----------------| +| Attribute root cause before checking `dist` distribution | Check tag values first — 99%+ MV3 → lifecycle, not app logic | +| Multiply a span `count()` by `1 / tracesSampleRate` | `count()` is already extrapolated, so read it as the estimate | +| Filter `environment:development` for dev builds | Filter `installType:normal` — environment ≠ install method | +| Skip `whoami` and guess org slug | Slug mismatch causes silent empty results | +| Treat Seer analysis as ground truth | Use as hypothesis to validate against code/traces | +| Compare raw event counts across releases | Normalize by sessions — traffic changes masquerade as regressions | +| Include a <48h-old release in a regression comparison | Wait for rollout; auto-update adoption takes 2–7 days | +| Treat every patch release as a comparison point | Most patches have low adoption — compare to the last *widely-adopted* release | +| Treat a release absent from a `sort:-count()` top-N result as filtered out | It fell below the volume cut, which is not an exclusion. Query it by name | +| Trust a release's p95 because its (extrapolated) span count looks large | Check the *stored* sample — p75+ needs hundreds-to-thousands of stored events to be stable | +| Compare span counts across a 30D+ window at face value | Normalize per-release sample rate; older releases are downsampled / retention-truncated | +| Read the newest minutes of a window that ends now as complete | Ingestion lag was measured at ~14 min, so those minutes read low | +| Anchor a percentile on a `.0` release | `.0` releases have 10–100× fewer samples — use the highest-sample patch in the minor line | diff --git a/domains/observability/skills/sentry-quota/repos/metamask-extension.md b/domains/observability/skills/sentry-quota/repos/metamask-extension.md new file mode 100644 index 00000000..c8661159 --- /dev/null +++ b/domains/observability/skills/sentry-quota/repos/metamask-extension.md @@ -0,0 +1,51 @@ +--- +repo: metamask-extension +parent: sentry-quota +--- + +## File Paths + +| Path | Role | +|---|---| +| `shared/lib/trace.ts` | `TraceName` / `TraceOperation` enums = the custom-span registry; `trace({ name, op, data }, cb)` API | +| `shared/lib/wrapper-sampling.ts` | `shouldSampleWrappers(traceId)` — the Tier-2 deterministic sub-sample gate | +| `shared/lib/messenger-tracing.ts` | `wrapMessengerWithTracing` + `isReadOnlyAction` read-only denylist (~90% volume cut before sampling) | +| `app/scripts/lib/createMetaRPCHandler.ts` | `rpc.handler` span — gated behind `shouldSampleWrappers` | +| `app/scripts/lib/setupSentry.js` | global `tracesSampleRate` fallback (`0.005` = 0.5%) | +| `app/scripts/lib/sentry-traces-sampler.ts` | `tracesSampler`: per-name rates (`DEFAULT_TRANSACTION_SAMPLE_RATES`) and the remote-rate ceiling. It takes precedence over `tracesSampleRate` | + +Core controller instrumentation lives in the **`MetaMask/core`** monorepo: per-package `TraceName` in `packages//src/**/{constants/traces,utils/trace}.ts` (e.g. `bridge-controller/src/constants/traces.ts`). Controllers don't import Sentry — they call an injected `trace` callback (`traceAsControllerCallback` in the extension). + +## Commands + +```bash +EXT= +CORE= + +# Span registries (the inventory) +rg -n 'enum TraceName' "$EXT/shared/lib/trace.ts" +rg -n -g '**/{traces,trace}.ts' 'enum TraceName' "$CORE/packages" + +# Locate a culprit's emit site +rg -n '|TraceName.' "$EXT" "$CORE/packages" + +# All span creation sites — then read each enclosing scope for loop/poller (fan-out) +rg -n 'trace\(' "$EXT/app" "$EXT/shared" "$CORE/packages//src" + +# Gate present before the span? (absence = always-on) +rg -n 'shouldSample|tracesSampleRate|hashBucket|Math.random' + +# Kill-switch present? +rg -n 'SENTRY_[A-Z_]*DISABLED' "$EXT" "$CORE/packages//src" + +# PR review — added instrumentation lines only +gh pr diff --repo MetaMask/metamask-extension \ + | rg '^\+' | rg 'TraceName|trace\(|shouldSampleWrappers|SENTRY_.*DISABLED|op:' +``` + +## Architectural Notes + +- **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"). +- **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. diff --git a/domains/observability/skills/sentry-quota/skill.md b/domains/observability/skills/sentry-quota/skill.md new file mode 100644 index 00000000..be000d0c --- /dev/null +++ b/domains/observability/skills/sentry-quota/skill.md @@ -0,0 +1,90 @@ +--- +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 +--- + +# Sentry Span Quota Guard + +Find and fix custom Sentry span instrumentation that blows the project span budget. Operates on **code and PRs**, not Sentry dashboards — you spot the culprit in Sentry (`sentry-mcp-queries`), this skill fixes it in code. + +## When To Use + +- A PR adds custom Sentry spans (`trace()` calls / `TraceName` entries) — review it before merge. +- A custom span/transaction dominates span volume in Sentry — locate where it's emitted and fix it. +- Auditing controllers/UI for always-on, fan-out-prone instrumentation. +- A custom span is the top span-count contributor and must be cut fast (release blocker). + +## Do Not Use When + +- 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. +- Error volume. Errors are metered separately from spans and transactions, so no change here moves the error quota. + +## Breach Triad + +A custom span is a quota risk when these stack. The first three together are the breach profile. + +| Signal | Static signature | Why it blows quota | +|---|---|---| +| **Fan-out** | span created in a loop / `.map` / `.forEach` / per-asset / per-account / per-chain / poller | N spans per trace, not 1 | +| **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 | +| 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. + +**The subtlest fan-out has no visible loop: a memoized selector.** A `trace` passed into a memoized selector (`createSelector` / `reselect`, or any function called from `useSelector`) fires on every input change by reference. If the selector also iterates entities, it is fan-out × recompute-frequency. Its volume tracks internal state-churn, not user action, so no user-facing metric predicts it — you cannot capacity-plan it. Treat any `trace` reaching a selector as fan-out. + +## Workflow + +### PR review (pre-merge gate) +1. `gh pr diff ` — scan **added** lines for three things, not two: new `TraceName` entries, new `trace(` call sites, **and a `trace`/trace-callback passed as an *argument*** into a call (`fn(…, trace)`). The third is the one reviews miss — a caller wiring up a function's optional `trace?` param adds instrumentation with no `trace(` site and no `TraceName` entry. +2. Check what already emits. For every trace name the diff adds, maps or reroutes, query the target project over the last 90 days (`sentry-mcp-queries`). A name that already emits is not new instrumentation. Review it as a change to measured volume, and say so first in the verdict, because a reviewer who believes it is new will look for a history that already exists. For a name that is new to the target, the other client where it already ships is the reference class. Before carrying that client's volume over, list which of its top names the diff can actually start in the target. +3. Score each against the breach triad: is the enclosing scope a loop, poller, **or selector**? is there a gate? a kill-switch? +4. Block if a new always-on span has no gate — require a sub-sample gate (`span-sub-sampling`) before merge. Cheaper than a post-ship cherry-pick. +5. If the diff adds no `trace(` sites, no `TraceName` entries, **no `trace` argument passed into a call, bumps no dependency, and adds or changes no SDK integration** → "no new instrumentation", stop. A dependency bump brings whatever instrumentation the package carries at the adopted version, which no grep of this diff can see, and that volume can be `http.client` traffic the tracing context surfaces rather than wrapper spans. An SDK integration such as `browserTracingIntegration()` emits `pageload`, `navigation` and `http.client` spans with no `trace(` site at all. + +> **Instrumentation is not always added by an instrumentation PR.** The costliest spans arrive incidentally — a caller passes a `trace` argument into an existing function during an unrelated change (a bug fix, a refactor), so the PR's stated purpose gives no signal to review it for quota. Do not gate this scan on the PR *looking* like instrumentation. And accept the limit: a `trace` argument buried in a bug-fix diff will slip a human reviewer, which is why a runtime backstop is needed. Sentry metric alerts cannot target one transaction name's billed volume, only project and category totals, so the backstop belongs in the sampler (the per-name budget below). This skill lowers the rate; it does not eliminate the class. + +### Locate (incident) +1. Grep the span name / `TraceName.X` across the consuming repo **and** the controller package source. +2. Open the call site; read the enclosing scope for the fan-out verdict (loop/poller?). +3. **No grep hits ≠ safe** — the culprit may be on a release ref not checked out. Verify the package version / `gh pr checkout` the shipping ref before concluding clean. + +### Audit +1. Sweep the span registries (`TraceName` enums) + `trace(` call sites. +2. Rank by breach triad — surface ungated × hot-path × fan-out first. + +### Mitigate +Pick the lowest tier that stops the bleed. + +## 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 | +| **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 | + +Tier 0 + 1 stop the bleed; Tier 2 is the follow-up so the metric returns. + +**Prevent the next one, not just this one.** Every tier above requires *naming* the offender first, so a new one runs unbounded until someone catches it. A per-transaction-name budget in the sampler — sample the first N of a name per session, then decay — bounds *any* name with no advance knowledge of which will misbehave. It is the only control that acts before the offender is named, and the only one that catches instrumentation added incidentally rather than deliberately. + +## Common Pitfalls + +| Mistake | Correct approach | +|---|---| +| Per-call random sampling (`Math.random()` per span) | Deterministic `traceId`-hash bucket — all gated spans in a trace kept-or-dropped together | +| Gate the span in Sentry config | Gate at the call site; for an injected-callback controller span, gate in the callback so every consumer inherits the cap | +| Inbound-filter a release you still need data from | Filters drop the whole release — fix in code (Tier 0/2) instead | +| "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 | +| 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 | +| Filter a release before its successor is fixed | The filter redirects users onto the next build; if that carries the same span, volume only moves. Filter a release only once the build users update to is clean. |