Skip to content

perf(telemetry): defer PostHog and skip disabled sessions - #33896

Closed
OrcaWin wants to merge 7 commits into
mainfrom
OrcaWin/mem-06-telemetry
Closed

OrcaWin wants to merge 7 commits into
mainfrom
OrcaWin/mem-06-telemetry

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

ELI5

Disabled and unofficial sessions skip loading PostHog. Enabled sessions keep consent/event validation synchronous and load one SDK owner through the existing first-window deferral. Queued events retain their original capture times. This PR is standalone against main.

What Changed

Use one retryable transport import and recheck live build/consent before construction and capture. Deferred capacity shares the SDK’s 5,000-event limit; the existing 1,000-event session cap remains tighter. Explicit opt-out saves false and discards ordinary pending events immediately, while preserving the existing admitted opt-out control event.

Shutdown closes admission and joins accepted work within the existing two-second caller budget. A constructed SDK receives shutdown exactly once, including shutdown(0) when accepted work consumes the allowance, so its flush timer is retired. Late imports cannot construct an owner after retirement. No protocol, settings schema, new process or shutdown call site is introduced.

Final head 65e22a1f6d59ad3927c75960ed38b86de24ad11c is based on main 77130982faf. The consent entrypoints catch/log an SDK startup failure and still admit their bounded events for later retry; direct transport startup keeps its rejection contract. This fixes a reproduced new failure where consent persisted but app_opened/opt-in events were lost even after retry. The final commit also preserves main’s deletion of an adapted low-value cohort test, resolving its modify/delete conflict without restoring it.

Measurements

Nine alternating samples per ref/scenario in fresh Linux Node26.8.1 processes compare final65e22 against exact main77130982 through the bundled production telemetry seam with the actual installed SDK and intercepted HTTP. User-disabled and environment-disabled sessions avoid 95 SDK modules and about 4.35 MiB retained heap; unofficial sessions avoid about 2.53 MiB. These are isolated subsystem measurements, not whole-app PSS or app startup/paint measurements.

Enabled demand loads the same 95 modules and converges to the parent footprint. Its final65 isolated demand median is65.3 ms versus2.7 ms with the parent already initialized: loading is deferred, not moved off the main thread. No enabled steady-state memory saving is claimed.

Validation

  • Final production-focused telemetry/usage checks: 20 files / 267 tests and the flaky-test ratchet passed. Both new settings/banner recovery tests fail before the fix and pass after it, preserving consent and exactly the expected queued events on retry.
  • Independent reviewed production6b4 passed 46 deterministic race controls and24 fresh-process actual-SDK scenarios, covering consent, transient failed-import retry, short sessions, accepted opt-out/quit, shared deadline, original timestamps, late-import fences and zero remaining-budget cleanup. The final65 consent fix additionally passes eight owner actual-SDK entrypoint recovery controls. Fresh independent final-delta review passed 20 pinned red/green consent cases,12 actual-SDK consent cases and four final deadline cases, with no pending introduced blocker.
  • Actual installed-SDK parent comparison reproduced the new owner’s retained 10-second flush timer at exhausted quit budget; the fix removes it. Four cleanup assertions fail before the fix and pass after it.
  • Current-main related daemon selection passes 115 tests, including the previously failing /fake fixtures. Main includes the sequential-import mock-race fix; this PR adds no permissions workaround or baseline waiver.
  • Final65e22 ORCA_BACKGROUND_LAUNCH=1 pnpm check:changed --maxWorkers=2 passed all 19 checks in830.4s. Final PR CI40430982152 and headless CI40430982048 passed; all17 non-skipped hosted checks are green. GitHub reports mergeable/clean, with no unresolved review thread. No baseline waiver or earlier-head result substitutes for this final gate.
  • Fresh hidden Electron proof at final65e22 passed real banner opt-out and sequential Privacy on/off/on:0→95 actual SDK modules, exactly five expected events across three actual batches, one SDK shutdown(2000), final queued event delivered, flush timer cleared and graceful exit0 in54ms. Main was rebuilt from the final source; all1202 preload/renderer files match the prior full build by hash. Ordinary output defaults were restored and owned app/display/profile cleanup passed.

Tradeoffs and limits

SDK evaluation remains on Electron main. The two-second caller budget is cooperative and cannot preempt synchronous JavaScript. Public SDK shutdown(0) may start best-effort I/O after the caller budget; it clears the flush timer but does not hard-cancel already issued SDK HTTP. Slow import, endpoint failure or deadline exhaustion can lose best-effort events. The existing overlapping off→on issue also reproduces on main and remains outside this loading change.

Native signed installation, production-endpoint delivery and live remote/mobile producers are not newly executed by the Linux controls. The historical combined-stack percentage is not a measurement of this standalone implementation. Benchmark scripts and reports remain external validation artifacts; #33912 has been reverted in #34162.

Visual Proof

Final65 hidden Playwright CLI/CDP run on isolated Linux Xvfb uses a synthetic official identity/dummy key and intercepts actual installed-SDK HTTP. It clicks the banner Opt out and Privacy on/off/on controls, verifying saved consent, rendered switch states and exact emitted batches. Both screenshots were directly inspected. The fixture launches with DNT to install interception, then clears DNT only in its owned main process; diagnostic availability is latched at startup, so its separate diagnostic-file warning remains visible. This is disclosed fixture setup, not a normal user DNT transition or production-endpoint proof.

Final saved consent renders the switch off

Final explicit opt-in renders the switch on

Agent skill upstream boundary

  • Not applicable; no upstream skill-installer content copied.

@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-06-telemetry branch from ac69283 to 6778f87 Compare October 26, 2026 10:43
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-05-yaml-boundary branch from d03bb76 to 2e12f47 Compare October 26, 2026 10:43
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-06-telemetry branch from 6778f87 to c01dd83 Compare October 26, 2026 10:52
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-05-yaml-boundary branch from 2e12f47 to 8e98f94 Compare October 26, 2026 10:52
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-05-yaml-boundary branch from 8e98f94 to d012458 Compare October 26, 2026 11:22
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-06-telemetry branch 2 times, most recently from 2fa7725 to c01dd83 Compare October 26, 2026 12:01
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-05-yaml-boundary branch from d012458 to 8e98f94 Compare October 26, 2026 12:01
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-06-telemetry branch from c01dd83 to 86f3699 Compare October 26, 2026 22:17
@OrcaWin
OrcaWin changed the base branch from OrcaWin/mem-05-yaml-boundary to main October 26, 2026 22:17
@OrcaWin
OrcaWin force-pushed the OrcaWin/mem-06-telemetry branch from bc7d27f to 70f292b Compare October 27, 2026 00:53
@OrcaWin
OrcaWin marked this pull request as ready for review October 27, 2026 00:53
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4c7137cc-c57b-4291-a2b8-698bc06c8ec7
Commits

Reviewing files that changed from the base of the PR and between 70f292b and 65e22a1.

Files selected for processing: 3
  • src/main/telemetry/client-onboarding-language-cohort.test.ts
  • src/main/telemetry/client-transport-startup.test.ts
  • src/main/telemetry/client.ts
Files with no reviewable changes: 1
  • src/main/telemetry/client-onboarding-language-cohort.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Walkthrough

Telemetry now initializes its transport on demand and can queue captures until startup. The transport applies consent and environment checks, handles opt-out captures, and limits shutdown to a two-second budget. Main-process startup schedules transport initialization after the first window appears, with a 15-second fallback. Tests cover startup, capture delivery, opt-out and shutdown behavior, retry handling, queue limits, and capture timestamps.

Priority: Low

Merge Risk: Minimal · up to 65e22

Telemetry now loads lazily, and a failure to start the transport no longer breaks the user's consent actions. No remaining merge-blocking risk was found in the reviewed changes.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check Warning The description follows the template's main sections (ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, AI Disclosure, Review, Agent skill upstream boundary, Notes, Checklist) with substan… 1) Add a 'Why' section that states the problem (PostHog loads for disabled and unofficial sessions) and explains why deferring SDK loading through runAfterFirstWindowShown is better than the alternatives considered. 2) Fill in the 'Linked I…
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title "perf(telemetry): defer PostHog and skip disabled sessions" matches the main change. The diff defers PostHog SDK loading through runAfterFirstWindowShown, and disabled or unofficial sessions…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "perf(telemetry): defer PostHog and skip disabled sessions" matches the main change. The diff defers PostHog SDK loading through runAfterFirstWindowShown, and disabled or unofficial sessions skip loading the SDK. The title is specific, follows a conventional commit format, and is 57 characters long.

Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 14 functions across 14 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Full details: Description check

Explanation

The description follows the template's main sections (ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, AI Disclosure, Review, Agent skill upstream boundary, Notes, Checklist) with substantial detail on mechanism, validation, tradeoffs, and visual proof. However, the required 'Why' section is missing: there is no explicit rationale comparing the chosen approach against alternatives. The 'Linked Issue' section does not reference an issue number (it says 'Fixes #' with nothing filled in) and the description references PR #33896 only in the objectives, not as a linked issue. The Testing checkboxes are not checked, and the 'Automated tests' item is not explicitly marked. The description is long and dense, and it uses unexplained internal identifiers (e.g., commit hashes, 'production6b4', 'final65'), which reduces clarity for reviewers. Overall the description is mostly complete, but the missing Why section and empty Linked Issue are material gaps.

Resolution

  1. Add a 'Why' section that states the problem (PostHog loads for disabled and unofficial sessions) and explains why deferring SDK loading through runAfterFirstWindowShown is better than the alternatives considered. 2) Fill in the 'Linked Issue' section with an actual issue number (replace 'Fixes #'); outside contributors' PRs are ignored without one. 3) Check the Testing checkboxes or state which platforms were manually tested (the description says Linux only; state this explicitly). 4) Replace internal labels such as 'final65', 'production6b4', and commit hashes with plain descriptions, or move them to a separate validation log. 5) Shorten the ELI5 and What Changed sections so the scope is clear to reviewers.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 165e13b2-9387-4a01-912d-6c7d03cf9f5d
Commits

Reviewing files that changed from the base of the PR and between 5b4c779 and 70f292b.

Files selected for processing: 15
  • src/main/startup/main-process-observers-hang-breadcrumb.test.ts
  • src/main/startup/main-process-observers.ts
  • src/main/telemetry/capture-queue-limit.ts
  • src/main/telemetry/client-agent-dashboard-active.test.ts
  • src/main/telemetry/client-lifecycle.test.ts
  • src/main/telemetry/client-onboarding-language-cohort.test.ts
  • src/main/telemetry/client-opt-out-shutdown.test.ts
  • src/main/telemetry/client-sdk-delivery.test.ts
  • src/main/telemetry/client-transport-startup.test.ts
  • src/main/telemetry/client.ts
  • src/main/telemetry/consent.ts
  • src/main/telemetry/deferred-transport.ts
  • src/main/telemetry/posthog-transport.ts
  • src/main/telemetry/session-properties.ts
  • src/main/usage/usage-provider-store-lifecycle.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Comment thread src/main/telemetry/client.ts Resolved
@nwparker nwparker closed this Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants