Skip to content

Add runpane wrapper telemetry identity#261

Merged
parsakhaz merged 1 commit into
mainfrom
better-tele
Jun 18, 2026
Merged

Add runpane wrapper telemetry identity#261
parsakhaz merged 1 commit into
mainfrom
better-tele

Conversation

@parsakhaz

Copy link
Copy Markdown
Member

Summary

  • add npm and PyPI wrapper lifecycle telemetry with a persisted anonymous install_id
  • reuse Pane config analytics.installId when safe, fall back without overwriting corrupt config, and sanitize telemetry properties
  • alias install:<install_id> into the app analytics identity after consent so wrapper and app funnels can be stitched
  • update the runpane contract/docs and add npm/pip telemetry sanitizer parity coverage

Companion website endpoint/docs PR: https://github.com/parsakhaz/runpane-website/pull/102

Refs #260

Testing

  • pnpm run test:runpane-contract
  • pnpm run typecheck
  • pnpm run lint
  • pnpm --filter runpane typecheck
  • pnpm --filter runpane lint

Note: local Node is v20.19.3, so pnpm prints the repo engine warning for Node >=22.14.0. The commands completed successfully; repo lint still reports existing warnings but exits 0.

@parsakhaz parsakhaz left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

PR Review

Issue context: #260 — add package wrapper install telemetry so the full npm/PyPI funnel (invoke → download → install → succeed/fail) is visible in PostHog, not just artifact download counts.

Quality Gates

  • Typecheck: PASS
  • Lint: PASS (0 errors, 165 pre-existing warnings)

Must-Fix (0)

None.

Should-Fix (1)

  1. Race condition in install ID persistence (packages/runpane/src/telemetry.ts:272-305, packages/runpane-py/src/runpane/telemetry.py:174-204): Both getOrCreateWrapperInstallId and get_or_create_wrapper_install_id do a read-then-write on config.json without file locking. If two wrapper processes start simultaneously (e.g. parallel CI steps that somehow bypass the CI guard, or a user running npx runpane in two terminals on first install), each generates a different install_id and one overwrites the other. This fragments the identity the telemetry is designed to preserve. Given this only matters on the very first run and telemetry is best-effort, this is low-severity, but worth noting for the record.

Suggestions (3)

  1. Python WrapperTelemetryContext is Dict[str, Any] (packages/runpane-py/src/runpane/telemetry.py:16): The TS side uses a proper typed interface (WrapperTelemetryContext), but the Python side is an untyped dict. A TypedDict would provide static type checking parity and catch key typos at mypy time. Not blocking since Python typing is advisory in this project.

  2. No unit tests for invocation detection (detectNpmInvocation in telemetry.ts, detect_python_invocation in telemetry.py): These functions have meaningful branching logic (user-agent parsing, env var checks) that would benefit from targeted unit tests. The contract tests cover sanitizer parity but not invocation detection. Could be a follow-up.

  3. Python telemetry HTTP call is synchronous/blocking (telemetry.py:235-246): urllib.request.urlopen blocks the process for up to 1.5s on timeout. The TS version is async so at least it doesn't block the event loop. For a CLI wrapper this is acceptable, but if startup latency is ever a concern, a fire-and-forget subprocess or a non-blocking approach would help. Not blocking.

Completeness (vs. issue #260)

All acceptance criteria are met:

  • npm vs PyPI wrapper invocations are distinguishable via wrapper and invocation properties
  • Commands (setup, install client/daemon, update, doctor) are distinguishable via command, resolved_command, target
  • Start/success/failure/fallback outcomes are tracked as separate events
  • Events include non-sensitive context (wrapper, wrapper_version, invocation, command, platform, arch, pane_version, download_source)
  • Sanitizers strip paths, bound exit codes, drop oversized strings
  • CI and RUNPANE_TELEMETRY_DISABLED disable telemetry
  • Contract, docs, and generated types are updated
  • Tests cover npm/Python sanitizer parity and sensitive value assertions
  • install:<install_id> is aliased into the app PostHog identity after consent

One item from the issue: "The PostHog event catalog and analytics attribution docs are updated with the final event names and properties" appears to be covered in the companion website PR (https://github.com/parsakhaz/runpane-website/pull/102).

Summary

Clean, well-structured PR. The telemetry module design is solid: proper sanitization, CI gating, fallback identity persistence for corrupt configs, and consistent npm/Python parity. The frontend install identity aliasing follows the established web visitor pattern exactly. The contract test additions give good confidence in cross-language consistency. Ready to merge.

@parsakhaz parsakhaz merged commit 5a4439c into main Jun 18, 2026
16 checks passed
@parsakhaz parsakhaz deleted the better-tele branch June 18, 2026 05:41
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.

1 participant