diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d552444..29b4ec9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Add `auto-instrumentation` knowledge to the `observability` domain: which Sentry spans the browser SDK creates without a `trace()` call, and why an `http.client` count is a floor on requests. `sentry-quota` now reviews requests a diff adds or re-triggers, and `sentry-mcp-queries` checks the active-span condition before using an endpoint as a volume anchor. - Add `navigation` skill with a repo-agnostic base and a MetaMask Mobile overlay for `Routes` and `NavigationService`. Marked `base: true` so it installs even when its domain is filtered out. - Add `feature-flags` skill with a repo-agnostic base and a MetaMask Mobile overlay for version-gated remote flags. Marked `base: true` so it installs even when its domain is filtered out. ([#147](https://github.com/MetaMask/skills/pull/147)) - Add `analytics` skill (`platform/analytics`, moved from `coding`) with a repo-agnostic base and a MetaMask Mobile overlay for the canonical tracking API. Marked `base: true` so it installs even when its domain is filtered out. ([#140](https://github.com/MetaMask/skills/pull/140)) diff --git a/domains/observability/knowledge/auto-instrumentation.md b/domains/observability/knowledge/auto-instrumentation.md new file mode 100644 index 00000000..21f99e59 --- /dev/null +++ b/domains/observability/knowledge/auto-instrumentation.md @@ -0,0 +1,62 @@ +--- +name: auto-instrumentation +domain: observability +description: Which Sentry spans the browser SDK creates with no trace() call, when it creates them, and how a diff with no trace() site still moves span volume +--- + +# Auto-Instrumentation Coverage + +`browserTracingIntegration()` creates most of the spans a browser client sends, and none of them has a `trace(` site. A review that counts only `trace(` sites and `TraceName` entries cannot see a change that makes these spans more frequent. + +## What the SDK creates + +| Span op | Option (default) | Created when | +|---|---|---| +| `pageload`, `navigation` | `instrumentPageLoad`, `instrumentNavigation` (on) | each page load and each route change. Both are idle spans, active until activity stops | +| `http.client` | `traceFetch`, `traceXHR` (on) | each `fetch` or XHR that starts **while a span is active** and passes `shouldCreateSpanForRequest` | +| `browser.*`, `resource.*`, `paint`, `mark`, `measure` | none; added from Performance API entries when a `pageload` or `navigation` span ends | one span per entry, so per page load or route change | +| `ui.long-animation-frame`, falling back to `ui.long-task` | `enableLongAnimationFrame`, `enableLongTask` (on) | each long frame or task under an active span | +| `ui.interaction.*` | `enableInp` (on) | slow interactions | + +Sources, `@sentry/*` [10.38.0](https://github.com/getsentry/sentry-javascript/tree/ed7956a01f3f6962d3e76ebf91dc3683027e71d8): + +- defaults: [browserTracingIntegration.ts L316-L326](https://github.com/getsentry/sentry-javascript/blob/ed7956a01f3f6962d3e76ebf91dc3683027e71d8/packages/browser/src/tracing/browserTracingIntegration.ts#L316-L326) +- entries added at span end: [L429-L438](https://github.com/getsentry/sentry-javascript/blob/ed7956a01f3f6962d3e76ebf91dc3683027e71d8/packages/browser/src/tracing/browserTracingIntegration.ts#L429-L438) +- request defaults: [request.ts L126-L127](https://github.com/getsentry/sentry-javascript/blob/ed7956a01f3f6962d3e76ebf91dc3683027e71d8/packages/browser/src/tracing/request.ts#L126-L127) + +## The parent condition + +The SDK creates an `http.client` span only if a span is active when the request starts ([fetch.ts L106-L109](https://github.com/getsentry/sentry-javascript/blob/ed7956a01f3f6962d3e76ebf91dc3683027e71d8/packages/core/src/fetch.ts#L106-L109), [request.ts L361-L364](https://github.com/getsentry/sentry-javascript/blob/ed7956a01f3f6962d3e76ebf91dc3683027e71d8/packages/browser/src/tracing/request.ts#L361-L364) for XHR). With no active span, the request carries a non-recording span and nothing is sent. A recorded span is kept or dropped with the trace it joins. + +Two consequences follow. + +- **An `http.client` count is a floor on requests.** Requests that start with no active span leave no span. The count matches the request count only for a call site that always runs inside an active span. +- **A `trace()` callback that awaits requests fans out.** The callback form keeps its span active until the callback settles, so every request inside becomes a child. Spans per trigger is one plus the requests made, and the children are kept at the rate the transaction was drawn at. + +## A new timing trace may already be recorded + +Before reviewing a new custom span for volume, ask whether an automatic span already records what it times. A time-to-content or load-duration trace usually waits on a request, and that request is often already an `http.client` span with a duration. + +1. List the requests the traced interval waits on, in the realm where each one runs. A UI surface can wait on a request the background makes. +2. Query `span.op:http.client` with `span.description` matching each endpoint, grouped by `transaction`, with `p50(span.duration)`. Rows mean the request is already recorded, and the `transaction` column says which spans it is recorded under. +3. For a background request, check whether any row sits under the RPC or controller path the new trace depends on. A background request gets a span only when a span is active there, so a UI trigger can be recorded under a sampled `Background RPC: ` wrapper and nowhere else. +4. Put the difference to the author: what the new span measures that those spans do not, such as time in the UI after the response, Redux propagation, or render. A new span that only restates a request duration duplicates volume without adding signal. + +Extension instance, 30 days to 2026-09-17: the carousel's Contentful `entries` request (`span.description:*promotionalBanner*`) was recorded under every UI page transaction, with a median of 58 to 78 ms by page. The notification list request (`POST .../api/v4/notifications`) was recorded under background transactions, among them `Background RPC: fetchAndUpdateMetamaskNotifications`. Both were already recorded before a PR added time-to-content spans that wait on them. + +## What moves span volume with no `trace(` site + +A diff can add spans when it: + +- adds a request, or makes one fire more often: a new effect dependency, a refetch on a new trigger, a shorter poll; +- moves a request into a `trace()` callback, or wraps a request-making call in one; +- adds routes, page loads, or images, iframes and scripts to a page, since each Performance API entry becomes a span on sampled page loads; +- changes `browserTracingIntegration()` options or the `shouldCreateSpanForRequest` filter. + +Estimate each as requests or entries per trigger × triggers × the kept rate of the trace the span joins. Under a UI `pageload` or `navigation` span that is the default `tracesSampleRate`. Under a context continued as sampled it is close to 1 (`span-sub-sampling`). + +## Measured share, extension + +In project `metamask` (273505), over the 30 days to 2026-09-17, `http.client` was about nine in ten billed-equivalent child spans. Billed-equivalent means `count()` × client rate, or `count()` where no client rate is recorded. Most of those `http.client` spans had no client rate and sat under background transactions such as `Simulate` and `Smart Transactions: Fetch Liveness`, which make requests inside their callbacks. + +Queries: [child spans by op and client rate](https://metamask.sentry.io/explore/traces/?query=environment%3Aproduction+project.id%3A273505+is_transaction%3Afalse&aggregateField=%7B%22groupBy%22%3A%22span.op%22%7D&aggregateField=%7B%22groupBy%22%3A%22client_sample_rate%22%7D&aggregateField=%7B%22yAxes%22%3A%5B%22count%28%29%22%2C%22count_sample%28%29%22%5D%7D&mode=aggregate&sort=-count_sample%28%29&statsPeriod=30d&table=span), [`http.client` with no client rate, by transaction](https://metamask.sentry.io/explore/traces/?query=environment%3Aproduction+project.id%3A273505+span.op%3Ahttp.client+%21has%3Aclient_sample_rate&aggregateField=%7B%22groupBy%22%3A%22transaction%22%7D&aggregateField=%7B%22yAxes%22%3A%5B%22count%28%29%22%2C%22count_sample%28%29%22%5D%7D&mode=aggregate&sort=-count%28%29&statsPeriod=30d&table=span). The window rolls, so re-run them before citing the share. diff --git a/domains/observability/skills/instrumentation/repos/metamask-extension.md b/domains/observability/skills/instrumentation/repos/metamask-extension.md index 33d06bc3..4608d124 100644 --- a/domains/observability/skills/instrumentation/repos/metamask-extension.md +++ b/domains/observability/skills/instrumentation/repos/metamask-extension.md @@ -38,6 +38,30 @@ The serialized context carries no sampled flag, so the background continues ever 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. +## Span Tags, Data and Measurements + +`trace()` routes `tags` by the type of each value, and the field name does not show the split. Non-numeric values reach `scope.setTag` in [`initScope`](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L618-L626). Numeric values never become tags: [`initSpan`](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L635-L643) passes them to `sentrySetMeasurement(key, value, 'none')` so they can be queried numerically, which also means a numeric tag will not match a tag filter. + +`trace()` takes `data` as well, and it becomes the span's `attributes` at creation in [`startSpan`](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L536-L560). + +`endTrace()` takes its own `data` and applies it with `span.setAttribute` immediately before ending the span, at [trace.ts L303-L308](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L303-L308). + +`scope.setTag` is called in one place in the file and `initScope` runs only from `startSpan`, so no path adds a tag after start. A value that must be a tag has to be known when `trace()` is called. + +## `endTrace()` No-Ops + +The pending trace is keyed `:`, with `id` defaulting to `'default'` ([trace.ts L125](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L125), [L596-L605](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L596-L605)). [`endTrace`](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L292-L301) looks that key up in `tracesByKey`, and on a miss it logs `No pending trace found` and returns without ending anything. A mismatched `id` between `trace()` and `endTrace()` produces no span and no error, so nothing reaches Sentry and the case is indistinguishable there from a span that was never started. + +[`startTrace`](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L442-L468) writes `tracesByKey` unconditionally, so a second `trace()` under the same name and `id` overwrites the first entry. The first span becomes unreachable by `endTrace` and never ends. Concurrent traces sharing a name need distinct `id` values. + +The `data` block in `endTrace` is additionally guarded on `pendingTrace.span`, which is `null` when `globalThis.sentry` is absent ([trace.ts L693-L704](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L693-L704)). That is the Sentry-not-loaded case, not the sampled-out case. + +## Debounced Background State Update + +`sendUpdate` is a debounced wrapper, `MILLISECOND * 200` with `{ maxWait: SECOND }`, assigned in the controller constructor at [metamask-controller.js L465-L469](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/app/scripts/metamask-controller.js#L465-L469). The underlying [`privateSendUpdate`](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/app/scripts/metamask-controller.js#L6218-L6220) is what emits `update` with `this.getState()`. + +A UI loading flag flips when the RPC response arrives, but the data reaches the Redux store only on that debounced update. A span ending on a loading flag therefore excludes 200 to 1000 ms plus render, and measures time to response under a time to content name. + ## Sentry Sample Rate ```bash diff --git a/domains/observability/skills/instrumentation/skill.md b/domains/observability/skills/instrumentation/skill.md index 9c88000e..e4322038 100644 --- a/domains/observability/skills/instrumentation/skill.md +++ b/domains/observability/skills/instrumentation/skill.md @@ -27,17 +27,34 @@ description: Create and update Sentry spans, MetaMetrics events, and Segment eve ### 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. +1. **Check what already records this interval.** Most spans a browser client sends have no `trace()` site: `pageload`, `navigation`, `http.client` and the Performance API entries all come from `browserTracingIntegration()`. A timing span that waits on a request is usually waiting on one that already carries an `http.client` span with a duration. Query `span.op:http.client` with `span.description` matching each endpoint the interval waits on, grouped by `transaction`. Rows mean the timing exists; the new span then has to measure something those do not, and the PR should say what. See `auto-instrumentation`. +2. **Register a named trace entry** in the repo's trace name enum before writing any span code. Unnamed spans are invisible in Sentry filters. +3. **Use the repo's `trace()` wrapper**, not raw `Sentry.startSpan()`. Wrappers handle cross-process context propagation, active-span inheritance, and consistent tag injection. +4. **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 +- `trace()` takes `tags`, `endTrace()` takes `data`. There is no post-start tag path, so a value that must be a tag has to be known when `trace()` is called +- A `tags` entry is routed by the type of its value. A non-numeric value becomes a tag; a numeric one is skipped and set as a Sentry measurement instead, so it never becomes filterable as a tag. Put a number in `tags` only when a measurement is what you want - 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 +### Timing To Content + +**End the span on the state the UI renders from, not on the request settling.** A loading flag flips when the response arrives; the data reaches the component later. In metamask-extension the background's `sendUpdate` is debounced 200 ms with a 1 s `maxWait`, so a span ending on a loading flag excludes 200 to 1000 ms plus render. That excluded window is the only part an `http.client` span does not already cover, so the span measures time-to-response under a time-to-content name. + +**Falsifier, before the span ships:** on a cold load, assert the content is in the store at the moment `endTrace` runs. If it is not, the end condition is wrong. A test that only asserts the span ended cannot see this. + +### Cross-Platform Parity + +**Parity is a property of the definition, not of the name.** Two platforms sharing a trace name and an `op` produce one queryable series, so a dashboard puts them side by side whatever the code does. They are comparable only if the start point, the end condition and every tag derivation match. + +- Read the other platform's implementation before choosing the name, not after. +- A tag derived differently means one value selects different populations. Extension and mobile both emit `source: cold|warm` under `notification.performance`: mobile takes `cold` from the render immediately before the span ends, the extension from any loading render since the span started, and the extension span also waits on `isPending`, so it ends at least a render later. +- Where the thing timed differs, no naming makes the numbers comparable. Mobile's banner trace times a Braze banner with SDK targeting in its path; the extension carousel times a Contentful fetch. + --- ## MetaMetrics / Segment Events @@ -85,3 +102,8 @@ Caveats: sample population is MetaMetrics opted-in users only. The extension's S | 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 | +| New timing span for a request that already has an `http.client` span | Query the endpoint first, and say what the new span measures that the automatic one does not | +| Span ends when the fetch settles, under a "time to content" name | End on the state the UI renders from; a debounced store update sits between the response and the render | +| Same trace name as another platform, different end condition or tag derivation | Parity is the definition. Match start, end and every tag, or use a different name | +| A value that must be a tag, passed to `endTrace` | `tags` are start-only via `scope.setTag`; `endTrace` data becomes attributes | +| Effect that owns a span listing dependencies it never reads | React Compiler's effect-dependency validation errors and skips the whole function, so the hook ships unmemoized behind a green build | diff --git a/domains/observability/skills/performance-attribution/repos/metamask-extension.md b/domains/observability/skills/performance-attribution/repos/metamask-extension.md index 32744604..cfd745bf 100644 --- a/domains/observability/skills/performance-attribution/repos/metamask-extension.md +++ b/domains/observability/skills/performance-attribution/repos/metamask-extension.md @@ -95,3 +95,47 @@ Example findings: | /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. + +## Cross-Platform Trace Names + +Trace names and ops are plain strings carrying no platform token, so a name the extension shares with `metamask-mobile` is a single series as far as a query is concerned. +At the shas cited below, 50 of the extension's 84 `TraceName` values and 10 of its 12 `TraceOperation` values also appear in mobile's trace module. +Two of the four Key Transactions above are in that shared set: `UI Startup` and `Asset Details`. + +Any query whose scope spans both platforms' Sentry projects therefore returns one series, and a dashboard will place the two side by side whatever the code does. +The numbers are comparable only when the start point, the end condition and every tag derivation match. **A matching name is not evidence of a matching definition.** + +Regenerate the shared set rather than trusting the counts above, since both enums move: + +```bash +names() { awk '/^export enum TraceName/,/^}/' "$1" | grep -oE "= '[^']+'" | sed "s/^= '//; s/'$//" | sort -u; } +git show e24e5a017af7a84eeaac7f4f6053eb83cd237f5c:shared/lib/trace.ts > /tmp/ext-trace.ts +curl -sS https://raw.githubusercontent.com/MetaMask/metamask-mobile/945c2ade9ec8c2989de8769de26978a0ea68c164/app/util/trace.ts > /tmp/mobile-trace.ts +comm -12 <(names /tmp/ext-trace.ts) <(names /tmp/mobile-trace.ts) # swap TraceName for TraceOperation to diff ops +``` + +### Notification List: One Tag Name, Two Populations + +Both platforms emit `Notification List Time To Content` under op `notification.performance`, so the two land in one series ([extension `shared/lib/trace.ts` L40 and L108](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L40-L41), [mobile `app/util/trace.ts` L315 and L395](https://github.com/MetaMask/metamask-mobile/blob/945c2ade9ec8c2989de8769de26978a0ea68c164/app/util/trace.ts#L315-L316)). +Both tag the span `source: cold` or `source: warm`, and those two tags select different populations. + +Mobile holds `isLoading` from the previous run of the same effect and ends the span on the first run where `isLoading` is false, so `cold` means loading was true on the render immediately before the end ([mobile `useNotificationListPerformance.ts` L49-L69](https://github.com/MetaMask/metamask-mobile/blob/945c2ade9ec8c2989de8769de26978a0ea68c164/app/util/notifications/hooks/useNotificationListPerformance.ts#L49-L69)). + +Extension latches `sawLoadingRef` on any render since the span started where `isLoading` was true and clears it only when a new span begins, so `cold` means loading was seen at any point in the span ([extension `useNotificationListPerformance.ts` L85-L115](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/ui/hooks/metamask-notifications/useNotificationListPerformance.ts#L85-L115)). +Extension also holds the span open while `isPending` is true, so it ends at least one render later than the mobile condition would. + +The two differences interact. +A span whose `isLoading` flickers true then false before `isPending` clears tags `cold` on extension and would tag `warm` under mobile's rule, so **extension `cold` is a superset of mobile `cold`** and the extension span is the longer of the two by at least one render. + +The hooks also take different inputs. +Extension takes an `error` argument and ends the span `success: false, reason: 'error'` with no `source` tag ([extension `useNotificationListPerformance.ts` L94-L101](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/ui/hooks/metamask-notifications/useNotificationListPerformance.ts#L94-L101)), while mobile's config has no error input at all. +What reaches mobile's `success: true` series on a failed fetch is decided by whatever its caller passes as `isLoading`, which is not visible in the hook and is not established here. + +### Banner: Different Work, Not a Naming Mismatch + +The banner traces differ in both fields, so they do not share a series today: extension emits `Home Banner Time To Content` under `banner.performance` ([extension `shared/lib/trace.ts` L41 and L109](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/shared/lib/trace.ts#L108-L109)), mobile emits `Braze Banner Time To Content` under `braze_banner.performance` ([mobile `app/util/trace.ts` L316 and L396](https://github.com/MetaMask/metamask-mobile/blob/945c2ade9ec8c2989de8769de26978a0ea68c164/app/util/trace.ts#L395-L396)). + +Aligning those names would not make the numbers comparable, because the two spans time different work. +Mobile's span wraps the Braze React Native SDK ([mobile `useBrazeBanner.ts` L269-L276](https://github.com/MetaMask/metamask-mobile/blob/945c2ade9ec8c2989de8769de26978a0ea68c164/app/components/UI/BrazeBanner/useBrazeBanner.ts#L269-L276)), while the extension's carousel times a Contentful fetch and tags the span `banner_source: 'contentful'` ([extension `carousel.tsx` L32-L41](https://github.com/MetaMask/metamask-extension/blob/e24e5a017af7a84eeaac7f4f6053eb83cd237f5c/ui/components/multichain/account-overview/carousel.tsx#L32-L41)). + +This is the case where no definition change helps. Renaming aligns a label over two different measurements. diff --git a/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md b/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md index b745ef0f..27e1e71a 100644 --- a/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md +++ b/domains/observability/skills/sentry-mcp-queries/repos/metamask-extension.md @@ -58,3 +58,39 @@ Lesson: pick the once-per-event endpoint or you over-count by the fan-out factor ## 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. + +## Checking Whether an Endpoint Already Has Timings + +Before adding a timer around a network request, ask whether the request already carries a duration: + +``` +mcp__sentry__search_events + dataset: spans + query: environment:production span.op:http.client span.description:** + fields: transaction, count(), p50(span.duration) + sort: -count() +``` + +Rows mean the request is already recorded and already carries a duration, so the timing exists and the open question is where to read it from. + +The `transaction` column answers that: it names the spans the request is recorded under. A single dominant `transaction` means one query reads the latency for that surface. A spread across many parents means no single query reads a per-surface latency off it. + +## Retention Silently Truncates the Window + +Sentry ignores a `start` older than the project's retention. It returns data for the shorter period with no error, and nothing in the response announces that the range moved. + +The result is a count that reads as the window you asked for and covers the window retention allowed. State the window you actually got, not the one you requested. + +The retention period for this project is not recorded here. Establish it before sizing any window that reaches back further than a few weeks. + +The response does not echo the range it used, but `meta.dataScanned` reports whether the scan was complete: `full` when the requested window was served, `partial` when it was not. Read that field on any query with an absolute window, and treat `partial` as a truncated result rather than a slow one. + +Verified against the events API with a control: the same span query over 2026-08-19 to 2026-09-17 returns `dataScanned: full`, and the same query with `start` set to 2025-01-01 returns `dataScanned: partial` with a count an order of magnitude apart. + +## Absolute Windows Need the Events API + +`search_events` takes `period` only, matching `^\d+[hdw]$`. It has no `start` or `end`, so an absolute window cannot be expressed through the MCP tool and has to go through the events API. + +Other tools in the same catalog do accept absolute `start` and `end` (`get_monitor_details`, `search_ai_conversations`, `get_profile_details`), but none of them queries spans. + +`search_issues` is narrower still. Its `period` is an enum of `24h`, `7d`, `14d`, `30d` and `90d`, so an arbitrary relative window is not available there either. diff --git a/domains/observability/skills/sentry-mcp-queries/skill.md b/domains/observability/skills/sentry-mcp-queries/skill.md index 7262b65b..89614b6a 100644 --- a/domains/observability/skills/sentry-mcp-queries/skill.md +++ b/domains/observability/skills/sentry-mcp-queries/skill.md @@ -49,6 +49,7 @@ Segment event volume is invisible from Sentry, but a correlated `http.client` sp 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). +6. Check that the calling path always runs inside an active span. The SDK records an `http.client` span only when a span is active at request start, so requests outside one leave nothing and the count undercounts (`knowledge/auto-instrumentation.md`). The two errors point opposite ways, so the count is neither bound until the call site is read. ## Workflow: Release Comparison diff --git a/domains/observability/skills/sentry-quota/repos/metamask-extension.md b/domains/observability/skills/sentry-quota/repos/metamask-extension.md index c8661159..79b13a2b 100644 --- a/domains/observability/skills/sentry-quota/repos/metamask-extension.md +++ b/domains/observability/skills/sentry-quota/repos/metamask-extension.md @@ -11,7 +11,7 @@ parent: sentry-quota | `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/setupSentry.js` | global `tracesSampleRate` fallback (`0.005` = 0.5%); `browserTracingIntegration()` options, with `enableLongAnimationFrame: true`; and `shouldCreateSpanForRequest`, which drops `http.client` spans for `sentry.io`, `segment.io`, `chainid.network`, `acl.execution.metamask.io` and extension-local snap, locale and hashed-bundle reads. Any other request under an active span is a span | | `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). @@ -38,6 +38,10 @@ rg -n 'shouldSample|tracesSampleRate|hashBucket|Math.random' # Kill-switch present? rg -n 'SENTRY_[A-Z_]*DISABLED' "$EXT" "$CORE/packages//src" +# PR review — added requests, which become automatic http.client spans under an active span +gh pr diff --repo MetaMask/metamask-extension \ + | rg '^\+' | rg 'fetch\(|fetchWithCache|XMLHttpRequest|setInterval|startPolling|setIntervalLength' + # PR review — added instrumentation lines only gh pr diff --repo MetaMask/metamask-extension \ | rg '^\+' | rg 'TraceName|trace\(|shouldSampleWrappers|SENTRY_.*DISABLED|op:' @@ -49,3 +53,53 @@ gh pr diff --repo MetaMask/metamask-extension \ - **`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. + +## Requests Already Recorded as `http.client` Spans + +A PR adding a timing span around a request is reviewed against what the request already records. The procedure is in `knowledge/auto-instrumentation.md` and the query that runs it is `sentry-mcp-queries` → Checking Whether an Endpoint Already Has Timings. The extension supplies a measured instance of each answer, one where the spans that exist are enough and one where they are not. + +Both requests were already recorded before a PR proposed adding custom time-to-content spans that wait on them. + +Figures are `environment:production`, project `metamask` (273505), window 2026-08-19 to 2026-09-17, grouped by `transaction`. Counts move with the window, so re-run before citing them. + +No Sentry Explore link is given, deliberately. Explore takes a relative `statsPeriod`, so a constructed URL would return a different window from the figures printed here and read as a citation that refutes them. Reproduce these through the events API with an absolute window, and check `meta.dataScanned` is `full`. + +### `promotionalBanner`: already under the transaction a new span would measure + +``` +span.op:http.client span.description:*promotionalBanner* +``` + +This is the Contentful `entries` request the carousel issues from the UI process ([`fetchCarouselSlidesFromContentful.ts` L132-L136](https://github.com/MetaMask/metamask-extension/blob/7d02d2d041b0d7817c577b2dd893dc7db42060a3/ui/hooks/useCarouselManagement/fetchCarouselSlidesFromContentful.ts#L132-L136)). 26,251,098 spans over 6 transactions. Four of the six are page transactions and hold all but 13,694 of the total. + +| transaction | spans | p50 | +|---|---|---| +| `/sidepanel.html` | 21,882,917 | 60.7 ms | +| `/popup.html` | 2,918,216 | 77.2 ms | +| `/home.html` | 1,422,554 | 57.5 ms | +| `/notification.html` | 13,717 | 51.8 ms | + +The other two are `UI Startup` at 12,554 and `Bridge Balances Updated` at 1,140. + +The request runs in the UI process, so its span is already a child of the page transaction a time-to-content span would be measuring. The per-page duration is readable today, by page, from the table above. A new name over that interval adds a label rather than a measurement, and the reviewer's question to the author is what it measures that the `p50` column does not, such as time after the response in Redux propagation or render. + +### `POST /api/v4/notifications`: recorded, but under whichever trace was active + +``` +span.op:http.client span.description:*notification* +``` + +The figures below are the rows for `POST https://notification.api.cx.metamask.io/api/v4/notifications`. The filter above is broader than that endpoint and returns more. 1,020,538 spans over 16 transactions. + +| transaction | spans | +|---|---| +| `Provider Create Accounts (v2 - batched)` | 197,600 | +| `/service-worker.js` | 192,207 | +| `BackendWebSocketService Connection` | 186,468 | +| `Background RPC: fetchAndUpdateMetamaskNotifications` | 26,113 | + +The UI thunk hands the work across the RPC boundary ([`actions.ts` L7170-L7172](https://github.com/MetaMask/metamask-extension/blob/7d02d2d041b0d7817c577b2dd893dc7db42060a3/ui/store/actions.ts#L7170-L7172)), so the request is issued in the background, where it is recorded only if some span happens to be active at that moment (the parent condition, `knowledge/auto-instrumentation.md`). The transaction named for the operation holds 26,113 of the 1,020,538, about 2.6%, and the three largest parents have nothing to do with the notification list. + +For what a dedicated span could add beyond these, the other half of the answer is `instrumentation` → Debounced Background State Update, which records the lag between a background response and the store update. + +So the spans exist and no query reads a list latency off them, because the grouping tracks whatever else the background was doing. That is a real argument for a dedicated name here, and it is the argument the banner case does not have. Review the two on their own merits rather than on the count they share. diff --git a/domains/observability/skills/sentry-quota/skill.md b/domains/observability/skills/sentry-quota/skill.md index be000d0c..8ec3363d 100644 --- a/domains/observability/skills/sentry-quota/skill.md +++ b/domains/observability/skills/sentry-quota/skill.md @@ -28,7 +28,7 @@ A custom span is a quota risk when these stack. The first three together are the | 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 | +| **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 | | Hot path | data-source / update-pipeline / network callback, not a discrete user action | high call frequency | @@ -41,10 +41,10 @@ Low fan-out + discrete user action + already gated = fine. Don't flag healthy sp ### 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. +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. For a timing span, also check whether the requests it waits on are already recorded as automatic `http.client` spans, and ask what the new span adds beyond them (`knowledge/auto-instrumentation.md`). 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. +5. If the diff adds no `trace(` sites, no `TraceName` entries, **no `trace` argument passed into a call, bumps no dependency, adds or changes no SDK integration, and adds or re-triggers no network request** → "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. A request the diff adds, or makes fire more often, becomes an `http.client` span whenever a span is active when it starts, so estimate it as requests × triggers × the kept rate of the trace it joins (`knowledge/auto-instrumentation.md`). > **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. @@ -87,4 +87,5 @@ Tier 0 + 1 stop the bleed; Tier 2 is the follow-up so the metric returns. | 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 | +| Count only `trace(` sites | Most child spans are automatic: in the extension project, `http.client` is about nine in ten billed child spans. Review added requests, refetch triggers and requests moved into a `trace()` callback the same way as a new span (`knowledge/auto-instrumentation.md`) | | 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. |