fix(cli): support DO_NOT_TRACK and ensure lazy telemetry initialization#2066
Conversation
|
|
- Support industry-standard DO_NOT_TRACK environment variable. - Defer PostHog client initialization until the first send_event call. - Fix Windows spawn EINVAL error by adding shell: true for .cmd execution.
Skywalkingzulu1
force-pushed
the
fix/telemetry-leak-and-windows-spawn-einval
branch
from
f660713 to
e0c2b5d
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI telemetry pipeline to better respect user opt-out signals and to avoid initializing the PostHog client until it’s actually needed, and it also fixes a Windows-specific spawn failure in the npm wrapper.
Changes:
- Add support for
DO_NOT_TRACKto disable analytics in the CLI entrypoint. - Make
PostHogSenderlazily initialize the PostHog client viatokio::sync::OnceCell(no client creation during sender construction). - On Windows, spawn the npm bootstrap command with
shell: trueto avoid.cmdEINVALerrors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/telemetry/src/send_event.rs | Refactors PostHog sender to lazily create the client on first use (and in the panic hook). |
| crates/cli/src/main.rs | Expands analytics-disable detection to include DO_NOT_TRACK (and env-driven disablement). |
| crates/cli/npm/codemod | Adds shell: true for win32 when spawning the bootstrap npm.cmd path. |
Comments suppressed due to low confidence (1)
crates/cli/npm/codemod:155
- The new Windows-only
shellbehavior isn’t covered by the existing wrapper tests (they only exercise the linux path). Adding a unit test case that runsrun({ argv: [], platform: "win32", ... })and assertsspawnImplreceives{ shell: true }would help prevent regressions for the.cmdEINVAL fix.
function spawnLatestCli({ env = process.env, platform = process.platform, spawnImpl = spawn }) {
return spawnImpl(npmExecutable(platform), latestBootstrapArgs(), {
stdio: "inherit",
shell: platform === "win32",
env: {
...env,
[CODEMOD_BOOTSTRAPPED_ENV]: "1",
},
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let is_analytics_disabled = cli.disable_analytics | ||
| || std::env::var("DO_NOT_TRACK").is_ok() | ||
| || std::env::var("DISABLE_ANALYTICS").is_ok() | ||
| || implicit_cli_params | ||
| .as_ref() | ||
| .map(|params| params.disable_analytics) | ||
| .unwrap_or(false) | ||
| { | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
DO_NOT_TRACK and DISABLE_ANALYTICS are treated as disabled when the env var merely exists (even if set to "0"/"false"/empty). This is a behavior change that can unintentionally force-disable analytics. Consider parsing these env vars the same way you later do for DISABLE_ANALYTICS (e.g., only treat "1"/"true" (case-insensitive) as disabled), and keep --disable-analytics as the explicit override.
|
Thank you so much! @Skywalkingzulu1 Could you please address the comment by Copilot? |
|
@Skywalkingzulu1 do you want to wrap this up so we can get it merged? thanks for the great work and contribution. |
Summary
This PR addresses Issue #2058 (telemetry leak) and a Windows-specific \spawn EINVAL\ error.
Changes
Verified locally in a win32 environment.