diff --git a/docs/design/system-traces-expansion.md b/docs/design/system-traces-expansion.md index 1b494eb6..7e93395f 100644 --- a/docs/design/system-traces-expansion.md +++ b/docs/design/system-traces-expansion.md @@ -501,7 +501,7 @@ shared `PageLifecycleTracker`, delete the legacy samplers, then ST1+ traces as p **Status (2026-10): shipped.** ST0′ (#55), the browser trackers (#62), `PageLifecycleTracker` (#63) and the node legacy-sampler deletion (#65; the browser sampler went with #62) are merged, so the bare `sample()` adapter no longer coexists with the producers on browser or node: the legacy browser sampler (which also emitted `battery`/`charging`, the duplicate rows an interim reading of this section implied) is -deleted and `systemMetricsSampler` survives only as an explicit caller override that REPLACES the heap, connection, orientation, `app_state` and `fps` sensor traces; the `battery` producer still runs alongside it (`browser/src/launch.ts:925-933`), so a sampler that also emits `battery`/`charging` would duplicate those rows. This is a code-level gap against the one-writer rule (#132) and is NOT fixed (the override at `packages/browser/src/launch.ts:1096-1108` neither filters those names nor suppresses the producer); it is tracked as a code follow-up, outside this docs PR. +deleted and `systemMetricsSampler` survives only as an explicit caller override that REPLACES the heap, connection, orientation, `app_state` and `fps` sensor traces; the `battery` producer still runs alongside it (`browser/src/launch.ts`, the `createSystemTracesProvider` wiring), so a sampler that also emits `battery`/`charging` would have duplicated those rows. Fixed (#154): the producers that keep running alongside the override own their names (`battery`, `charging`, `battery_charging_time_s`, `battery_discharging_time_s`, `route`, `view`, `url` when captured, and `capture` when replay is on) and `launch` drops sampler rows under them (`withoutNames`/`producerOwnedNames`, `packages/browser/src/launch.ts`), so there is one writer per name; the sampler's other rows are kept. The domain-tracker follow-on (§15) is tracked in §15.5. The node `ProcessStatsTracker` that §14.5 lists for ST0′ was replaced by the node `ProcessTracker` in TR8 part 1 (#87): its files, tests and `@bugsee/node` exports (`ProcessStatsTracker`, `ProcessStats`) are gone, and `packages/trackers/src/node/` is no longer a placeholder. The Electron trackers (ST6 above) shipped as TR9 (#90, #91). The per-signal browser trackers that §14.5 originally listed as remaining (`MemoryTracker`, `OrientationTracker`, `FpsTracker`, `ScreenSizeTracker`, `MainThreadLagTracker`) were folded into the domain trackers by TR2 (#82) and TR5 (#83) and their files are deleted: `packages/browser/src/trackers/` no longer exists on `main`. diff --git a/packages/browser/src/launch.test.ts b/packages/browser/src/launch.test.ts index 3cf95f1d..669f9376 100644 --- a/packages/browser/src/launch.test.ts +++ b/packages/browser/src/launch.test.ts @@ -1356,6 +1356,55 @@ describe('launch', () => { ); }); + it('a caller sampler returning battery/charging does not duplicate the battery producer (one writer per name)', async () => { + vi.stubGlobal('navigator', { getBattery: () => Promise.resolve(batteryManager()) }); + const store = memStore(); + await launchTracked( + 'tok', + baseOptions({ + captureStore: store, + systemMetricsSampler: () => [ + { name: 'ram_js_heap_used', value: 42 }, + { name: 'battery', value: 7 }, + { name: 'charging', value: { state: 1 } }, + { name: 'battery_charging_time_s', value: 111 }, + { name: 'battery_discharging_time_s', value: 222 }, + { name: 'route', value: 'sampler-route' }, + { name: 'view', value: 'sampler-view' }, + { name: 'url', value: 'sampler-url' }, + ], + }), + ); + await new Promise((r) => setTimeout(r, 0)); + const traces = ((await drain(store, 'traces.system')) ?? []).map( + (e) => e.data as { name: string; value: unknown }, + ); + // the producer owns the names: the sampler's rows are dropped, its other rows are kept + expect(traces.filter((t) => t.name === 'battery').map((t) => t.value)).toEqual([50]); + expect(traces.filter((t) => t.name === 'charging').map((t) => t.value)).toEqual([{ state: 2 }]); + expect(traces).toContainEqual(expect.objectContaining({ name: 'ram_js_heap_used', value: 42 })); + const values = traces.map((t) => t.value); + for (const leaked of [111, 222, 'sampler-route', 'sampler-view', 'sampler-url']) { + expect(values).not.toContain(leaked); + } + }); + + it('a caller sampler may still write url when URL capture is off', async () => { + const store = memStore(); + await launchTracked( + 'tok', + baseOptions({ + captureStore: store, + captureUrl: false, + systemMetricsSampler: () => [{ name: 'url', value: 'sampler-url' }], + }), + ); + const traces = ((await drain(store, 'traces.system')) ?? []).map( + (e) => e.data as { name: string; value: unknown }, + ); + expect(traces).toContainEqual(expect.objectContaining({ name: 'url', value: 'sampler-url' })); + }); + it('a battery manager that throws after getBattery resolves reaches options.onError', async () => { const getBattery = vi.fn(() => Promise.resolve({ @@ -3909,6 +3958,42 @@ describe('launch — session replay (lazy)', () => { await vi.waitFor(() => expect(registerReplay).toHaveBeenCalledTimes(1)); }); + it.each([ + { replay: undefined, kept: false }, + { replay: false, kept: true }, + ])('a caller sampler row named `capture` is dropped only while the capture producer exists (replay=$replay)', async ({ + replay, + kept, + }) => { + const store = memStore(); + // A real recorder makes the producer's own `capture` row appear (`active`); `undefined` would leave it unwritten. + if (replay === undefined) registerReplay.mockReturnValueOnce({ stop: vi.fn() }); + await launchTracked( + 'tok', + domReplayOptions({ + captureStore: store, + ...(replay === undefined ? {} : { replay }), + systemMetricsSampler: () => [{ name: 'capture', value: 'sampler-capture' }], + }), + ); + if (replay === undefined) { + await vi.waitFor(() => expect(registerReplay).toHaveBeenCalledTimes(1)); + await vi.waitFor(async () => { + const rows = ((await drain(store, 'traces.system')) ?? []).map( + (e) => e.data as { value: unknown }, + ); + // the producer's row survives the filter, and the caller's same-named row does not + expect(rows.some((r) => (r.value as { state?: string } | null)?.state === 'active')).toBe( + true, + ); + }); + } + const values = ((await drain(store, 'traces.system')) ?? []).map( + (e) => (e.data as { value: unknown }).value, + ); + expect(values.includes('sampler-capture')).toBe(kept); + }); + it('the DEFAULT path resolves to FAIL-CLOSED masking (mask all text/inputs, block all media)', async () => { // The default now applies to EVERY integration that does not opt out, so "fail-closed" has to hold on // the default path and not merely on the opted-in one. Asserting the forwarded options object is `{}` diff --git a/packages/browser/src/launch.ts b/packages/browser/src/launch.ts index 1b24fb71..f9465a4f 100644 --- a/packages/browser/src/launch.ts +++ b/packages/browser/src/launch.ts @@ -22,6 +22,7 @@ import { type NetworkCapture, type TraceSample, type TraceSuppression, + withoutTraceNames, } from '@bugsee/capture'; import { asInternalClient, @@ -82,7 +83,7 @@ import { type TriggerPipeline, type UploadResult, } from '@bugsee/core'; -import { BugseeOption, type EnvironmentEnvelope } from '@bugsee/protocol'; +import { BugseeOption, type EnvironmentEnvelope, TraceName } from '@bugsee/protocol'; import { applicationTrackerFor } from '@bugsee/trackers/browser'; import { reportGuarded } from '@bugsee/util'; import { BROWSER_APP_VERSION_HINT, metaAppIdentity, warnToConsole } from './app-identity'; @@ -568,7 +569,7 @@ export interface BugseeLaunchOptions { systemProbe?: BrowserProbe; /** * LEGACY polled system-traces override. It REPLACES the default heap/connection/orientation traces (as on - * Node); battery stays a tracker-backed producer. The defaults are producers, not a sampler. + * Node); battery stays a tracker-backed producer that (with `url`/`route`/`view`) owns its trace names; sampler rows under them are dropped. The defaults are producers, not a sampler. */ systemMetricsSampler?: () => readonly TraceSample[]; /** Durable bundle store override (crash recovery across reloads). Default none (until B5). */ @@ -662,6 +663,20 @@ function reportInvalidHostOwned(onError: ((error: unknown) => void) | undefined) ); } +/** Names the producers that run alongside a caller sampler (`sensors: false`) own: battery x4, route, view, url when built, and capture when replay is on. */ +function producerOwnedNames(withUrl: boolean, withCapture: boolean): ReadonlySet { + return new Set([ + TraceName.BATTERY, + TraceName.CHARGING, + TraceName.BATTERY_CHARGING_TIME_S, + TraceName.BATTERY_DISCHARGING_TIME_S, + TraceName.ROUTE, + TraceName.VIEW, + ...(withUrl ? [TraceName.URL] : []), + ...(withCapture ? [TraceName.CAPTURE] : []), + ]); +} + // The public composition root's lifecycle shell: publishes Launching while the (async) composition is in // flight, Launched once it has built a client, and falls back to Stopped if it throws. A repeat launch // (a client already owns the slot) leaves the state alone. @@ -1093,11 +1108,19 @@ async function buildLaunch( if (diskPressure !== undefined && resolved.isEnabled(BugseeOption.CaptureDiskSpace)) { captureState?.bindPressure(diskPressure); // the disk STATES are telemetry: behind the opt-in, the protection is not } + const captureUrl = resolved.options.get(CAPTURE_URL_OPTION_KEY, true); client.addCaptureProvider( createSystemTracesProvider({ - // A `systemMetricsSampler` REPLACES the sensor traces (heap, connection, orientation, app_state, fps), as on Node; battery still runs. + // A `systemMetricsSampler` REPLACES the sensor traces (heap, connection, orientation, app_state, fps), as on Node; battery still + // runs alongside it and OWNS its names (battery, charging, the two battery times, route, view, url when captured, capture when replay is on): a sampler row + // under those names is dropped (one writer per trace name, #154). ...(options.systemMetricsSampler !== undefined - ? { sample: options.systemMetricsSampler } + ? { + sample: withoutTraceNames( + options.systemMetricsSampler, + producerOwnedNames(captureUrl, captureState !== undefined), + ), + } : {}), ...(suppression !== undefined ? { suppression } : {}), producers: [ @@ -1108,7 +1131,7 @@ async function buildLaunch( ...(options.systemMetricsSampler !== undefined ? { sensors: false } : {}), captureUserIdle: resolved.options.get(CAPTURE_USER_IDLE_OPTION_KEY, false), // The page's own sanitized address: written from the navigation source, not from the performance extension. - ...(resolved.options.get(CAPTURE_URL_OPTION_KEY, true) + ...(captureUrl ? withUrl( urlProducerDepsFor(win, { ...(carrier !== undefined ? { carrier } : {}), diff --git a/packages/electron/src/host-owned-traces.e2e.test.ts b/packages/electron/src/host-owned-traces.e2e.test.ts index ff56ec8e..308cdaf6 100644 --- a/packages/electron/src/host-owned-traces.e2e.test.ts +++ b/packages/electron/src/host-owned-traces.e2e.test.ts @@ -271,14 +271,15 @@ describe('host-owned traces: Electron renderers write none of the names main own systemMetricsSampler: () => [ { name: 'capture', value: { state: 'active' } }, { name: 'displays', value: [{ id: 0, width: 800, height: 600 }] }, - { name: 'battery', value: 42 }, + // `battery` is owned by the renderer's own producer (a sampler row under it is dropped, #154), so use a sampler-only name + { name: 'app_state', value: 42 }, ], }); r.tick(); await client.logException(new Error('report')); const traces = tracesOf(puts[0] as Uint8Array); expect(traces.some((t) => t.name === 'capture' || t.name === 'displays')).toBe(false); - expect(traces.filter((t) => t.name === 'battery').map((t) => t.value)).toContain(42); // unlisted still flows + expect(traces.filter((t) => t.name === 'app_state').map((t) => t.value)).toContain(42); // unlisted still flows await client.stop(); });