Add runpane wrapper telemetry identity#261
Conversation
parsakhaz
left a comment
There was a problem hiding this comment.
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)
- Race condition in install ID persistence (
packages/runpane/src/telemetry.ts:272-305,packages/runpane-py/src/runpane/telemetry.py:174-204): BothgetOrCreateWrapperInstallIdandget_or_create_wrapper_install_iddo a read-then-write onconfig.jsonwithout file locking. If two wrapper processes start simultaneously (e.g. parallel CI steps that somehow bypass the CI guard, or a user runningnpx runpanein two terminals on first install), each generates a differentinstall_idand 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)
-
Python
WrapperTelemetryContextisDict[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. ATypedDictwould provide static type checking parity and catch key typos at mypy time. Not blocking since Python typing is advisory in this project. -
No unit tests for invocation detection (
detectNpmInvocationin telemetry.ts,detect_python_invocationin 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. -
Python telemetry HTTP call is synchronous/blocking (
telemetry.py:235-246):urllib.request.urlopenblocks 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
wrapperandinvocationproperties - 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_DISABLEDdisable 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.
Summary
Companion website endpoint/docs PR: https://github.com/parsakhaz/runpane-website/pull/102
Refs #260
Testing
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.