From 8bf512685b5616918beed931db8718824c41ef59 Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Tue, 4 Aug 2026 10:31:06 +0200 Subject: [PATCH 1/5] feat(python-setup): setup attempt/result telemetry *Why* The VPEX setup flow is uninstrumented: we cannot see how often setup runs, where it breaks, or how long it takes -- the numbers the ERD's Metrics section asks for. DECO-27787. *What* Adds two events emitted from the setup orchestrator, following the existing PackageManagerTelemetry/packageManagerExtensions split (module augmentation for the emit half, injected deps for collection): - python_env.setup.attempt: package manager, target type, serverless version, mode, is-greenfield. Recorded once the compute target resolves and a CLI run is about to start. - python_env.setup.result: outcome (ok/failed/cancelled/not_started), failure phase, error code, env key, disk-mutated, duration. recordPythonSetupAttempt returns the result reporter, so the 1:1 attempt/result pairing is structural rather than a convention. Two ERD fields are not taken from the CLI result, because it has no producer for either: - duration is measured in-extension (the CLI documents durationMs as reserved and always 0), which also captures the latency the user actually experiences, including spawn and interpreter adoption; - the merge-conflict warning count is omitted -- nothing in the CLI ever appends to Result.Warnings, so the field would be a permanent 0 and would read as "merge quality is perfect" rather than "unmeasured". `adopt` is added as a synthetic seventh failure phase for the extension-side interpreter adoption step, which happens after the CLI exits and so cannot appear in its own phases[]. Categorical/enum data only -- no cluster IDs/names, paths, or package names; absent optionals are omitted rather than stringified to "undefined". Emission is fully best-effort: a detection, emit, or report failure can never break the setup run it measures. Schema docs in src/telemetry/PYTHON_SETUP_TELEMETRY.md. telemetry.json is generated from EventTypes (scripts/generateTelemetry.ts), so it needs no manual edit. *Verification* - yarn build: clean - yarn test:lint: 0 errors, prettier clean - unit tests: 461 passing, +14 new (attempt/result per outcome path, no-attempt for each early return, pairing invariant, greenfield conditionality, no-cluster-id, telemetry-failure resilience). The 6 failures in cli/CliWrapper.test.ts are pre-existing on the base commit (7 there, one flaky) and untouched by this change. - regenerated telemetry.json and confirmed both events serialize. Co-authored-by: Isaac --- packages/databricks-vscode/src/extension.ts | 1 + .../PythonSetupEnvironmentSetup.test.ts | 274 ++++++++++++++++++ .../PythonSetupEnvironmentSetup.ts | 131 ++++++++- .../controllers/pythonSetupDeps.test.ts | 7 + .../controllers/pythonSetupDeps.ts | 20 ++ .../src/telemetry/PYTHON_SETUP_TELEMETRY.md | 159 ++++++++++ .../src/telemetry/constants.ts | 110 +++++++ .../telemetry/pythonSetupExtensions.test.ts | 167 +++++++++++ .../src/telemetry/pythonSetupExtensions.ts | 101 +++++++ 9 files changed, 969 insertions(+), 1 deletion(-) create mode 100644 packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md create mode 100644 packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts create mode 100644 packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts diff --git a/packages/databricks-vscode/src/extension.ts b/packages/databricks-vscode/src/extension.ts index 092e818db..96bf589bc 100644 --- a/packages/databricks-vscode/src/extension.ts +++ b/packages/databricks-vscode/src/extension.ts @@ -878,6 +878,7 @@ export async function activate( append: (chunk) => getPythonSetupLogChannel().append(chunk), show: () => getPythonSetupLogChannel().show(true), }, + telemetry, }) ); context.subscriptions.push( diff --git a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts index 4065ded43..695b6442a 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts @@ -14,6 +14,30 @@ import { ERROR_NO_TARGET, } from "../models/fixtures/setupLocalResults"; import {SetupLocalInvocation} from "../utils/setupLocalArgs"; +import { + PythonSetupAttempt, + PythonSetupOutcomeReport, +} from "../../telemetry/pythonSetupExtensions"; + +/** + * Records the attempt/result telemetry the orchestrator emits. Stands in for + * `Telemetry.recordPythonSetupAttempt`, keeping its shape: recording an attempt + * hands back the reporter for that run's outcome. + */ +function makeTelemetryRecorder() { + const attempts: PythonSetupAttempt[] = []; + const results: PythonSetupOutcomeReport[] = []; + return { + attempts, + results, + recordSetupAttempt: (attempt: PythonSetupAttempt) => { + attempts.push(attempt); + return (report: PythonSetupOutcomeReport) => { + results.push(report); + }; + }, + }; +} /** * A never-cancelled {@link CancellationLike} that can be flipped via `cancel()`, @@ -82,6 +106,11 @@ function makeDeps( // Mirror the production wrapper: hand the task a log sink and a // (never-cancelled) progress token. withProgress: async (_title, task) => task(() => {}, makeToken()), + // Telemetry defaults to a no-op sink; tests that assert on events pass + // a recorder instead. + recordSetupAttempt: () => () => {}, + getPackageManager: async () => "uv", + hasPyprojectToml: async () => true, ...overrides, }; } @@ -519,3 +548,248 @@ describe("PythonSetupEnvironmentSetup.setup", () => { expect(succeeded).to.have.length(0); }); }); + +describe("PythonSetupEnvironmentSetup telemetry", () => { + it("records an attempt and an ok result on a successful run", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({...telemetry, getPackageManager: async () => "uv"}) + ); + + await setup.setup(); + + expect(telemetry.attempts).to.deep.equal([ + { + packageManager: "uv", + targetType: "serverless", + serverlessVersion: "5", + mode: "default", + // hasPyprojectToml defaults to true, so this is not greenfield. + isGreenfield: false, + }, + ]); + expect(telemetry.results).to.deep.equal([ + {outcome: "ok", envKey: SUCCESS_REAL_RUN.compute!.envKey}, + ]); + }); + + it("omits serverlessVersion and reports cluster for a cluster target", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + resolveCompute: async () => ({ + kind: "cluster", + clusterId: "0710-abc", + }), + }) + ); + + await setup.setup(); + + expect(telemetry.attempts).to.have.length(1); + expect(telemetry.attempts[0].targetType).to.equal("cluster"); + // Never emit a cluster id: the attempt carries only the target *kind*. + expect(telemetry.attempts[0].serverlessVersion).to.equal(undefined); + expect(JSON.stringify(telemetry.attempts[0])).to.not.contain("0710"); + }); + + it("reports isGreenfield when the project has no pyproject.toml", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + getPackageManager: async () => "unknown", + hasPyprojectToml: async () => false, + }) + ); + + await setup.setup(); + + expect(telemetry.attempts[0].isGreenfield).to.equal(true); + }); + + it("omits isGreenfield for a non-uv project (the signal is unreliable there)", async () => { + const telemetry = makeTelemetryRecorder(); + let probed = 0; + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + // pip/conda users may never have a pyproject.toml, so its + // absence says nothing about greenfield-ness. + getPackageManager: async () => "pip", + hasPyprojectToml: async () => { + probed += 1; + return false; + }, + }) + ); + + await setup.setup(); + + expect(telemetry.attempts[0].packageManager).to.equal("pip"); + expect(telemetry.attempts[0].isGreenfield).to.equal(undefined); + // Not even probed: the answer could not be reported either way. + expect(probed).to.equal(0); + }); + + it("reports the failure phase, error code and disk state on CLI failure", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({...telemetry, cli: makeCli({resolve: ERROR_NO_TARGET})}) + ); + + await setup.setup(); + + expect(telemetry.attempts).to.have.length(1); + expect(telemetry.results).to.deep.equal([ + { + outcome: "failed", + failurePhase: ERROR_NO_TARGET.error!.failurePhase, + errorCode: ERROR_NO_TARGET.error!.code, + envKey: ERROR_NO_TARGET.compute?.envKey, + diskMutated: ERROR_NO_TARGET.error!.diskMutated, + }, + ]); + }); + + it('reports the synthetic "adopt" phase when interpreter adoption fails', async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + adoptInterpreter: async () => { + throw new Error("could not select interpreter"); + }, + }) + ); + + await setup.setup(); + + // The CLI exited ok, so there is no CLI error code — but the flow + // failed, at the extension's own phase. + expect(telemetry.results).to.deep.equal([ + { + outcome: "failed", + failurePhase: "adopt", + envKey: SUCCESS_REAL_RUN.compute!.envKey, + }, + ]); + }); + + it("reports cancelled when the user aborts the run", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + cli: makeCli({reject: new PythonSetupCancelledError()}), + }) + ); + + await setup.setup(); + + // Distinct from `failed`: the user gave up, nothing broke. + expect(telemetry.results).to.deep.equal([{outcome: "cancelled"}]); + }); + + it("reports not_started when the CLI produces no result", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + cli: makeCli({reject: new Error("spawn databricks ENOENT")}), + }) + ); + + await setup.setup(); + + // A spawn/parse error has no result object, so there is no phase or + // error code to attribute the break to. + expect(telemetry.results).to.deep.equal([{outcome: "not_started"}]); + }); + + it("records nothing when the run never starts", async () => { + for (const overrides of [ + {projectRoot: () => undefined}, + {isVisible: async () => false}, + {resolveCompute: async () => undefined}, + ]) { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({...telemetry, ...overrides}) + ); + + await setup.setup(); + + // No CLI ran, so there is no attempt to pair a result with. These + // clicks are covered by python_env.setup.detected instead. + expect(telemetry.attempts).to.have.length(0); + expect(telemetry.results).to.have.length(0); + } + }); + + it("records exactly one result per attempt, including across runs", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup(makeDeps(telemetry)); + + // Two coalesced calls (one run), then a second, separate run. + await Promise.all([setup.setup(), setup.setup()]); + await setup.setup(); + + expect(telemetry.attempts).to.have.length(2); + expect(telemetry.results).to.have.length(2); + }); + + it("completes the setup even when the telemetry emit itself throws", async () => { + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + recordSetupAttempt: () => { + throw new Error("telemetry blew up"); + }, + }) + ); + + await setup.setup(); + + // Measurement must never break the flow it measures. + expect(setup.ready).to.equal(true); + }); + + it("completes the setup even when the result reporter throws", async () => { + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + recordSetupAttempt: () => () => { + throw new Error("reporter blew up"); + }, + }) + ); + + await setup.setup(); + + expect(setup.ready).to.equal(true); + }); + + it("still records the attempt when gathering its context fails", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + getPackageManager: async () => { + throw new Error("detection blew up"); + }, + }) + ); + + await setup.setup(); + + // Telemetry must never cost the user their setup run: the attempt + // degrades to `unknown` rather than propagating. + expect(setup.ready).to.equal(true); + expect(telemetry.attempts).to.have.length(1); + expect(telemetry.attempts[0].packageManager).to.equal("unknown"); + expect(telemetry.attempts[0].isGreenfield).to.equal(undefined); + expect(telemetry.results).to.deep.equal([ + {outcome: "ok", envKey: SUCCESS_REAL_RUN.compute!.envKey}, + ]); + }); +}); diff --git a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts index eb17b1814..df040ea90 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts @@ -13,6 +13,11 @@ import { NO_COMPUTE_TARGET_MESSAGE, } from "../utils/errorMessages"; import {SetupLocalInvocation} from "../utils/setupLocalArgs"; +import { + PythonSetupAttempt, + PythonSetupResultReporter, +} from "../../telemetry/pythonSetupExtensions"; +import {PrimaryManager} from "../../language/packageManagerDetection"; /** * The one method the orchestrator needs from {@link PythonSetupCliClient}, typed @@ -108,6 +113,53 @@ export interface PythonSetupSetupDeps { showSuccess: (result: PythonSetupResult) => Promise; withProgress: (title: string, task: ProgressTask) => Promise; + + /** + * Record that a setup run is starting, returning the reporter for its + * outcome. Injected (rather than taking a `Telemetry`) so the flow's tests + * assert on plain recorded values with no telemetry client in sight. + * + * Called only once a run is actually about to spawn the CLI, so every + * attempt has exactly one outcome. Clicks that stop earlier (no compute + * attached, gate closed) are already covered by the + * `python_env.setup.detected` event's `explicit_command` trigger. + */ + recordSetupAttempt: ( + attempt: PythonSetupAttempt + ) => PythonSetupResultReporter; + + /** + * The project's detected package manager, for the attempt event. Reads the + * same detection the visibility gate runs; `undefined` when detection was + * unavailable, in which case the attempt reports `unknown`. + */ + getPackageManager: () => Promise; + + /** + * Whether the project has no `pyproject.toml` yet. Consulted only when the + * detected manager is uv/unknown — see {@link greenfieldSignal}. + */ + hasPyprojectToml: (projectRoot: string) => Promise; +} + +/** + * The greenfield flag for the attempt event, or `undefined` to omit it. + * + * A missing `pyproject.toml` only means "greenfield" for a project that has no + * competing manager: pip and conda users may never have one, so for them the + * absence says nothing and reporting it would inflate the greenfield rate. The + * signal is therefore emitted only for uv/unknown projects — which is exactly + * the population the visibility gate admits. + */ +async function greenfieldSignal( + manager: PrimaryManager, + projectRoot: string, + hasPyprojectToml: (projectRoot: string) => Promise +): Promise { + if (manager !== "uv" && manager !== "unknown") { + return undefined; + } + return !(await hasPyprojectToml(projectRoot)); } /** @@ -201,6 +253,12 @@ export class PythonSetupEnvironmentSetup implements Disposable { compute, }; + // From here a run really happens, so the attempt is recorded and every + // exit below reports an outcome. The reporter also starts the clock: + // the duration we publish is the whole user-visible wait, including CLI + // spawn and interpreter adoption. + const reportResult = await this.recordAttempt(invocation, cwd); + let result: PythonSetupResult; try { result = await withProgress( @@ -210,15 +268,26 @@ export class PythonSetupEnvironmentSetup implements Disposable { } catch (e) { // A cancelled run is a user action, not a failure: stay quiet. if (e instanceof PythonSetupCancelledError) { + reportResult({outcome: "cancelled"}); return; } // Spawn/parse errors reject with a real Error carrying CLI stderr; - // there is no result to map, so surface the message directly. + // there is no result to map, so surface the message directly. No + // result object exists, hence `not_started` rather than `failed`: + // there is no phase or error code to attribute the break to. + reportResult({outcome: "not_started"}); await this.deps.showError((e as Error).message); return; } if (!isLocalEnvironmentReady(result)) { + reportResult({ + outcome: "failed", + failurePhase: result.error?.failurePhase, + errorCode: result.error?.code, + envKey: result.compute?.envKey, + diskMutated: result.error?.diskMutated, + }); await this.deps.showError(getPythonSetupErrorMessage(result)); return; } @@ -232,10 +301,20 @@ export class PythonSetupEnvironmentSetup implements Disposable { // project's interpreter setting at this run's venv. await this.deps.adoptInterpreter(result.venvPath, cwd); } catch (e) { + // The CLI succeeded, so there is no CLI error to report — but the + // flow failed. `adopt` is the extension's own phase, appended to the + // CLI's six so the funnel shows breaks that happen after it exits. + reportResult({ + outcome: "failed", + failurePhase: "adopt", + envKey: result.compute.envKey, + }); await this.deps.showError((e as Error).message); return; } + reportResult({outcome: "ok", envKey: result.compute.envKey}); + this.deps.saveState({ envKey: result.compute.envKey, pythonVersion: result.resolved.pythonVersion, @@ -249,6 +328,56 @@ export class PythonSetupEnvironmentSetup implements Disposable { await this.deps.showSuccess(result); } + /** + * Emit the attempt event for a run that is about to start and return its + * outcome reporter. + * + * Measurement must never break the flow it measures, so everything here is + * best-effort: a failure gathering the attempt's context degrades to + * `unknown`/omitted, and a failure in the emit itself is swallowed — the + * returned reporter then becomes a no-op rather than throwing mid-run. + */ + private async recordAttempt( + invocation: SetupLocalInvocation, + projectRoot: string + ): Promise { + const {compute} = invocation; + let packageManager: PrimaryManager = "unknown"; + let isGreenfield: boolean | undefined; + try { + packageManager = (await this.deps.getPackageManager()) ?? "unknown"; + isGreenfield = await greenfieldSignal( + packageManager, + projectRoot, + this.deps.hasPyprojectToml + ); + } catch { + // Keep the defaults: an attempt with a coarser package-manager + // value is still worth recording, and a probe failure must not cost + // the user their setup run. + } + try { + const reportResult = this.deps.recordSetupAttempt({ + packageManager, + targetType: compute.kind, + serverlessVersion: + compute.kind === "serverless" ? compute.version : undefined, + mode: invocation.mode, + isGreenfield, + }); + return (report) => { + try { + reportResult(report); + } catch { + // Swallow: the run's outcome has already been decided and + // surfaced to the user by the time this is called. + } + }; + } catch { + return () => {}; + } + } + dispose(): void { this.stateEmitter.dispose(); } diff --git a/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.test.ts b/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.test.ts index 541774b8f..cdaee99b7 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.test.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.test.ts @@ -7,6 +7,7 @@ import { resolveComputeFrom, } from "./pythonSetupDeps"; import {PythonSetupState} from "../../vscode-objs/StateStorage"; +import {Telemetry} from "../../telemetry"; describe("makePythonSetupVisibility", () => { const uvDetection = {primary: "uv" as const, managers: ["uv" as const]}; @@ -132,6 +133,9 @@ describe("makePythonSetupDeps saveState", () => { setActiveInterpreter: async () => {}, persistSetupState: () => {}, log: {append: () => {}, show: () => {}}, + // A reporter-less client: recordEvent short-circuits, so the setup + // events are inert here (they have their own tests). + telemetry: new Telemetry(undefined), ...overrides, }; } @@ -228,6 +232,9 @@ describe("makePythonSetupDeps withProgress", () => { setActiveInterpreter: async () => {}, persistSetupState: () => {}, log: {append: () => {}, show: () => {}}, + // A reporter-less client: recordEvent short-circuits, so the setup + // events are inert here (they have their own tests). + telemetry: new Telemetry(undefined), ...overrides, }; } diff --git a/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts b/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts index f5531ff49..cc7a63b11 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts @@ -1,5 +1,9 @@ +import {existsSync} from "fs"; +import path from "path"; import {ProgressLocation, Uri, window} from "vscode"; import {PackageManagerDetection} from "../../language/packageManagerDetection"; +import {Telemetry} from "../../telemetry"; +import "../../telemetry/pythonSetupExtensions"; import {PythonSetupState} from "../../vscode-objs/StateStorage"; import {shouldShowPythonSetup} from "../utils/pythonSetupGate"; import {venvInterpreterPath} from "../utils/venvInterpreterPath"; @@ -104,6 +108,8 @@ export interface PythonSetupWiringDeps { append: (chunk: string) => void; show: () => void; }; + /** Records the setup attempt/result events. */ + telemetry: Telemetry; } /** @@ -153,6 +159,20 @@ export function makePythonSetupDeps( "Python environment is set up for Databricks Connect." ); }, + recordSetupAttempt: (attempt) => + wiring.telemetry.recordPythonSetupAttempt(attempt), + getPackageManager: async () => { + const root = wiring.projectRoot(); + if (root === undefined) { + return undefined; + } + // Same detection the gate ran for this click. It is re-run rather + // than cached because a project's markers can change between the + // config view rendering the entry and the user pressing it. + return (await wiring.detect(root)).primary; + }, + hasPyprojectToml: async (projectRoot: string) => + existsSync(path.join(projectRoot, "pyproject.toml")), withProgress: (title, task) => // window.withProgress returns a Thenable; the seam is typed as a // Promise, so normalise it. `cancellable` is required for the token diff --git a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md new file mode 100644 index 000000000..420798679 --- /dev/null +++ b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md @@ -0,0 +1,159 @@ +# Telemetry: Python environment setup attempt / result + +Instrumentation for the uv-native "Set up Python environment" flow (VPEX): one +event when a setup run starts, one when it finishes. Together they measure the +funnel, where failures land across the CLI's phases, and how long provisioning +actually takes. + +These events do **not** change any setup behaviour. + +## Events + +| | | +| ---------------- | ---------------------------------------------------------------------------- | +| **Event names** | `python_env.setup.attempt`, `python_env.setup.result` | +| **Defined in** | `src/telemetry/constants.ts` (`Events.PYTHON_ENV_SETUP_ATTEMPT` / `_RESULT`) | +| **Emitted from** | `src/telemetry/pythonSetupExtensions.ts` (`recordPythonSetupAttempt`) | +| **Called from** | `src/python-setup/controllers/PythonSetupEnvironmentSetup.ts` (`runSetup`) | + +Transport is the shared `Telemetry` client, so properties are prefixed with +`event.`, `telemetry.telemetryLevel` opt-out is honoured, and the ambient +user/workspace envelope is attached automatically. + +`recordPythonSetupAttempt` emits the attempt and **returns the reporter for that +run's result**. The pairing is therefore structural: an outcome cannot be +reported without an attempt having been recorded, and each attempt has exactly +one result. + +## When they fire + +The attempt is recorded once the compute target is resolved and immediately +before the CLI is spawned — that is, once a run is genuinely about to happen. +Clicks that stop earlier (no project open, the visibility gate closed, or no +compute attached) record **nothing**; those are already visible as +`python_env.setup.detected` with trigger `explicit_command`. + +That gives a three-stage funnel: + +``` +python_env.setup.detected (explicit_command) user clicked + └─ python_env.setup.attempt a run started + └─ python_env.setup.result how it ended +``` + +Overlapping clicks coalesce onto the in-flight run (the orchestrator's +re-entrancy guard), so they produce one attempt, not two. + +## `python_env.setup.attempt` schema + +| Field | Type | Notes | +| ------------------- | ---------- | ----------------------------------------------------------------------------------- | +| `packageManager` | enum | `uv \| poetry \| pip \| conda \| unknown`. Priority `uv > poetry > conda > pip`. | +| `targetType` | enum | `cluster \| serverless`. **No** cluster IDs or names. | +| `serverlessVersion` | `string?` | The chosen serverless environment version (e.g. `"5"`). Omitted for clusters. | +| `mode` | enum | `default` (includes `databricks-connect`) \| `constraints-only`. | +| `isGreenfield` | `boolean?` | Project has no `pyproject.toml`. Omitted unless `packageManager` is `uv`/`unknown`. | + +### Why `isGreenfield` is conditional + +A missing `pyproject.toml` only means "greenfield" for a project with no +competing manager — pip and conda users may never have one, so for them the +absence says nothing and would inflate the greenfield rate. The field is emitted +only for `uv`/`unknown` projects, which is exactly the population the visibility +gate admits (`shouldShowPythonSetup` rejects anything with a pip/poetry/conda +signal). For other managers the probe is not even performed. + +### Why there is no `envKey` here + +A cluster's env key is `dbr/`, derived inside the CLI from a spark +version the extension never reads. Recomputing it locally would be a second +source of truth that can drift from the CLI. The authoritative key rides the +result event instead; the two join on session. + +## `python_env.setup.result` schema + +| Field | Type | Notes | +| -------------- | ---------- | --------------------------------------------------------------- | +| `outcome` | enum | `ok \| failed \| cancelled \| not_started`. | +| `failurePhase` | enum? | `preflight\|resolve\|fetch\|merge\|provision\|validate\|adopt`. | +| `errorCode` | enum? | The CLI's stable `E_*` failure class. | +| `envKey` | `string?` | e.g. `dbr/15.4.x-scala2.12`, `serverless/serverless-v5`. | +| `diskMutated` | `boolean?` | Whether a failed run had already modified project files. | +| `duration` | number | Milliseconds, measured by the extension (see below). | + +### Outcome values + +| Value | Meaning | +| ------------- | ------------------------------------------------------------------- | +| `ok` | CLI succeeded, venv provisioned **and** adopted as the interpreter. | +| `failed` | CLI returned `ok:false`, **or** adoption failed after a good run. | +| `cancelled` | The user cancelled the progress notification. | +| `not_started` | Spawn/parse error — the CLI produced no result object at all. | + +`cancelled` is kept distinct from `failed` on purpose: a user abandoning a slow +setup is a signal about provisioning time, not about breakage. `not_started` is +distinct because there is no phase or error code to attribute the break to. + +### The `adopt` phase + +`adopt` is a **synthetic seventh phase**, appended to the CLI's canonical six. It +covers pointing the MS Python extension at the provisioned venv — an +extension-side step that happens _after_ the CLI exits, so the CLI's own `phases` +array cannot describe it. A venv the editor never selects is unusable, so this +counts as a setup failure. + +`envKey` is a runtime coordinate from a closed vocabulary, **never** a cluster id +or a user-chosen cluster name. + +## Two ERD fields deliberately not reported + +The design doc for this work asked for a duration and a merge-conflict warning +count. Neither can be taken from the CLI result as-is: + +1. **Duration is measured in the extension, not read from `result.durationMs`.** + The CLI documents that field as reserved and always emits `0` + (`libs/localenv/result.go`: _"the pipeline does not measure wall time … so it + is always emitted as 0"_). The extension clock starts when the attempt is + recorded, which is also the better measurement: it is the latency the user + experiences, including process spawn and interpreter adoption. + +2. **The merge-conflict warning count is not emitted at all.** Nothing in the CLI + ever appends to `Result.Warnings` — `NewResult()` seeds it to `[]` and only + the text renderer reads it — and merge conflicts are not a modelled concept + there. The field would be a permanent `0`, and a dashboard built on it would + read "merge quality is perfect" when the truth is "unmeasured". It will be + added once the CLI has a producer for it. + +Also note `mode` is currently always `default`: the orchestrator hardcodes it +until the Quick-setup / `--constraints-only` picker ships. The field is in the +schema from the start so no migration is needed then. + +## Privacy + +Only categorical/enum values and a duration. No file paths, cluster names or +IDs, package names, project names, or user content. Optional fields are **omitted +when unknown** rather than sent as `undefined` — the transport would stringify +that to the literal `"undefined"` and pollute the schema. + +Telemetry never costs the user their setup run: gathering the attempt's context +is wrapped so a detection/probe failure degrades to `unknown` (and an omitted +`isGreenfield`) instead of propagating into the flow. + +Because every input is already in the orchestrator's local scope, an opted-out +user incurs no extra work — unlike package-manager detection, there are no +speculative disk reads to guard. + +Like every event from this extension, these inherit the ambient user/workspace +envelope (`user.hashedUserName`, `user.host`, `workspaceId`, `authType`), so the +outcome is linked to a stable hashed identity. + +## Suggested analysis + +- Funnel: `detected(explicit_command)` → `attempt` → `result(outcome=ok)`. +- Failure distribution over `failurePhase` × `errorCode` — where the funnel + breaks, without funnel tracking. +- `duration` percentiles for `outcome=ok`, to test the ~3 min setup claim; and + the `cancelled` rate against that distribution. +- Greenfield vs existing-project success rates (`isGreenfield` on the attempt, + joined to the result by session). +- `diskMutated` on failures — how often a failed run leaves the project modified. diff --git a/packages/databricks-vscode/src/telemetry/constants.ts b/packages/databricks-vscode/src/telemetry/constants.ts index 9ed8b89f4..9cf87edeb 100644 --- a/packages/databricks-vscode/src/telemetry/constants.ts +++ b/packages/databricks-vscode/src/telemetry/constants.ts @@ -25,6 +25,8 @@ export enum Events { DBCONNECT_RUN = "dbconnectRun", OPEN_RESOURCE_EXTERNALLY = "openResourceExternally", PYTHON_ENV_SETUP_DETECTED = "python_env.setup.detected", + PYTHON_ENV_SETUP_ATTEMPT = "python_env.setup.attempt", + PYTHON_ENV_SETUP_RESULT = "python_env.setup.result", } /* eslint-enable @typescript-eslint/naming-convention */ @@ -60,6 +62,37 @@ export type TargetCompute = ComputeType | "none"; /** What triggered a package-manager detection emission. */ export type SetupTrigger = "auto_open" | "explicit_command" | "run" | "debug"; +// The uv-native ("VPEX") python-setup flow mirrors the CLI's `environments +// setup-local --output json` contract, so the setup event unions are owned by +// the result model (the TypeScript view of that contract) and re-exported here. +// Type-only, so the event schema can never drift from the wire shape it +// describes. +import type { + PythonSetupMode, + PythonSetupPhaseName, + PythonSetupErrorCode, +} from "../python-setup/models/PythonSetupResult"; +export type {PythonSetupMode, PythonSetupErrorCode}; + +/** + * How a setup run ended. + * + * `not_started` is distinct from `failed`: the CLI never produced a result + * (spawn/parse error), so no phase or error code exists to attribute. And + * `cancelled` is distinct from both — a user abandoning a slow setup is the + * signal that the provisioning time is unacceptable, not that it broke. + */ +export type PythonSetupOutcome = "ok" | "failed" | "cancelled" | "not_started"; + +/** + * Where a failed setup broke. The CLI's six canonical phases plus `adopt`, the + * extension-side step that points the MS Python extension at the provisioned + * venv. Adoption is the point of the flow (an unselected venv is unusable from + * the editor), so its failure is a setup failure — but it happens after the CLI + * has exited ok, so the CLI's own `phases` array cannot describe it. + */ +export type PythonSetupFailurePhase = PythonSetupPhaseName | "adopt"; + /** Documentation about all of the properties and metrics of the event. */ type EventDescription = {[K in keyof T]?: {comment?: string}}; @@ -270,6 +303,83 @@ export class EventTypes { comment: "Which setup touchpoint triggered detection", }, }; + [Events.PYTHON_ENV_SETUP_ATTEMPT]: EventType<{ + packageManager: PrimaryManager; + targetType: ComputeType; + serverlessVersion?: string; + mode: PythonSetupMode; + isGreenfield?: boolean; + }> = { + comment: + "A uv-native Python environment setup run is starting: emitted once the compute " + + "target is known and immediately before the CLI is spawned, so every attempt has " + + "exactly one matching python_env.setup.result. Categorical data only — no cluster " + + "IDs/names, paths, or package names.", + packageManager: { + comment: + "The package manager detected for the project (uv > poetry > conda > pip), or unknown", + }, + targetType: { + comment: "Whether the environment targets a cluster or serverless", + }, + serverlessVersion: { + comment: + 'The chosen serverless environment version (e.g. "5"); omitted for clusters', + }, + mode: { + comment: + "Whether databricks-connect is included (default) or only the runtime constraints (constraints-only)", + }, + isGreenfield: { + comment: + "Whether the project has no pyproject.toml yet. Omitted unless packageManager " + + "is uv or unknown: for a pip/conda project the absence of a pyproject.toml says " + + "nothing about greenfield-ness, so the signal would be misleading", + }, + }; + [Events.PYTHON_ENV_SETUP_RESULT]: EventType< + { + outcome: PythonSetupOutcome; + failurePhase?: PythonSetupFailurePhase; + errorCode?: PythonSetupErrorCode; + envKey?: string; + diskMutated?: boolean; + } & DurationMeasurement + > = { + comment: + "The outcome of a uv-native Python environment setup run. Pairs 1:1 with a preceding " + + "python_env.setup.attempt. The failure phase localises where the funnel breaks without " + + "requiring funnel tracking. Categorical data only.", + outcome: { + comment: + "ok | failed | cancelled (user aborted) | not_started (the CLI produced no result)", + }, + failurePhase: { + comment: + "Which phase broke: the CLI's preflight/resolve/fetch/merge/provision/validate, " + + 'or "adopt" when the venv was provisioned but could not be selected as the ' + + "interpreter. Omitted unless the outcome is failed", + }, + errorCode: { + comment: + "The CLI's stable failure-class code (E_*). Omitted when the CLI reported no error object", + }, + envKey: { + comment: + 'The resolved environment key (e.g. "dbr/15.4.x-scala2.12", ' + + '"serverless/serverless-v5") — a runtime coordinate, never a cluster ID or name. ' + + "Omitted when the run failed before resolving one", + }, + diskMutated: { + comment: + "Whether the failed run had already modified project files. Omitted when the CLI reported no error object", + }, + // Measured by the extension around the whole run, not read from the + // CLI's own durationMs (documented as reserved and always 0). This is + // also the latency the user actually experiences: it includes process + // spawn and interpreter adoption. + ...getDurationProperty(), + }; } /** diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts new file mode 100644 index 000000000..052a1cd0d --- /dev/null +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts @@ -0,0 +1,167 @@ +// The recorded event keys are transport-prefixed ("event."), not +// identifiers, so the camelCase rule does not apply to these assertions. +/* eslint-disable @typescript-eslint/naming-convention */ +import {expect} from "chai"; +import {Telemetry} from "."; +import "./pythonSetupExtensions"; + +type RecordedEvent = { + name: string; + props: Record; + metrics: Record; +}; + +/** A Telemetry backed by a fake reporter that captures sent events. */ +function makeTelemetry(level: "all" | "error" | "crash" | "off" = "all") { + const events: RecordedEvent[] = []; + const reporter = { + telemetryLevel: level, + sendTelemetryEvent: ( + name: string, + props?: Record, + metrics?: Record + ) => { + events.push({name, props: props ?? {}, metrics: metrics ?? {}}); + }, + sendTelemetryErrorEvent: () => {}, + sendDangerousTelemetryEvent: () => {}, + sendDangerousTelemetryErrorEvent: () => {}, + dispose: () => Promise.resolve(), + }; + return {telemetry: new Telemetry(reporter as any), events}; +} + +describe(__filename, () => { + it("records an attempt, then its result via the returned reporter", () => { + const {telemetry, events} = makeTelemetry(); + + const reportResult = telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "serverless", + serverlessVersion: "5", + mode: "default", + isGreenfield: true, + }); + reportResult({ + outcome: "ok", + envKey: "serverless/serverless-v5", + }); + + expect(events.map((e) => e.name)).to.deep.equal([ + "python_env.setup.attempt", + "python_env.setup.result", + ]); + expect(events[0].props).to.deep.equal({ + "version": "1.0", + "event.packageManager": "uv", + "event.targetType": "serverless", + "event.serverlessVersion": "5", + "event.mode": "default", + "event.isGreenfield": "true", + }); + expect(events[1].props).to.deep.equal({ + "version": "1.0", + "event.outcome": "ok", + "event.envKey": "serverless/serverless-v5", + }); + }); + + it("stamps a duration on the result, measured from the attempt", () => { + const {telemetry, events} = makeTelemetry(); + + const reportResult = telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + }); + reportResult({outcome: "ok"}); + + // The CLI's own durationMs is always 0; this must be our own clock. + expect(events[1].metrics).to.have.property("event.duration"); + expect(events[1].metrics["event.duration"]).to.be.a("number"); + expect(events[1].metrics["event.duration"]).to.be.at.least(0); + // Duration is a metric, never a property. + expect(events[1].props).to.not.have.property("event.duration"); + }); + + it("omits absent optional fields instead of sending the string 'undefined'", () => { + const {telemetry, events} = makeTelemetry(); + + // A cluster attempt: no serverless version, and a manager for which the + // greenfield signal is not reportable. + const reportResult = telemetry.recordPythonSetupAttempt({ + packageManager: "pip", + targetType: "cluster", + mode: "constraints-only", + serverlessVersion: undefined, + isGreenfield: undefined, + }); + // A cancelled run has no phase, error code, env key or disk state. + reportResult({ + outcome: "cancelled", + failurePhase: undefined, + errorCode: undefined, + envKey: undefined, + diskMutated: undefined, + }); + + expect(events[0].props).to.deep.equal({ + "version": "1.0", + "event.packageManager": "pip", + "event.targetType": "cluster", + "event.mode": "constraints-only", + }); + expect(events[1].props).to.deep.equal({ + "version": "1.0", + "event.outcome": "cancelled", + }); + // The failure mode this guards against: recordEvent stringifies an + // explicit undefined, which would pollute the schema. + for (const event of events) { + expect(Object.values(event.props)).to.not.contain("undefined"); + } + }); + + it("records a full failure report", () => { + const {telemetry, events} = makeTelemetry(); + + const reportResult = telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + isGreenfield: false, + }); + reportResult({ + outcome: "failed", + failurePhase: "provision", + errorCode: "E_PROVISION", + envKey: "dbr/15.4.x-scala2.12", + diskMutated: true, + }); + + expect(events[1].props).to.deep.equal({ + "version": "1.0", + "event.outcome": "failed", + "event.failurePhase": "provision", + "event.errorCode": "E_PROVISION", + "event.envKey": "dbr/15.4.x-scala2.12", + "event.diskMutated": "true", + }); + }); + + it("sends nothing when the telemetry reporter is unavailable", () => { + // No reporter: recordEvent short-circuits, so neither event is built. + // (Level-based opt-out is enforced inside the real reporter and covered + // by the client's own tests.) + const telemetry = new Telemetry(undefined); + + const reportResult = telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + }); + + expect(() => reportResult({outcome: "ok"})).to.not.throw(); + expect(telemetry.isTelemetryEnabled).to.equal(false); + }); +}); diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts new file mode 100644 index 000000000..a7bf5370d --- /dev/null +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts @@ -0,0 +1,101 @@ +import {Events, Telemetry} from "."; +import { + ComputeType, + PrimaryManager, + PythonSetupErrorCode, + PythonSetupFailurePhase, + PythonSetupMode, + PythonSetupOutcome, +} from "./constants"; + +/** + * What a starting setup run is about to do. Everything here is known before the + * CLI is spawned: the project's package manager (from the visibility gate's + * detection), the compute target, and the provisioning mode. + */ +export interface PythonSetupAttempt { + packageManager: PrimaryManager; + targetType: ComputeType; + /** The chosen serverless environment version; absent for clusters. */ + serverlessVersion?: string; + mode: PythonSetupMode; + /** + * Whether the project has no `pyproject.toml` yet, or `undefined` when the + * signal would be misleading — for a pip/conda project the absence of a + * `pyproject.toml` says nothing about greenfield-ness. + */ + isGreenfield?: boolean; +} + +/** How a setup run ended, reduced to the categorical fields we report. */ +export interface PythonSetupOutcomeReport { + outcome: PythonSetupOutcome; + failurePhase?: PythonSetupFailurePhase; + errorCode?: PythonSetupErrorCode; + envKey?: string; + diskMutated?: boolean; +} + +/** Reports the outcome of the run whose attempt returned it. */ +export type PythonSetupResultReporter = ( + report: PythonSetupOutcomeReport +) => void; + +/** + * Drop keys whose value is `undefined`. + * + * `recordEvent` stringifies an explicit `undefined` to the literal "undefined", + * so passing an absent optional through would pollute the event schema with a + * bogus value. Callers build reports straight from optional chaining + * (`result.error?.code`), so the filtering belongs here rather than at every + * call site. + */ +function withoutUndefined(source: T): Partial { + return Object.fromEntries( + Object.entries(source).filter(([, v]) => v !== undefined) + ) as Partial; +} + +declare module "." { + interface Telemetry { + /** + * Record the start of a uv-native Python environment setup run, and + * return the reporter for its outcome. + * + * Emits PYTHON_ENV_SETUP_ATTEMPT immediately and returns a reporter for + * PYTHON_ENV_SETUP_RESULT whose `duration` is measured from this call — + * so the reported time covers the whole run as the user experiences it + * (CLI spawn, provisioning, and interpreter adoption), not just the + * CLI's internal pipeline. The CLI's own `durationMs` is deliberately + * not used: it is documented as reserved and always 0. + * + * Returning the reporter (rather than exposing two independent record + * methods) is what makes the attempt/result pairing structural: an + * outcome cannot be reported without an attempt having been recorded. + */ + recordPythonSetupAttempt( + attempt: PythonSetupAttempt + ): PythonSetupResultReporter; + } +} + +Telemetry.prototype.recordPythonSetupAttempt = function ( + attempt: PythonSetupAttempt +): PythonSetupResultReporter { + this.recordEvent(Events.PYTHON_ENV_SETUP_ATTEMPT, { + ...withoutUndefined(attempt), + // Re-assert the required fields: withoutUndefined widens everything to + // optional, and the event schema requires these three. + packageManager: attempt.packageManager, + targetType: attempt.targetType, + mode: attempt.mode, + }); + + // start() stamps the elapsed time onto the result event as `duration`. + const reportResult = this.start(Events.PYTHON_ENV_SETUP_RESULT); + return (report: PythonSetupOutcomeReport) => + reportResult({ + ...withoutUndefined(report), + outcome: report.outcome, + }); +}; From bef83830c3b58ca5e5b8948f16acfb0030ab8249 Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Tue, 4 Aug 2026 10:38:26 +0200 Subject: [PATCH 2/5] fix(python-setup): harden setup telemetry against review findings *Why* Codex review of #2074 found three ways the events could misreport. All are latent on current paths, but each is cheap to make structurally impossible rather than relying on a convention holding. *What* - Never record `ok` for a run that then rejects. The success report moved after saveState/readiness, with a new `persist` failure phase for a throw there. It stays *before* showSuccess deliberately: that awaits the user dismissing a toast, so reporting after it would fold think-time into `duration` and wreck the setup-time metric. - Constrain `envKey` to the CLI's two documented shapes (serverless/serverless-v, dbr/) before emission; anything else collapses to "other". The key comes from JSON the parser validates only minimally, and the DBR arm is a raw "dbr/" + sparkVersion concatenation -- so drift could otherwise put unbounded, potentially identifying, high-cardinality content into a field documented as categorical. - Make the result reporter once-only, so the documented 1:1 attempt/result pairing survives a future refactor that adds a terminal path. *Verification* - yarn build clean; test:lint 0 errors, prettier clean - unit tests: 466 passing, +5 (persist-phase reporting, report-before-toast ordering, once-only reporter, env-key passthrough for valid shapes, env-key collapse for 5 invalid/identifying ones). Same 6 pre-existing cli/CliWrapper.test.ts failures as the base commit. - PR CI on the parent commit: Linux + Windows unit tests and VSIX packaging all passed. Co-authored-by: Isaac --- .../PythonSetupEnvironmentSetup.test.ts | 50 ++++++++++++++++ .../PythonSetupEnvironmentSetup.ts | 34 +++++++---- .../src/telemetry/PYTHON_SETUP_TELEMETRY.md | 55 ++++++++++++------ .../src/telemetry/constants.ts | 25 +++++--- .../telemetry/pythonSetupExtensions.test.ts | 58 +++++++++++++++++++ .../src/telemetry/pythonSetupExtensions.ts | 50 +++++++++++++++- 6 files changed, 235 insertions(+), 37 deletions(-) diff --git a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts index 695b6442a..1b6ba5ba1 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts @@ -740,6 +740,56 @@ describe("PythonSetupEnvironmentSetup telemetry", () => { expect(telemetry.results).to.have.length(2); }); + it("does not report ok when the post-adoption state bookkeeping throws", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + saveState: () => { + throw new Error("workspaceState write failed"); + }, + }) + ); + + let rejected = false; + try { + await setup.setup(); + } catch { + rejected = true; + } + + // The run rejected, so recording success would permanently overstate the + // success rate. + expect(rejected).to.equal(true); + expect(telemetry.results).to.deep.equal([ + { + outcome: "failed", + failurePhase: "persist", + envKey: SUCCESS_REAL_RUN.compute!.envKey, + }, + ]); + }); + + it("reports ok before showSuccess, so user think-time is not in the duration", async () => { + const telemetry = makeTelemetryRecorder(); + let reportedBeforeToast = false; + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + // showSuccess wraps window.showInformationMessage, whose promise + // settles only once the user dismisses the toast. + showSuccess: async () => { + reportedBeforeToast = telemetry.results.length === 1; + }, + }) + ); + + await setup.setup(); + + expect(reportedBeforeToast).to.equal(true); + expect(telemetry.results[0].outcome).to.equal("ok"); + }); + it("completes the setup even when the telemetry emit itself throws", async () => { const setup = new PythonSetupEnvironmentSetup( makeDeps({ diff --git a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts index df040ea90..65c0c9864 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts @@ -313,18 +313,32 @@ export class PythonSetupEnvironmentSetup implements Disposable { return; } - reportResult({outcome: "ok", envKey: result.compute.envKey}); + // Do the state bookkeeping *before* reporting success, so a throw here + // is never recorded as `ok`. + try { + this.deps.saveState({ + envKey: result.compute.envKey, + pythonVersion: result.resolved.pythonVersion, + }); + // Record readiness for the project this run provisioned (the + // captured cwd), not the live active project — a mid-run switch must + // not mark a different project ready. + this.readyRoots.add(cwd); + this.stateEmitter.fire(); + } catch (e) { + reportResult({ + outcome: "failed", + failurePhase: "persist", + envKey: result.compute.envKey, + }); + throw e; + } - this.deps.saveState({ - envKey: result.compute.envKey, - pythonVersion: result.resolved.pythonVersion, - }); + // Reported before `showSuccess` on purpose: that awaits the user + // dismissing a toast, and folding think-time into `duration` would wreck + // the setup-time metric this event exists to measure. + reportResult({outcome: "ok", envKey: result.compute.envKey}); - // Record readiness for the project this run provisioned (the captured - // cwd), not the live active project — a mid-run switch must not mark a - // different project ready. - this.readyRoots.add(cwd); - this.stateEmitter.fire(); await this.deps.showSuccess(result); } diff --git a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md index 420798679..e74ee185b 100644 --- a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md +++ b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md @@ -21,9 +21,10 @@ Transport is the shared `Telemetry` client, so properties are prefixed with user/workspace envelope is attached automatically. `recordPythonSetupAttempt` emits the attempt and **returns the reporter for that -run's result**. The pairing is therefore structural: an outcome cannot be -reported without an attempt having been recorded, and each attempt has exactly -one result. +run's result**. The pairing is therefore structural rather than a convention: an +outcome cannot be reported without an attempt having been recorded, and the +reporter is once-only — a second call is dropped, so one attempt can never +inflate into several results. ## When they fire @@ -72,14 +73,14 @@ result event instead; the two join on session. ## `python_env.setup.result` schema -| Field | Type | Notes | -| -------------- | ---------- | --------------------------------------------------------------- | -| `outcome` | enum | `ok \| failed \| cancelled \| not_started`. | -| `failurePhase` | enum? | `preflight\|resolve\|fetch\|merge\|provision\|validate\|adopt`. | -| `errorCode` | enum? | The CLI's stable `E_*` failure class. | -| `envKey` | `string?` | e.g. `dbr/15.4.x-scala2.12`, `serverless/serverless-v5`. | -| `diskMutated` | `boolean?` | Whether a failed run had already modified project files. | -| `duration` | number | Milliseconds, measured by the extension (see below). | +| Field | Type | Notes | +| -------------- | ---------- | ----------------------------------------------------------- | +| `outcome` | enum | `ok \| failed \| cancelled \| not_started`. | +| `failurePhase` | enum? | The CLI's six phases, plus `adopt` / `persist` (see below). | +| `errorCode` | enum? | The CLI's stable `E_*` failure class. | +| `envKey` | `string?` | e.g. `dbr/15.4.x-scala2.12`, `serverless/serverless-v5`. | +| `diskMutated` | `boolean?` | Whether a failed run had already modified project files. | +| `duration` | number | Milliseconds, measured by the extension (see below). | ### Outcome values @@ -94,16 +95,34 @@ result event instead; the two join on session. setup is a signal about provisioning time, not about breakage. `not_started` is distinct because there is no phase or error code to attribute the break to. -### The `adopt` phase +### The extension-side phases -`adopt` is a **synthetic seventh phase**, appended to the CLI's canonical six. It -covers pointing the MS Python extension at the provisioned venv — an -extension-side step that happens _after_ the CLI exits, so the CLI's own `phases` -array cannot describe it. A venv the editor never selects is unusable, so this -counts as a setup failure. +Two phases are appended to the CLI's canonical six. Both cover steps that run +_after_ the CLI has already exited ok, so its own `phases` array cannot describe +them: + +- **`adopt`** — pointing the MS Python extension at the provisioned venv. A venv + the editor never selects is unusable, so this counts as a setup failure. +- **`persist`** — recording the drift-detection baseline and readiness. The + environment itself works, but the extension's own state did not stick. + +The success report is emitted only after both have completed, so a throw in +either is never recorded as `ok`. It is emitted _before_ the success toast, +though: `showSuccess` resolves only when the user dismisses the notification, and +folding think-time into `duration` would wreck the metric. + +### `envKey` is constrained before emission `envKey` is a runtime coordinate from a closed vocabulary, **never** a cluster id -or a user-chosen cluster name. +or a user-chosen cluster name. It is validated against the CLI's two documented +shapes (`serverless/serverless-v` and `dbr/`) before being +emitted; anything else collapses to `"other"`. + +This matters because the key is copied out of CLI JSON that the parser +deliberately validates only minimally, and the DBR arm is a raw +`"dbr/" + sparkVersion` concatenation. Without the check, schema drift or an +unexpected runtime string could put unbounded, potentially identifying, +high-cardinality content into a field documented as categorical. ## Two ERD fields deliberately not reported diff --git a/packages/databricks-vscode/src/telemetry/constants.ts b/packages/databricks-vscode/src/telemetry/constants.ts index 9cf87edeb..9e8821f77 100644 --- a/packages/databricks-vscode/src/telemetry/constants.ts +++ b/packages/databricks-vscode/src/telemetry/constants.ts @@ -85,13 +85,20 @@ export type {PythonSetupMode, PythonSetupErrorCode}; export type PythonSetupOutcome = "ok" | "failed" | "cancelled" | "not_started"; /** - * Where a failed setup broke. The CLI's six canonical phases plus `adopt`, the - * extension-side step that points the MS Python extension at the provisioned - * venv. Adoption is the point of the flow (an unselected venv is unusable from - * the editor), so its failure is a setup failure — but it happens after the CLI - * has exited ok, so the CLI's own `phases` array cannot describe it. + * Where a failed setup broke: the CLI's six canonical phases, plus the two + * extension-side steps that run after the CLI has already exited ok (so its own + * `phases` array cannot describe them). + * + * - `adopt` — pointing the MS Python extension at the provisioned venv. + * Adoption is the point of the flow: an unselected venv is unusable from the + * editor, so failing here is a setup failure. + * - `persist` — recording the drift-detection baseline and readiness. The + * environment works, but the extension's own state did not stick. */ -export type PythonSetupFailurePhase = PythonSetupPhaseName | "adopt"; +export type PythonSetupFailurePhase = + | PythonSetupPhaseName + | "adopt" + | "persist"; /** Documentation about all of the properties and metrics of the event. */ type EventDescription = {[K in keyof T]?: {comment?: string}}; @@ -357,8 +364,9 @@ export class EventTypes { failurePhase: { comment: "Which phase broke: the CLI's preflight/resolve/fetch/merge/provision/validate, " + - 'or "adopt" when the venv was provisioned but could not be selected as the ' + - "interpreter. Omitted unless the outcome is failed", + 'or the extension-side "adopt" (venv provisioned but not selectable as the ' + + 'interpreter) / "persist" (state bookkeeping failed). Omitted unless the ' + + "outcome is failed", }, errorCode: { comment: @@ -368,6 +376,7 @@ export class EventTypes { comment: 'The resolved environment key (e.g. "dbr/15.4.x-scala2.12", ' + '"serverless/serverless-v5") — a runtime coordinate, never a cluster ID or name. ' + + 'Constrained to those two shapes before emission; anything else becomes "other". ' + "Omitted when the run failed before resolving one", }, diskMutated: { diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts index 052a1cd0d..a477e5244 100644 --- a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts @@ -149,6 +149,64 @@ describe(__filename, () => { }); }); + it("reports at most one result per attempt", () => { + const {telemetry, events} = makeTelemetry(); + + const reportResult = telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + }); + reportResult({outcome: "ok"}); + // A second call (e.g. from a future refactor that adds a terminal path + // without returning) must not inflate one attempt into two results. + reportResult({outcome: "failed", failurePhase: "persist"}); + + expect(events.map((e) => e.name)).to.deep.equal([ + "python_env.setup.attempt", + "python_env.setup.result", + ]); + expect(events[1].props["event.outcome"]).to.equal("ok"); + }); + + it("passes through the CLI's documented env-key shapes", () => { + for (const envKey of [ + "serverless/serverless-v5", + "serverless/serverless-v12", + "dbr/15.4.x-scala2.12", + "dbr/14.3.x-photon-scala2.12", + ]) { + const {telemetry, events} = makeTelemetry(); + telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + })({outcome: "ok", envKey}); + expect(events[1].props["event.envKey"]).to.equal(envKey); + } + }); + + it("collapses an unrecognised env key to a categorical placeholder", () => { + // The DBR arm of the key is a raw "dbr/" + sparkVersion concatenation + // from minimally-validated CLI JSON, so schema drift must not put + // unbounded (potentially identifying) content into the field. + for (const envKey of [ + "cluster-0710-142042-abcdefgh", + "dbr/../../etc/passwd", + "/Users/someone/projects/secret-project", + "serverless/serverless-vNEXT", + "", + ]) { + const {telemetry, events} = makeTelemetry(); + telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + })({outcome: "ok", envKey}); + expect(events[1].props["event.envKey"]).to.equal("other"); + } + }); + it("sends nothing when the telemetry reporter is unavailable", () => { // No reporter: recordEvent short-circuits, so neither event is built. // (Level-based opt-out is enforced inside the real reporter and covered diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts index a7bf5370d..7c927a9cf 100644 --- a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts @@ -56,6 +56,42 @@ function withoutUndefined(source: T): Partial { ) as Partial; } +/** + * The env-key shapes the CLI produces: `serverless/serverless-v` and + * `dbr/` (see `EnvKeyForServerless` / `EnvKeyForSparkVersion`). + * The DBR arm is deliberately narrow — a Spark version is dotted/dashed + * alphanumerics, nothing else. + */ +const ENV_KEY_PATTERNS = [ + /^serverless\/serverless-v\d+$/, + /^dbr\/[A-Za-z0-9][A-Za-z0-9.\-_]*$/, +]; + +/** + * Reported in place of an env key that does not match a known shape. + */ +const UNRECOGNISED_ENV_KEY = "other"; + +/** + * Constrain `envKey` to the CLI's documented shapes before it is emitted. + * + * The key is copied from CLI JSON that {@link parsePythonSetupResult} + * deliberately validates only minimally, and the DBR arm is a raw + * `"dbr/" + sparkVersion` concatenation. Without this, schema drift or an + * unexpected runtime string would put unbounded — potentially identifying — + * high-cardinality content into a field documented as a closed vocabulary. + * Anything unrecognised collapses to {@link UNRECOGNISED_ENV_KEY}, which keeps + * the dimension categorical while still flagging that drift happened. + */ +function categoricalEnvKey(envKey: string | undefined): string | undefined { + if (envKey === undefined) { + return undefined; + } + return ENV_KEY_PATTERNS.some((p) => p.test(envKey)) + ? envKey + : UNRECOGNISED_ENV_KEY; +} + declare module "." { interface Telemetry { /** @@ -93,9 +129,21 @@ Telemetry.prototype.recordPythonSetupAttempt = function ( // start() stamps the elapsed time onto the result event as `duration`. const reportResult = this.start(Events.PYTHON_ENV_SETUP_RESULT); - return (report: PythonSetupOutcomeReport) => + // Enforce the 1:1 pairing rather than only documenting it: a second call + // (from a future refactor that adds a terminal path without returning) is + // dropped, so one attempt can never inflate into several results. + let reported = false; + return (report: PythonSetupOutcomeReport) => { + if (reported) { + return; + } + reported = true; reportResult({ ...withoutUndefined(report), outcome: report.outcome, + ...(report.envKey !== undefined + ? {envKey: categoricalEnvKey(report.envKey)} + : {}), }); + }; }; From e423680720b70688d654abc0c250f23701bac075 Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Tue, 4 Aug 2026 11:20:03 +0200 Subject: [PATCH 3/5] fix(python-setup): enforce the telemetry allowlist, close the funnel gap *Why* A second review pass (Claude subagent) found two P1s in the previous commits. Both were demonstrated empirically, not just argued. *What* 1. The emit half no longer spreads the caller's object. Spreading a *variable* disables TypeScript's excess-property check, so any field later added to PythonSetupAttempt / PythonSetupOutcomeReport -- or any wider object passed through the seam -- was emitted automatically, with objects JSON-stringified by recordEvent. The reviewer proved it: adding `leakedCompute?: {clusterId: string}` compiled with exit 0 and put the cluster ID on the wire. There was no live leak (the orchestrator builds an explicit literal), but nothing made the privacy claim enforceable. Both payloads now name every field, so the schema is a compiler-checked allowlist. 2. The documented three-stage funnel did not exist. The design justified recording nothing for early-abort clicks on the grounds that python_env.setup.detected covers them -- but its `explicit_command` trigger fires only from the *legacy* databricks.environment.setup command, while this entry dispatches setupPythonEnv, and the config view renders the two mutually exclusively. So the VPEX cohort never emitted that event, and the no-compute dead end (a real user-facing one, reached via NO_COMPUTE_TARGET_MESSAGE) was invisible. Adds an `outcome: "no_compute"` result, emitted without an attempt, and corrects the docs to stop claiming coverage that does not hold. `duration` is now optional and omitted there: nothing ran, and a 0 would drag the setup-time percentiles down. Also from that pass: - Tighten the envKey DBR arm to the spark-version grammar with a length bound. The previous pattern admitted cluster *names*, which are user-chosen and often contain a person's name (`dbr/janes-dev-cluster` passed). Four such cases added to the tests. - Split the two attempt probes into independent try blocks, so a failing pyproject.toml probe no longer discards a successfully detected package manager (which biased the distribution toward `unknown`). *Verification* - yarn build clean; test:lint 0 errors, prettier clean - unit tests: 471 passing, +5. New: schema-allowlist (extra fields with a cluster id / path / email are not emitted, exact key sets asserted), no_compute emitted alone with no duration, no_compute resilience, manager preserved when only the greenfield probe fails, cluster-name env keys collapse to "other". - One existing test's assertion updated: with independent try blocks a detection failure now still runs the greenfield probe, so isGreenfield is false rather than undefined. New behaviour is the intended one. - Same 6 pre-existing cli/CliWrapper.test.ts failures as the base commit. - Isaac Review: clean pass, 0 findings (its earlier run aborted on context thrashing and was rerun). Co-authored-by: Isaac --- .../PythonSetupEnvironmentSetup.test.ts | 71 +++++++++++++++-- .../PythonSetupEnvironmentSetup.ts | 28 ++++++- .../controllers/pythonSetupDeps.ts | 1 + .../src/telemetry/PYTHON_SETUP_TELEMETRY.md | 55 +++++++++---- .../src/telemetry/constants.ts | 42 ++++++---- .../telemetry/pythonSetupExtensions.test.ts | 61 +++++++++++++++ .../src/telemetry/pythonSetupExtensions.ts | 77 +++++++++++++------ 7 files changed, 277 insertions(+), 58 deletions(-) diff --git a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts index 1b6ba5ba1..56f6b8855 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.test.ts @@ -36,6 +36,9 @@ function makeTelemetryRecorder() { results.push(report); }; }, + recordNoCompute: () => { + results.push({outcome: "no_compute"}); + }, }; } @@ -109,6 +112,7 @@ function makeDeps( // Telemetry defaults to a no-op sink; tests that assert on events pass // a recorder instead. recordSetupAttempt: () => () => {}, + recordNoCompute: () => {}, getPackageManager: async () => "uv", hasPyprojectToml: async () => true, ...overrides, @@ -708,11 +712,10 @@ describe("PythonSetupEnvironmentSetup telemetry", () => { expect(telemetry.results).to.deep.equal([{outcome: "not_started"}]); }); - it("records nothing when the run never starts", async () => { + it("records nothing when there is no project or the gate is closed", async () => { for (const overrides of [ {projectRoot: () => undefined}, {isVisible: async () => false}, - {resolveCompute: async () => undefined}, ]) { const telemetry = makeTelemetryRecorder(); const setup = new PythonSetupEnvironmentSetup( @@ -721,13 +724,47 @@ describe("PythonSetupEnvironmentSetup telemetry", () => { await setup.setup(); - // No CLI ran, so there is no attempt to pair a result with. These - // clicks are covered by python_env.setup.detected instead. + // Neither is a user-visible dead end: with no project there is + // nothing to set up, and a closed gate means the CTA was never shown. expect(telemetry.attempts).to.have.length(0); expect(telemetry.results).to.have.length(0); } }); + it("reports no_compute (without an attempt) when the CTA is a dead end", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({...telemetry, resolveCompute: async () => undefined}) + ); + + await setup.setup(); + + // The entry is visible whenever the project fits, independent of + // compute, so this is a real dead-end click worth measuring. No run + // started, hence no attempt to pair with. + expect(telemetry.attempts).to.have.length(0); + expect(telemetry.results).to.deep.equal([{outcome: "no_compute"}]); + }); + + it("still guides the user when the no_compute emit throws", async () => { + const notified: string[] = []; + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + resolveCompute: async () => undefined, + recordNoCompute: () => { + throw new Error("telemetry blew up"); + }, + notify: async (m) => { + notified.push(m); + }, + }) + ); + + await setup.setup(); + + expect(notified).to.have.length(1); + }); + it("records exactly one result per attempt, including across runs", async () => { const telemetry = makeTelemetryRecorder(); const setup = new PythonSetupEnvironmentSetup(makeDeps(telemetry)); @@ -819,6 +856,27 @@ describe("PythonSetupEnvironmentSetup telemetry", () => { expect(setup.ready).to.equal(true); }); + it("keeps the detected manager when only the pyproject probe fails", async () => { + const telemetry = makeTelemetryRecorder(); + const setup = new PythonSetupEnvironmentSetup( + makeDeps({ + ...telemetry, + getPackageManager: async () => "uv", + hasPyprojectToml: async () => { + throw new Error("stat failed"); + }, + }) + ); + + await setup.setup(); + + // The two probes are independent: a failing greenfield probe must not + // discard a successfully detected manager, or the manager distribution + // would skew toward `unknown`. + expect(telemetry.attempts[0].packageManager).to.equal("uv"); + expect(telemetry.attempts[0].isGreenfield).to.equal(undefined); + }); + it("still records the attempt when gathering its context fails", async () => { const telemetry = makeTelemetryRecorder(); const setup = new PythonSetupEnvironmentSetup( @@ -837,7 +895,10 @@ describe("PythonSetupEnvironmentSetup telemetry", () => { expect(setup.ready).to.equal(true); expect(telemetry.attempts).to.have.length(1); expect(telemetry.attempts[0].packageManager).to.equal("unknown"); - expect(telemetry.attempts[0].isGreenfield).to.equal(undefined); + // The greenfield probe is independent and still runs: `unknown` is one + // of the two managers for which the signal is meaningful, and this + // project has a pyproject.toml. + expect(telemetry.attempts[0].isGreenfield).to.equal(false); expect(telemetry.results).to.deep.equal([ {outcome: "ok", envKey: SUCCESS_REAL_RUN.compute!.envKey}, ]); diff --git a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts index 65c0c9864..8dbe61bfd 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/PythonSetupEnvironmentSetup.ts @@ -128,6 +128,16 @@ export interface PythonSetupSetupDeps { attempt: PythonSetupAttempt ) => PythonSetupResultReporter; + /** + * Record that the CTA was pressed with nothing to set up for, so no run + * started. Reported without an attempt (the one exception to the pairing), + * because a visible button that dead-ends is worth measuring and no other + * event covers it: `python_env.setup.detected`'s `explicit_command` trigger + * fires only from the legacy setup command, which the config view shows + * mutually exclusively with this entry. + */ + recordNoCompute: () => void; + /** * The project's detected package manager, for the attempt event. Reads the * same detection the visibility gate runs; `undefined` when detection was @@ -244,6 +254,11 @@ export class PythonSetupEnvironmentSetup implements Disposable { // Tell them what to do instead of silently no-op'ing the button. // Plain notify (not showError): no CLI ran, so there is no log to // reveal. + try { + this.deps.recordNoCompute(); + } catch { + // Measurement must never break the flow it measures. + } await this.deps.notify(NO_COMPUTE_TARGET_MESSAGE); return; } @@ -358,17 +373,24 @@ export class PythonSetupEnvironmentSetup implements Disposable { const {compute} = invocation; let packageManager: PrimaryManager = "unknown"; let isGreenfield: boolean | undefined; + // Two independent probes, so they get independent try blocks: a failing + // pyproject.toml probe must not discard a package manager that was + // detected successfully (that would bias the manager distribution toward + // `unknown`). Either failing just narrows the attempt, never breaks the + // user's setup run. try { packageManager = (await this.deps.getPackageManager()) ?? "unknown"; + } catch { + // Keep `unknown`. + } + try { isGreenfield = await greenfieldSignal( packageManager, projectRoot, this.deps.hasPyprojectToml ); } catch { - // Keep the defaults: an attempt with a coarser package-manager - // value is still worth recording, and a probe failure must not cost - // the user their setup run. + // Leave isGreenfield undefined, i.e. omitted from the event. } try { const reportResult = this.deps.recordSetupAttempt({ diff --git a/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts b/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts index cc7a63b11..f24a46521 100644 --- a/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts +++ b/packages/databricks-vscode/src/python-setup/controllers/pythonSetupDeps.ts @@ -161,6 +161,7 @@ export function makePythonSetupDeps( }, recordSetupAttempt: (attempt) => wiring.telemetry.recordPythonSetupAttempt(attempt), + recordNoCompute: () => wiring.telemetry.recordPythonSetupNoCompute(), getPackageManager: async () => { const root = wiring.projectRoot(); if (root === undefined) { diff --git a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md index e74ee185b..acf22bcfe 100644 --- a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md +++ b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md @@ -30,18 +30,29 @@ inflate into several results. The attempt is recorded once the compute target is resolved and immediately before the CLI is spawned — that is, once a run is genuinely about to happen. -Clicks that stop earlier (no project open, the visibility gate closed, or no -compute attached) record **nothing**; those are already visible as -`python_env.setup.detected` with trigger `explicit_command`. -That gives a three-stage funnel: +Clicks that stop earlier fall into two groups: + +- **No compute attached** (or a serverless session with no chosen version) — the + CTA is a visible dead end, so this reports a lone + `python_env.setup.result` with `outcome: "no_compute"` and no attempt. +- **No project open, or the visibility gate closed** — records nothing. Neither + is a dead end: with no project there is nothing to set up, and a closed gate + means the entry was never shown in the first place. ``` -python_env.setup.detected (explicit_command) user clicked - └─ python_env.setup.attempt a run started - └─ python_env.setup.result how it ended +python_env.setup.attempt a run started + └─ python_env.setup.result how it ended +python_env.setup.result(no_compute) the CTA dead-ended, no run ``` +Note that `python_env.setup.detected` does **not** provide a top-of-funnel stage +for this flow. Its `explicit_command` trigger fires only from the _legacy_ +`databricks.environment.setup` command, whereas the uv-native entry dispatches +`databricks.environment.setupPythonEnv` — and the config view renders the two +mutually exclusively, so a user who sees this entry never emits that event. Any +click-through analysis has to start from `attempt` plus the `no_compute` result. + Overlapping clicks coalesce onto the in-flight run (the orchestrator's re-entrancy guard), so they produce one attempt, not two. @@ -73,14 +84,14 @@ result event instead; the two join on session. ## `python_env.setup.result` schema -| Field | Type | Notes | -| -------------- | ---------- | ----------------------------------------------------------- | -| `outcome` | enum | `ok \| failed \| cancelled \| not_started`. | -| `failurePhase` | enum? | The CLI's six phases, plus `adopt` / `persist` (see below). | -| `errorCode` | enum? | The CLI's stable `E_*` failure class. | -| `envKey` | `string?` | e.g. `dbr/15.4.x-scala2.12`, `serverless/serverless-v5`. | -| `diskMutated` | `boolean?` | Whether a failed run had already modified project files. | -| `duration` | number | Milliseconds, measured by the extension (see below). | +| Field | Type | Notes | +| -------------- | ---------- | ----------------------------------------------------------------- | +| `outcome` | enum | `ok \| failed \| cancelled \| not_started \| no_compute`. | +| `failurePhase` | enum? | The CLI's six phases, plus `adopt` / `persist` (see below). | +| `errorCode` | enum? | The CLI's stable `E_*` failure class. | +| `envKey` | `string?` | e.g. `dbr/15.4.x-scala2.12`, `serverless/serverless-v5`. | +| `diskMutated` | `boolean?` | Whether a failed run had already modified project files. | +| `duration` | `number?` | Milliseconds, measured by the extension. Absent for `no_compute`. | ### Outcome values @@ -90,11 +101,16 @@ result event instead; the two join on session. | `failed` | CLI returned `ok:false`, **or** adoption failed after a good run. | | `cancelled` | The user cancelled the progress notification. | | `not_started` | Spawn/parse error — the CLI produced no result object at all. | +| `no_compute` | The CTA dead-ended: nothing was attached to set up for. | `cancelled` is kept distinct from `failed` on purpose: a user abandoning a slow setup is a signal about provisioning time, not about breakage. `not_started` is distinct because there is no phase or error code to attribute the break to. +`no_compute` is the one outcome emitted **without** a preceding attempt, and the +only one with no `duration` — nothing ran, and a 0 ms value would drag the +setup-time percentiles down. Exclude it when computing a per-run success rate. + ### The extension-side phases Two phases are appended to the CLI's canonical six. Both cover steps that run @@ -154,6 +170,15 @@ IDs, package names, project names, or user content. Optional fields are **omitte when unknown** rather than sent as `undefined` — the transport would stringify that to the literal `"undefined"` and pollute the schema. +The emit half names every field explicitly instead of spreading the caller's +object. This is load-bearing, not style: spreading a _variable_ disables +TypeScript's excess-property check, so any field later added to +`PythonSetupAttempt` / `PythonSetupOutcomeReport` — or any wider object passed +through the seam — would be emitted automatically (with objects +JSON-stringified), on a clean build. Enumerating the fields makes the schema an +allowlist the compiler enforces, so this document's privacy claim cannot silently +go stale. + Telemetry never costs the user their setup run: gathering the attempt's context is wrapped so a detection/probe failure degrades to `unknown` (and an omitted `isGreenfield`) instead of propagating into the flow. diff --git a/packages/databricks-vscode/src/telemetry/constants.ts b/packages/databricks-vscode/src/telemetry/constants.ts index 9e8821f77..51306ab9c 100644 --- a/packages/databricks-vscode/src/telemetry/constants.ts +++ b/packages/databricks-vscode/src/telemetry/constants.ts @@ -81,8 +81,19 @@ export type {PythonSetupMode, PythonSetupErrorCode}; * (spawn/parse error), so no phase or error code exists to attribute. And * `cancelled` is distinct from both — a user abandoning a slow setup is the * signal that the provisioning time is unacceptable, not that it broke. + * + * `no_compute` is the pre-flight dead end: the user pressed the CTA with no + * cluster attached (or a serverless session with no chosen version), so nothing + * could run. It is reported without a preceding attempt — see + * {@link Telemetry.recordPythonSetupNoCompute} — because measuring how often the + * button is a dead end is the whole point of tracking it. */ -export type PythonSetupOutcome = "ok" | "failed" | "cancelled" | "not_started"; +export type PythonSetupOutcome = + | "ok" + | "failed" + | "cancelled" + | "not_started" + | "no_compute"; /** * Where a failed setup broke: the CLI's six canonical phases, plus the two @@ -344,22 +355,27 @@ export class EventTypes { "nothing about greenfield-ness, so the signal would be misleading", }, }; - [Events.PYTHON_ENV_SETUP_RESULT]: EventType< - { - outcome: PythonSetupOutcome; - failurePhase?: PythonSetupFailurePhase; - errorCode?: PythonSetupErrorCode; - envKey?: string; - diskMutated?: boolean; - } & DurationMeasurement - > = { + [Events.PYTHON_ENV_SETUP_RESULT]: EventType<{ + outcome: PythonSetupOutcome; + failurePhase?: PythonSetupFailurePhase; + errorCode?: PythonSetupErrorCode; + envKey?: string; + diskMutated?: boolean; + // Optional rather than the usual required DurationMeasurement: the + // `no_compute` outcome is reported without a run having started, so + // there is no elapsed time. Emitting 0 there would drag the + // setup-time percentiles toward zero. + duration?: number; + }> = { comment: "The outcome of a uv-native Python environment setup run. Pairs 1:1 with a preceding " + - "python_env.setup.attempt. The failure phase localises where the funnel breaks without " + - "requiring funnel tracking. Categorical data only.", + "python_env.setup.attempt, except for outcome=no_compute, which is reported on its own " + + "when the user pressed the CTA with nothing attached to set up for. The failure phase " + + "localises where the funnel breaks without requiring funnel tracking. Categorical data only.", outcome: { comment: - "ok | failed | cancelled (user aborted) | not_started (the CLI produced no result)", + "ok | failed | cancelled (user aborted) | not_started (the CLI produced no " + + "result) | no_compute (the CTA was a dead end: nothing was attached to set up for)", }, failurePhase: { comment: diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts index a477e5244..c0d74dbbb 100644 --- a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.test.ts @@ -196,6 +196,12 @@ describe(__filename, () => { "/Users/someone/projects/secret-project", "serverless/serverless-vNEXT", "", + // Cluster *names* are user-chosen and often contain a person's + // name. These must not pass as a "spark version". + "dbr/janes-dev-cluster", + "dbr/johns.laptop.cluster", + "dbr/jdoe-databricks-com", + `dbr/${"a".repeat(500)}`, ]) { const {telemetry, events} = makeTelemetry(); telemetry.recordPythonSetupAttempt({ @@ -207,6 +213,61 @@ describe(__filename, () => { } }); + it("emits only the schema's fields, never extra ones on the caller's object", () => { + const {telemetry, events} = makeTelemetry(); + + // Model a future refactor that widens the attempt/report objects (or + // passes a wider object through this seam). The transport must be an + // allowlist: TypeScript's excess-property check does not apply to a + // spread variable, so the emit half is the only place this can be + // enforced. + telemetry.recordPythonSetupAttempt({ + packageManager: "uv", + targetType: "cluster", + mode: "default", + clusterId: "0710-142042-secretcluster", + projectPath: "/Users/jane/projects/acme", + } as any)({ + outcome: "ok", + envKey: "dbr/15.4.x-scala2.12", + rawCliMessage: "failed for user jane@example.com", + } as any); + + for (const event of events) { + const serialized = JSON.stringify(event.props); + expect(serialized).to.not.contain("0710"); + expect(serialized).to.not.contain("jane"); + expect(serialized).to.not.contain("acme"); + } + expect(Object.keys(events[0].props).sort()).to.deep.equal([ + "event.mode", + "event.packageManager", + "event.targetType", + "version", + ]); + expect(Object.keys(events[1].props).sort()).to.deep.equal([ + "event.envKey", + "event.outcome", + "version", + ]); + }); + + it("reports no_compute on its own, with no attempt and no duration", () => { + const {telemetry, events} = makeTelemetry(); + + telemetry.recordPythonSetupNoCompute(); + + expect(events).to.have.length(1); + expect(events[0].name).to.equal("python_env.setup.result"); + expect(events[0].props).to.deep.equal({ + "version": "1.0", + "event.outcome": "no_compute", + }); + // Nothing ran, so a 0ms duration would drag the setup-time percentiles + // down rather than describing anything. + expect(events[0].metrics).to.not.have.property("event.duration"); + }); + it("sends nothing when the telemetry reporter is unavailable", () => { // No reporter: recordEvent short-circuits, so neither event is built. // (Level-based opt-out is enforced inside the real reporter and covered diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts index 7c927a9cf..3b39dbddf 100644 --- a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts @@ -41,30 +41,20 @@ export type PythonSetupResultReporter = ( report: PythonSetupOutcomeReport ) => void; -/** - * Drop keys whose value is `undefined`. - * - * `recordEvent` stringifies an explicit `undefined` to the literal "undefined", - * so passing an absent optional through would pollute the event schema with a - * bogus value. Callers build reports straight from optional chaining - * (`result.error?.code`), so the filtering belongs here rather than at every - * call site. - */ -function withoutUndefined(source: T): Partial { - return Object.fromEntries( - Object.entries(source).filter(([, v]) => v !== undefined) - ) as Partial; -} - /** * The env-key shapes the CLI produces: `serverless/serverless-v` and * `dbr/` (see `EnvKeyForServerless` / `EnvKeyForSparkVersion`). - * The DBR arm is deliberately narrow — a Spark version is dotted/dashed - * alphanumerics, nothing else. + * + * The DBR arm matches the Spark-version grammar (`15.4.x-scala2.12`, + * `14.3.x-photon-scala2.12`) rather than "alphanumerics and punctuation": the + * looser form would admit a cluster *name*, which is user-chosen and routinely + * contains a person's name (`dbr/janes-dev-cluster` would have passed). The + * leading `..` requirement and the length bound are what keep this + * a closed vocabulary. */ const ENV_KEY_PATTERNS = [ /^serverless\/serverless-v\d+$/, - /^dbr\/[A-Za-z0-9][A-Za-z0-9.\-_]*$/, + /^dbr\/\d+\.\d+\.[A-Za-z0-9.-]{1,30}$/, ]; /** @@ -112,19 +102,47 @@ declare module "." { recordPythonSetupAttempt( attempt: PythonSetupAttempt ): PythonSetupResultReporter; + + /** + * Record that the setup CTA was a dead end: it was pressed with no + * compute attached (or a serverless session with no chosen version), so + * no run could start. + * + * Emits a lone PYTHON_ENV_SETUP_RESULT with `outcome: "no_compute"` and + * no attempt, since no run was attempted. This is the one intentional + * exception to the 1:1 pairing, and it exists because the alternative — + * relying on `python_env.setup.detected` to cover early aborts — does not + * work for this cohort: that event's `explicit_command` trigger fires + * only from the *legacy* setup command, and the config view shows the + * legacy checklist and the uv-native entry mutually exclusively. + */ + recordPythonSetupNoCompute(): void; } } +// Both payloads below name every field explicitly instead of spreading the +// caller's object. Spreading a *variable* switches off TypeScript's +// excess-property check, so any field later added to PythonSetupAttempt / +// PythonSetupOutcomeReport — or any wider object passed through this seam — +// would be emitted automatically, with objects JSON-stringified by +// recordEvent's addKeys. That would make this transport silently widen what is +// collected on a clean build. Enumerating the fields makes the event schema an +// allowlist the compiler enforces, which is what the privacy claim in +// PYTHON_SETUP_TELEMETRY.md rests on. Optionals are spread individually so an +// absent one is omitted rather than serialized as the string "undefined". Telemetry.prototype.recordPythonSetupAttempt = function ( attempt: PythonSetupAttempt ): PythonSetupResultReporter { this.recordEvent(Events.PYTHON_ENV_SETUP_ATTEMPT, { - ...withoutUndefined(attempt), - // Re-assert the required fields: withoutUndefined widens everything to - // optional, and the event schema requires these three. packageManager: attempt.packageManager, targetType: attempt.targetType, mode: attempt.mode, + ...(attempt.serverlessVersion !== undefined + ? {serverlessVersion: attempt.serverlessVersion} + : {}), + ...(attempt.isGreenfield !== undefined + ? {isGreenfield: attempt.isGreenfield} + : {}), }); // start() stamps the elapsed time onto the result event as `duration`. @@ -139,11 +157,26 @@ Telemetry.prototype.recordPythonSetupAttempt = function ( } reported = true; reportResult({ - ...withoutUndefined(report), outcome: report.outcome, + ...(report.failurePhase !== undefined + ? {failurePhase: report.failurePhase} + : {}), + ...(report.errorCode !== undefined + ? {errorCode: report.errorCode} + : {}), ...(report.envKey !== undefined ? {envKey: categoricalEnvKey(report.envKey)} : {}), + ...(report.diskMutated !== undefined + ? {diskMutated: report.diskMutated} + : {}), }); }; }; + +Telemetry.prototype.recordPythonSetupNoCompute = function () { + // `duration` is deliberately omitted, not 0: nothing ran, and a zero would + // drag the setup-time percentiles down. Recorded directly rather than via + // start(), which always stamps an elapsed time. + this.recordEvent(Events.PYTHON_ENV_SETUP_RESULT, {outcome: "no_compute"}); +}; From a87085a60b64c88616b0887d53c5f066ea3ffc75 Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Tue, 4 Aug 2026 12:09:18 +0200 Subject: [PATCH 4/5] docs(telemetry): move the setup-telemetry doc to README.md and trim it *Why* Review feedback: a feature-specific PYTHON_SETUP_TELEMETRY.md dropped into the shared src/telemetry/ folder is undiscoverable. Nothing linked to it -- the same is true of the PACKAGE_MANAGER_DETECTION.md it was modelled on -- so the next person adds a third file instead of extending one. It also restated the event schema in prose tables, giving a second source of truth that goes stale silently while the generated telemetry.json stays correct. *What* - Rename to src/telemetry/README.md, the name tooling and humans already look for and what GitHub renders when browsing the folder. Links out to the existing PACKAGE_MANAGER_DETECTION.md rather than absorbing it. - Trim 203 -> 101 lines by dropping every field table. EventTypes in constants.ts owns the schema (telemetry.json is generated from it); the doc now keeps only the reasoning that has nowhere to live in a field comment: why duration is measured in-extension, why the merge-conflict count is omitted, why the emit half enumerates fields, why envKey is pattern-checked, why no_compute breaks the attempt/result pairing. - Document the convention in CODE_CONVENTIONS.md (new section 4a) so this is a repo rule and not one PR's taste, with the survey that informed it: vscode colocates ~29 architecture docs under src/ and has no top-level docs/; react and rust colocate README.md per package/crate; vscode-pull-request-github, kubernetes and eslint centralize. We colocate because docs/ is gitignored here, and standardize on README.md rather than vscode's descriptive-filename style because that only stays navigable when a folder is one subsystem -- src/telemetry/ serves every feature. - Also note in section 4a and the telemetry bullet that EventTypes is the only place a schema may be maintained. *Verification* - yarn build clean; test:lint 0 errors, prettier clean - unit tests: 471 passing, same 6 pre-existing cli/CliWrapper.test.ts failures - grepped for references before renaming; the one code comment pointing at the old filename now points at the README Co-authored-by: Isaac --- CODE_CONVENTIONS.md | 187 ++++++++++------ .../src/telemetry/PYTHON_SETUP_TELEMETRY.md | 203 ------------------ .../databricks-vscode/src/telemetry/README.md | 101 +++++++++ .../src/telemetry/pythonSetupExtensions.ts | 6 +- 4 files changed, 228 insertions(+), 269 deletions(-) delete mode 100644 packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md create mode 100644 packages/databricks-vscode/src/telemetry/README.md diff --git a/CODE_CONVENTIONS.md b/CODE_CONVENTIONS.md index 7547eb8bf..d56d9d094 100644 --- a/CODE_CONVENTIONS.md +++ b/CODE_CONVENTIONS.md @@ -8,18 +8,18 @@ A practical guide for **writing new code that matches the existing codebase**. Use this as a lookup while you code. -| Thing | Convention | Example | -| ----------------------------------------------------------- | ------------------------------------------------------------- | --------------------------------------------- | -| File exporting a **class** | `PascalCase.ts` | `ClusterModel.ts` | -| File exporting **functions** (util / helper / registration) | `camelCase.ts` | `fileUtils.ts`, `registerDetailPanel.ts` | -| Class / interface / type / enum | `PascalCase` | `ConnectionManager`, `RunState` | -| Method / local variable / function | `camelCase` | `refresh()`, `activeCluster` | -| Module-level constant | `UPPER_SNAKE_CASE` | `SCHEME`, `PROD_APP_INSIGHTS_KEY` | +| Thing | Convention | Example | +| ----------------------------------------------------------- | ----------------------------------------------------------------------------------------- | --------------------------------------------------------- | +| File exporting a **class** | `PascalCase.ts` | `ClusterModel.ts` | +| File exporting **functions** (util / helper / registration) | `camelCase.ts` | `fileUtils.ts`, `registerDetailPanel.ts` | +| Class / interface / type / enum | `PascalCase` | `ConnectionManager`, `RunState` | +| Method / local variable / function | `camelCase` | `refresh()`, `activeCluster` | +| Module-level constant | `UPPER_SNAKE_CASE` | `SCHEME`, `PROD_APP_INSIGHTS_KEY` | | Private field | `camelCase`; `_`-prefix a private field backing a public getter/accessor of the same name | `private disposables`, `private _state` (→ `get state()`) | -| Public event | `onDid` | `onDidChangeState` | -| String-union members | quoted literals | `"CONNECTED" \| "DISCONNECTED"` | -| VS Code command ID | `databricks..` | `databricks.cluster.refresh` | -| Unit test | `.test.ts`, co-located | `ClusterModel.test.ts` | +| Public event | `onDid` | `onDidChangeState` | +| String-union members | quoted literals | `"CONNECTED" \| "DISCONNECTED"` | +| VS Code command ID | `databricks..` | `databricks.cluster.refresh` | +| Unit test | `.test.ts`, co-located | `ClusterModel.test.ts` | **Rule of thumb for file casing:** does the file's _primary_ export have a name starting with a capital (a class/type)? Name the file to match it exactly @@ -52,10 +52,10 @@ matches the responsibility - don't invent new ones. **The core triad.** Most features are built from three cooperating classes: -- **`XModel`** — owns the data and fires events when it changes. No VS Code UI. -- **`XManager`** — orchestrates: constructs collaborators, reacts to events, - handles refresh/polling. -- **`XCommands`** — thin command handlers that call into the model/manager. +- **`XModel`** — owns the data and fires events when it changes. No VS Code UI. +- **`XManager`** — orchestrates: constructs collaborators, reacts to events, + handles refresh/polling. +- **`XCommands`** — thin command handlers that call into the model/manager. **Keep `window.*` UI out of `Model` / `Manager` / `Loader`.** These surface data/results and fire events; let `Commands` / `Component` do the @@ -100,6 +100,8 @@ makes it hard to unit-test (see section 8). dependencies, register commands. 7. **Add a barrel (`index.ts`) only if the feature has a clear public surface** other modules import — and make it a _selective_ re-export, not `export *`. +8. **If the feature needs a prose doc, add `README.md` to its folder** — see + section 4a. Most features don't. A typical new feature: @@ -161,21 +163,80 @@ dispose() { Three decorators are in use — prefer them over hand-rolled equivalents: -- **`@Mutex.synchronise("someMutexField")`** — serialize an async method against a - named `Mutex` field on the instance (`locking/Mutex.ts`). The local ESLint rule - `mutex-synchronised-decorator` verifies correct usage, so it will fail lint if - misapplied. -- **`@onError({log, popup})`** — uniform error handling / notifications on an async - method (`utils/onErrorDecorator.ts`). -- **`@logging.withLogContext(Loggers.Extension)`** — attach a logging context - (from `@databricks/sdk-experimental`), optionally with a `@context` parameter. +- **`@Mutex.synchronise("someMutexField")`** — serialize an async method against a + named `Mutex` field on the instance (`locking/Mutex.ts`). The local ESLint rule + `mutex-synchronised-decorator` verifies correct usage, so it will fail lint if + misapplied. +- **`@onError({log, popup})`** — uniform error handling / notifications on an async + method (`utils/onErrorDecorator.ts`). +- **`@logging.withLogContext(Loggers.Extension)`** — attach a logging context + (from `@databricks/sdk-experimental`), optionally with a `@context` parameter. ### Logging & telemetry -- Log through the named loggers in `logger/` (`Loggers.Extension`, …) — **never - `console.log`** (`no-console` is an ESLint error outside tests). -- Define new telemetry events in `telemetry/constants.ts`. User-facing commands are - instrumented automatically by the `telemetry.registerCommand` wrapper (section 6). +- Log through the named loggers in `logger/` (`Loggers.Extension`, …) — **never + `console.log`** (`no-console` is an ESLint error outside tests). +- Define new telemetry events in `telemetry/constants.ts`. User-facing commands are + instrumented automatically by the `telemetry.registerCommand` wrapper (section 6). +- **`EventTypes` in `telemetry/constants.ts` is the only source of truth for an + event's schema.** Each field carries a `comment`; `telemetry.json` is generated + from the class by `scripts/generateTelemetry.ts` and is gitignored. Never + hand-maintain a field list elsewhere — including in prose docs, which go stale + silently while the generated file stays correct. + +--- + +## 4a. Where module documentation goes + +Most code needs no prose doc — a class doc-comment and good names are enough. +When a module genuinely needs one (a design rationale, a privacy posture, a +protocol), write a **`README.md` in the module's folder**, next to the code it +describes. + +- **`README.md`, not a descriptive name.** `src/telemetry/README.md`, not + `src/telemetry/PYTHON_SETUP_TELEMETRY.md`. A file named for one feature inside a + shared folder is undiscoverable — nothing links to it, and the next person adds + a second one rather than extending it. `README.md` is the name tooling and + humans already look for, and GitHub renders it when browsing the folder. +- **Document the _why_, not the _what_.** Schemas, field lists, and signatures + belong in the code (or, for telemetry, in generated output). A doc that restates + them acquires a second source of truth that drifts. Record the decisions and + trade-offs that have nowhere else to live. +- **No top-level `docs/`.** It is gitignored in this repo (local scratch only), so + anything committed must live beside the code or in an existing root file + (`README.md`, `CONTRIBUTING.md`, this file). + +### Why colocation + +Surveyed when this convention was written (August 2026): + +| Project | Practice | +| ----------------------------------- | --------------------------------------------------------------------------------------------------------------- | +| [microsoft/vscode][vsc] | Colocated docs, **no top-level `docs/`**: ~29 architecture docs under `src/` (e.g. `src/vs/sessions/LAYOUT.md`) | +| [facebook/react][react] | Colocated `README.md` per package (`packages/react-reconciler/README.md`) | +| [rust-lang/rust][rust] | Colocated `README.md` per crate, a few lines, pointing at the external dev guide | +| [vscode-pull-request-github][vscpr] | Centralized `documentation/` directory | +| [kubernetes][k8s], [eslint][eslint] | Centralized `docs/` + external enhancement proposals | + +Both colocation and centralization are well-established; there is no universal +answer. We colocate because this repo has no committed `docs/` tree, and we +standardize on `README.md` (React/Rust style) rather than VS Code's +descriptive-filename style because the latter only stays navigable when a folder +is one subsystem — `src/telemetry/` serves every feature. + +**Telemetry specifically:** VS Code documents events _inline in TypeScript_ — +GDPR classification, purpose, and comment in a type beside the emit call — with no +per-event markdown ([`telemetry.instructions.md`][vsctel]). Our `EventTypes` +`comment` fields are the same idea, which is why prose docs must not duplicate +them. + +[vsc]: https://github.com/microsoft/vscode/tree/main/src/vs/sessions +[vsctel]: https://github.com/microsoft/vscode/blob/main/.github/instructions/telemetry.instructions.md +[react]: https://github.com/facebook/react/blob/main/packages/react-reconciler/README.md +[rust]: https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/README.md +[vscpr]: https://github.com/microsoft/vscode-pull-request-github/tree/main/documentation +[k8s]: https://github.com/kubernetes/enhancements +[eslint]: https://github.com/eslint/eslint/tree/main/docs --- @@ -201,12 +262,12 @@ The Databricks SDK (`@databricks/sdk-experimental`) is the biggest unguarded cross-cutting dependency in the codebase — dozens of files import it, and many reach the `WorkspaceClient` / `apiClient` directly. -- **Reach the workspace client through the connection seam** (`ConnectionManager`), - not by constructing your own client in a feature. -- **Never import through deep `/dist/...` paths** (`.../dist/apis/…`, - `.../dist/retries/…`). They're not a stable entry point — import from the package - root. Keeping SDK access behind one seam is also what turns a future SDK migration - into a bounded change instead of a repo-wide edit. +- **Reach the workspace client through the connection seam** (`ConnectionManager`), + not by constructing your own client in a feature. +- **Never import through deep `/dist/...` paths** (`.../dist/apis/…`, + `.../dist/retries/…`). They're not a stable entry point — import from the package + root. Keeping SDK access behind one seam is also what turns a future SDK migration + into a bounded change instead of a repo-wide edit. --- @@ -238,29 +299,29 @@ reach the `WorkspaceClient` / `apiClient` directly. ## 7. Imports & exports -- **Double quotes** for imports (Prettier-enforced). -- Order: external packages first (`vscode`, `@databricks/sdk-experimental`, …), - then local relative imports. -- Use **`import type { … }`** for type-only imports and to break dependency cycles. -- For utility folders, follow the **namespace-barrel** pattern — - `export * as FileUtils from "./fileUtils"` in `index.ts`, consumed as - `import {FileUtils} from "./utils"`. -- For feature barrels, re-export only the intended public surface; avoid blanket - `export *` of internal files. Blanket `export *` barrels hurt navigation — - "go to definition" and grep-for-usage land on the barrel instead of the real file, - adding an indirection hop with no encapsulation benefit. **Note:** most existing - `index.ts` files still use `export *` (10 of 12 today); those should migrate to - selective re-exports (or be dropped where under-used), not be treated as the - pattern to copy. +- **Double quotes** for imports (Prettier-enforced). +- Order: external packages first (`vscode`, `@databricks/sdk-experimental`, …), + then local relative imports. +- Use **`import type { … }`** for type-only imports and to break dependency cycles. +- For utility folders, follow the **namespace-barrel** pattern — + `export * as FileUtils from "./fileUtils"` in `index.ts`, consumed as + `import {FileUtils} from "./utils"`. +- For feature barrels, re-export only the intended public surface; avoid blanket + `export *` of internal files. Blanket `export *` barrels hurt navigation — + "go to definition" and grep-for-usage land on the barrel instead of the real file, + adding an indirection hop with no encapsulation benefit. **Note:** most existing + `index.ts` files still use `export *` (10 of 12 today); those should migrate to + selective re-exports (or be dropped where under-used), not be treated as the + pattern to copy. --- ## 8. Tests -- **Co-locate** unit tests: `X.test.ts` beside `X.ts` (Mocha via - `@vscode/test-electron`). Prefer testing `Model`/`Manager`/util logic — it's the - most testable, since UI is isolated behind `vscode-objs/`. -- Use the right suffix for the right kind of test: +- **Co-locate** unit tests: `X.test.ts` beside `X.ts` (Mocha via + `@vscode/test-electron`). Prefer testing `Model`/`Manager`/util logic — it's the + most testable, since UI is isolated behind `vscode-objs/`. +- Use the right suffix for the right kind of test: | Suffix | Kind | Where | | ------------ | -------------------------------- | -------------------- | @@ -269,7 +330,7 @@ reach the `WorkspaceClient` / `apiClient` directly. | `*.e2e.ts` | end-to-end (WebdriverIO) | `test/e2e/` | | `*_test.py` | Python unit | `test/python/` | -- Mock with `ts-mockito`. Never commit `.only` — `no-only-tests` is an error. +- Mock with `ts-mockito`. Never commit `.only` — `no-only-tests` is an error. --- @@ -278,19 +339,19 @@ reach the `WorkspaceClient` / `apiClient` directly. Formatting is enforced by Prettier + ESLint; `yarn fix` auto-applies most of it. The choices that affect how you write: -- 4-space indentation, double quotes, semicolons required. -- `{a: 1}` — **no** space inside braces (`bracketSpacing: false`). -- `(x) => …` — always parenthesize arrow params. -- Trailing commas where ES5 allows. -- Prefer `===` / `!==` and always use curly braces (`curly`). +- 4-space indentation, double quotes, semicolons required. +- `{a: 1}` — **no** space inside braces (`bracketSpacing: false`). +- `(x) => …` — always parenthesize arrow params. +- Trailing commas where ES5 allows. +- Prefer `===` / `!==` and always use curly braces (`curly`). --- ## Issues -- extension.ts is becoming too big -- Large feature folders mix many role-suffixes (logic / presentation / I/O) flat at - one level, which is hard to navigate — and co-located tests double the file count. - Today `run/` (14 files), `bundle/` (12), `language/` (11), and `ui/unity-catalog/` - (10) sit flat at the top level, across 17 distinct suffixes repo-wide. Needs a - grouping convention (see the section 3 role-suffix rule). +- extension.ts is becoming too big +- Large feature folders mix many role-suffixes (logic / presentation / I/O) flat at + one level, which is hard to navigate — and co-located tests double the file count. + Today `run/` (14 files), `bundle/` (12), `language/` (11), and `ui/unity-catalog/` + (10) sit flat at the top level, across 17 distinct suffixes repo-wide. Needs a + grouping convention (see the section 3 role-suffix rule). diff --git a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md b/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md deleted file mode 100644 index acf22bcfe..000000000 --- a/packages/databricks-vscode/src/telemetry/PYTHON_SETUP_TELEMETRY.md +++ /dev/null @@ -1,203 +0,0 @@ -# Telemetry: Python environment setup attempt / result - -Instrumentation for the uv-native "Set up Python environment" flow (VPEX): one -event when a setup run starts, one when it finishes. Together they measure the -funnel, where failures land across the CLI's phases, and how long provisioning -actually takes. - -These events do **not** change any setup behaviour. - -## Events - -| | | -| ---------------- | ---------------------------------------------------------------------------- | -| **Event names** | `python_env.setup.attempt`, `python_env.setup.result` | -| **Defined in** | `src/telemetry/constants.ts` (`Events.PYTHON_ENV_SETUP_ATTEMPT` / `_RESULT`) | -| **Emitted from** | `src/telemetry/pythonSetupExtensions.ts` (`recordPythonSetupAttempt`) | -| **Called from** | `src/python-setup/controllers/PythonSetupEnvironmentSetup.ts` (`runSetup`) | - -Transport is the shared `Telemetry` client, so properties are prefixed with -`event.`, `telemetry.telemetryLevel` opt-out is honoured, and the ambient -user/workspace envelope is attached automatically. - -`recordPythonSetupAttempt` emits the attempt and **returns the reporter for that -run's result**. The pairing is therefore structural rather than a convention: an -outcome cannot be reported without an attempt having been recorded, and the -reporter is once-only — a second call is dropped, so one attempt can never -inflate into several results. - -## When they fire - -The attempt is recorded once the compute target is resolved and immediately -before the CLI is spawned — that is, once a run is genuinely about to happen. - -Clicks that stop earlier fall into two groups: - -- **No compute attached** (or a serverless session with no chosen version) — the - CTA is a visible dead end, so this reports a lone - `python_env.setup.result` with `outcome: "no_compute"` and no attempt. -- **No project open, or the visibility gate closed** — records nothing. Neither - is a dead end: with no project there is nothing to set up, and a closed gate - means the entry was never shown in the first place. - -``` -python_env.setup.attempt a run started - └─ python_env.setup.result how it ended -python_env.setup.result(no_compute) the CTA dead-ended, no run -``` - -Note that `python_env.setup.detected` does **not** provide a top-of-funnel stage -for this flow. Its `explicit_command` trigger fires only from the _legacy_ -`databricks.environment.setup` command, whereas the uv-native entry dispatches -`databricks.environment.setupPythonEnv` — and the config view renders the two -mutually exclusively, so a user who sees this entry never emits that event. Any -click-through analysis has to start from `attempt` plus the `no_compute` result. - -Overlapping clicks coalesce onto the in-flight run (the orchestrator's -re-entrancy guard), so they produce one attempt, not two. - -## `python_env.setup.attempt` schema - -| Field | Type | Notes | -| ------------------- | ---------- | ----------------------------------------------------------------------------------- | -| `packageManager` | enum | `uv \| poetry \| pip \| conda \| unknown`. Priority `uv > poetry > conda > pip`. | -| `targetType` | enum | `cluster \| serverless`. **No** cluster IDs or names. | -| `serverlessVersion` | `string?` | The chosen serverless environment version (e.g. `"5"`). Omitted for clusters. | -| `mode` | enum | `default` (includes `databricks-connect`) \| `constraints-only`. | -| `isGreenfield` | `boolean?` | Project has no `pyproject.toml`. Omitted unless `packageManager` is `uv`/`unknown`. | - -### Why `isGreenfield` is conditional - -A missing `pyproject.toml` only means "greenfield" for a project with no -competing manager — pip and conda users may never have one, so for them the -absence says nothing and would inflate the greenfield rate. The field is emitted -only for `uv`/`unknown` projects, which is exactly the population the visibility -gate admits (`shouldShowPythonSetup` rejects anything with a pip/poetry/conda -signal). For other managers the probe is not even performed. - -### Why there is no `envKey` here - -A cluster's env key is `dbr/`, derived inside the CLI from a spark -version the extension never reads. Recomputing it locally would be a second -source of truth that can drift from the CLI. The authoritative key rides the -result event instead; the two join on session. - -## `python_env.setup.result` schema - -| Field | Type | Notes | -| -------------- | ---------- | ----------------------------------------------------------------- | -| `outcome` | enum | `ok \| failed \| cancelled \| not_started \| no_compute`. | -| `failurePhase` | enum? | The CLI's six phases, plus `adopt` / `persist` (see below). | -| `errorCode` | enum? | The CLI's stable `E_*` failure class. | -| `envKey` | `string?` | e.g. `dbr/15.4.x-scala2.12`, `serverless/serverless-v5`. | -| `diskMutated` | `boolean?` | Whether a failed run had already modified project files. | -| `duration` | `number?` | Milliseconds, measured by the extension. Absent for `no_compute`. | - -### Outcome values - -| Value | Meaning | -| ------------- | ------------------------------------------------------------------- | -| `ok` | CLI succeeded, venv provisioned **and** adopted as the interpreter. | -| `failed` | CLI returned `ok:false`, **or** adoption failed after a good run. | -| `cancelled` | The user cancelled the progress notification. | -| `not_started` | Spawn/parse error — the CLI produced no result object at all. | -| `no_compute` | The CTA dead-ended: nothing was attached to set up for. | - -`cancelled` is kept distinct from `failed` on purpose: a user abandoning a slow -setup is a signal about provisioning time, not about breakage. `not_started` is -distinct because there is no phase or error code to attribute the break to. - -`no_compute` is the one outcome emitted **without** a preceding attempt, and the -only one with no `duration` — nothing ran, and a 0 ms value would drag the -setup-time percentiles down. Exclude it when computing a per-run success rate. - -### The extension-side phases - -Two phases are appended to the CLI's canonical six. Both cover steps that run -_after_ the CLI has already exited ok, so its own `phases` array cannot describe -them: - -- **`adopt`** — pointing the MS Python extension at the provisioned venv. A venv - the editor never selects is unusable, so this counts as a setup failure. -- **`persist`** — recording the drift-detection baseline and readiness. The - environment itself works, but the extension's own state did not stick. - -The success report is emitted only after both have completed, so a throw in -either is never recorded as `ok`. It is emitted _before_ the success toast, -though: `showSuccess` resolves only when the user dismisses the notification, and -folding think-time into `duration` would wreck the metric. - -### `envKey` is constrained before emission - -`envKey` is a runtime coordinate from a closed vocabulary, **never** a cluster id -or a user-chosen cluster name. It is validated against the CLI's two documented -shapes (`serverless/serverless-v` and `dbr/`) before being -emitted; anything else collapses to `"other"`. - -This matters because the key is copied out of CLI JSON that the parser -deliberately validates only minimally, and the DBR arm is a raw -`"dbr/" + sparkVersion` concatenation. Without the check, schema drift or an -unexpected runtime string could put unbounded, potentially identifying, -high-cardinality content into a field documented as categorical. - -## Two ERD fields deliberately not reported - -The design doc for this work asked for a duration and a merge-conflict warning -count. Neither can be taken from the CLI result as-is: - -1. **Duration is measured in the extension, not read from `result.durationMs`.** - The CLI documents that field as reserved and always emits `0` - (`libs/localenv/result.go`: _"the pipeline does not measure wall time … so it - is always emitted as 0"_). The extension clock starts when the attempt is - recorded, which is also the better measurement: it is the latency the user - experiences, including process spawn and interpreter adoption. - -2. **The merge-conflict warning count is not emitted at all.** Nothing in the CLI - ever appends to `Result.Warnings` — `NewResult()` seeds it to `[]` and only - the text renderer reads it — and merge conflicts are not a modelled concept - there. The field would be a permanent `0`, and a dashboard built on it would - read "merge quality is perfect" when the truth is "unmeasured". It will be - added once the CLI has a producer for it. - -Also note `mode` is currently always `default`: the orchestrator hardcodes it -until the Quick-setup / `--constraints-only` picker ships. The field is in the -schema from the start so no migration is needed then. - -## Privacy - -Only categorical/enum values and a duration. No file paths, cluster names or -IDs, package names, project names, or user content. Optional fields are **omitted -when unknown** rather than sent as `undefined` — the transport would stringify -that to the literal `"undefined"` and pollute the schema. - -The emit half names every field explicitly instead of spreading the caller's -object. This is load-bearing, not style: spreading a _variable_ disables -TypeScript's excess-property check, so any field later added to -`PythonSetupAttempt` / `PythonSetupOutcomeReport` — or any wider object passed -through the seam — would be emitted automatically (with objects -JSON-stringified), on a clean build. Enumerating the fields makes the schema an -allowlist the compiler enforces, so this document's privacy claim cannot silently -go stale. - -Telemetry never costs the user their setup run: gathering the attempt's context -is wrapped so a detection/probe failure degrades to `unknown` (and an omitted -`isGreenfield`) instead of propagating into the flow. - -Because every input is already in the orchestrator's local scope, an opted-out -user incurs no extra work — unlike package-manager detection, there are no -speculative disk reads to guard. - -Like every event from this extension, these inherit the ambient user/workspace -envelope (`user.hashedUserName`, `user.host`, `workspaceId`, `authType`), so the -outcome is linked to a stable hashed identity. - -## Suggested analysis - -- Funnel: `detected(explicit_command)` → `attempt` → `result(outcome=ok)`. -- Failure distribution over `failurePhase` × `errorCode` — where the funnel - breaks, without funnel tracking. -- `duration` percentiles for `outcome=ok`, to test the ~3 min setup claim; and - the `cancelled` rate against that distribution. -- Greenfield vs existing-project success rates (`isGreenfield` on the attempt, - joined to the result by session). -- `diskMutated` on failures — how often a failed run leaves the project modified. diff --git a/packages/databricks-vscode/src/telemetry/README.md b/packages/databricks-vscode/src/telemetry/README.md new file mode 100644 index 000000000..c27f455e1 --- /dev/null +++ b/packages/databricks-vscode/src/telemetry/README.md @@ -0,0 +1,101 @@ +# Telemetry + +Events are defined in `constants.ts`: add a member to the `Events` enum and a +typed entry to `EventTypes`, where each field carries a `comment` describing it. +**That is the schema's single source of truth** — `telemetry.json` is generated +from it by `scripts/generateTelemetry.ts` (and is gitignored), so field lists are +never maintained by hand, here or anywhere else. + +This file records the decisions behind the two non-obvious event families — the +reasoning that has nowhere to live in a field comment. For what each field _is_, +read `EventTypes`. + +## Python package-manager detection + +`python_env.setup.detected` — see [PACKAGE_MANAGER_DETECTION.md](./PACKAGE_MANAGER_DETECTION.md). + +## Python environment setup (VPEX) + +`python_env.setup.attempt` / `python_env.setup.result`, emitted by +`pythonSetupExtensions.ts` and called from +`python-setup/controllers/PythonSetupEnvironmentSetup.ts`. + +`recordPythonSetupAttempt` emits the attempt and **returns the reporter for that +run's result**, so the 1:1 pairing is structural rather than a convention: an +outcome cannot be reported without an attempt, and the reporter is once-only. + +### Why the emit half lists every field explicitly + +It would be shorter to spread the caller's object. Don't. Spreading a _variable_ +disables TypeScript's excess-property check, so any field later added to +`PythonSetupAttempt` / `PythonSetupOutcomeReport` — or any wider object passed +through the seam — would be emitted automatically, with objects +JSON-stringified. That was demonstrated in review: adding a field holding a +cluster ID put it on the wire with an exit-0 build. Enumerating the fields makes +the schema an allowlist the compiler enforces. + +### Why `duration` is measured here, not read from the CLI + +The CLI's `durationMs` is documented as reserved and always emits `0` +(`libs/localenv/result.go`). Measuring in-extension is also the better number: +it is the latency the user experiences, including process spawn and interpreter +adoption. + +It is reported _before_ the success toast, because `showSuccess` resolves only +when the user dismisses the notification — folding think-time in would wreck the +metric. Everything that can fail (adoption, state persistence) happens before the +report, so a throw is never recorded as `ok`. + +### Why there is no merge-conflict warning count + +The design asked for one as a merge-quality proxy. Nothing in the CLI ever +appends to `Result.Warnings` — `NewResult()` seeds it to `[]` and only the text +renderer reads it — so the field would be a permanent `0`, which reads as "merge +quality is perfect" rather than "unmeasured". Tracked in DECO-27875; add the +field once there is a producer. + +### Why `isGreenfield` is conditional + +A missing `pyproject.toml` only means "greenfield" for a project with no +competing manager: pip and conda users may never have one. It is emitted only for +`uv`/`unknown` projects — exactly the population `shouldShowPythonSetup` admits — +and for other managers the probe is not even performed. + +### Why `envKey` is pattern-checked + +It is copied from CLI JSON that the parser validates only minimally, and its DBR +arm is a raw `"dbr/" + sparkVersion` concatenation. The check keeps it a closed +vocabulary; anything unrecognised collapses to `"other"`. Note the pattern +deliberately matches the spark-version grammar rather than "alphanumerics and +punctuation" — the looser form admitted cluster _names_, which are user-chosen +and routinely contain a person's name. + +### Why `no_compute` has no attempt and no duration + +Pressing the CTA with nothing attached is a real user-facing dead end, so it is +worth counting — but no run started, so there is no attempt to pair with and no +elapsed time (a `0` would drag the setup-time percentiles down). Exclude it when +computing a per-run success rate. + +It exists because `python_env.setup.detected` does **not** cover early aborts for +this flow: its `explicit_command` trigger fires only from the _legacy_ +`databricks.environment.setup` command, while the uv-native entry dispatches +`databricks.environment.setupPythonEnv`, and the config view renders the two +mutually exclusively. A user who sees this entry never emits that event. + +## Privacy + +Only categorical/enum values and durations — no file paths, cluster names or IDs, +package names, project names, or user content. Absent optionals are **omitted** +rather than passed as `undefined`, which `recordEvent` would stringify to the +literal `"undefined"`. + +Every event also inherits the ambient user/workspace envelope +(`user.hashedUserName`, `user.host`, `workspaceId`, `authType`), so payloads that +carry no identifiers still link to a stable hashed identity. + +Opt-out rides the client: `recordEvent` drops events when no reporter exists, and +`@vscode/extension-telemetry` honours `telemetry.telemetryLevel`. Callers that +would do real work purely to build an event (e.g. reading project files) should +short-circuit on `isTelemetryEnabled` so an opted-out user gets no disk access +either. diff --git a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts index 3b39dbddf..6d92678c4 100644 --- a/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts +++ b/packages/databricks-vscode/src/telemetry/pythonSetupExtensions.ts @@ -127,9 +127,9 @@ declare module "." { // would be emitted automatically, with objects JSON-stringified by // recordEvent's addKeys. That would make this transport silently widen what is // collected on a clean build. Enumerating the fields makes the event schema an -// allowlist the compiler enforces, which is what the privacy claim in -// PYTHON_SETUP_TELEMETRY.md rests on. Optionals are spread individually so an -// absent one is omitted rather than serialized as the string "undefined". +// allowlist the compiler enforces, which is what the privacy claim in this +// folder's README rests on. Optionals are spread individually so an absent one +// is omitted rather than serialized as the string "undefined". Telemetry.prototype.recordPythonSetupAttempt = function ( attempt: PythonSetupAttempt ): PythonSetupResultReporter { From cc87c3106d8f6cb250a0d829179ba9ad5297af5c Mon Sep 17 00:00:00 2001 From: "@rugpanov" Date: Tue, 4 Aug 2026 12:15:10 +0200 Subject: [PATCH 5/5] docs: split the module-doc convention out of this PR *Why* The CODE_CONVENTIONS.md change is a repo-wide rule that deserves its own review by whoever owns the conventions -- it should not ride in on a telemetry feature PR. *What* Reverts CODE_CONVENTIONS.md to its state before a87085a. The telemetry README rename/trim stays here, since it is part of this feature. The new section 4a ("Where module documentation goes") moves to a dedicated PR. *Verification* CODE_CONVENTIONS.md is byte-identical to e423680's version (git diff empty); no source files touched. Co-authored-by: Isaac --- CODE_CONVENTIONS.md | 187 +++++++++++++++----------------------------- 1 file changed, 63 insertions(+), 124 deletions(-) diff --git a/CODE_CONVENTIONS.md b/CODE_CONVENTIONS.md index d56d9d094..7547eb8bf 100644 --- a/CODE_CONVENTIONS.md +++ b/CODE_CONVENTIONS.md @@ -8,18 +8,18 @@ A practical guide for **writing new code that matches the existing codebase**. Use this as a lookup while you code. -| Thing | Convention | Example | -| ----------------------------------------------------------- | ----------------------------------------------------------------------------------------- | --------------------------------------------------------- | -| File exporting a **class** | `PascalCase.ts` | `ClusterModel.ts` | -| File exporting **functions** (util / helper / registration) | `camelCase.ts` | `fileUtils.ts`, `registerDetailPanel.ts` | -| Class / interface / type / enum | `PascalCase` | `ConnectionManager`, `RunState` | -| Method / local variable / function | `camelCase` | `refresh()`, `activeCluster` | -| Module-level constant | `UPPER_SNAKE_CASE` | `SCHEME`, `PROD_APP_INSIGHTS_KEY` | +| Thing | Convention | Example | +| ----------------------------------------------------------- | ------------------------------------------------------------- | --------------------------------------------- | +| File exporting a **class** | `PascalCase.ts` | `ClusterModel.ts` | +| File exporting **functions** (util / helper / registration) | `camelCase.ts` | `fileUtils.ts`, `registerDetailPanel.ts` | +| Class / interface / type / enum | `PascalCase` | `ConnectionManager`, `RunState` | +| Method / local variable / function | `camelCase` | `refresh()`, `activeCluster` | +| Module-level constant | `UPPER_SNAKE_CASE` | `SCHEME`, `PROD_APP_INSIGHTS_KEY` | | Private field | `camelCase`; `_`-prefix a private field backing a public getter/accessor of the same name | `private disposables`, `private _state` (→ `get state()`) | -| Public event | `onDid` | `onDidChangeState` | -| String-union members | quoted literals | `"CONNECTED" \| "DISCONNECTED"` | -| VS Code command ID | `databricks..` | `databricks.cluster.refresh` | -| Unit test | `.test.ts`, co-located | `ClusterModel.test.ts` | +| Public event | `onDid` | `onDidChangeState` | +| String-union members | quoted literals | `"CONNECTED" \| "DISCONNECTED"` | +| VS Code command ID | `databricks..` | `databricks.cluster.refresh` | +| Unit test | `.test.ts`, co-located | `ClusterModel.test.ts` | **Rule of thumb for file casing:** does the file's _primary_ export have a name starting with a capital (a class/type)? Name the file to match it exactly @@ -52,10 +52,10 @@ matches the responsibility - don't invent new ones. **The core triad.** Most features are built from three cooperating classes: -- **`XModel`** — owns the data and fires events when it changes. No VS Code UI. -- **`XManager`** — orchestrates: constructs collaborators, reacts to events, - handles refresh/polling. -- **`XCommands`** — thin command handlers that call into the model/manager. +- **`XModel`** — owns the data and fires events when it changes. No VS Code UI. +- **`XManager`** — orchestrates: constructs collaborators, reacts to events, + handles refresh/polling. +- **`XCommands`** — thin command handlers that call into the model/manager. **Keep `window.*` UI out of `Model` / `Manager` / `Loader`.** These surface data/results and fire events; let `Commands` / `Component` do the @@ -100,8 +100,6 @@ makes it hard to unit-test (see section 8). dependencies, register commands. 7. **Add a barrel (`index.ts`) only if the feature has a clear public surface** other modules import — and make it a _selective_ re-export, not `export *`. -8. **If the feature needs a prose doc, add `README.md` to its folder** — see - section 4a. Most features don't. A typical new feature: @@ -163,80 +161,21 @@ dispose() { Three decorators are in use — prefer them over hand-rolled equivalents: -- **`@Mutex.synchronise("someMutexField")`** — serialize an async method against a - named `Mutex` field on the instance (`locking/Mutex.ts`). The local ESLint rule - `mutex-synchronised-decorator` verifies correct usage, so it will fail lint if - misapplied. -- **`@onError({log, popup})`** — uniform error handling / notifications on an async - method (`utils/onErrorDecorator.ts`). -- **`@logging.withLogContext(Loggers.Extension)`** — attach a logging context - (from `@databricks/sdk-experimental`), optionally with a `@context` parameter. +- **`@Mutex.synchronise("someMutexField")`** — serialize an async method against a + named `Mutex` field on the instance (`locking/Mutex.ts`). The local ESLint rule + `mutex-synchronised-decorator` verifies correct usage, so it will fail lint if + misapplied. +- **`@onError({log, popup})`** — uniform error handling / notifications on an async + method (`utils/onErrorDecorator.ts`). +- **`@logging.withLogContext(Loggers.Extension)`** — attach a logging context + (from `@databricks/sdk-experimental`), optionally with a `@context` parameter. ### Logging & telemetry -- Log through the named loggers in `logger/` (`Loggers.Extension`, …) — **never - `console.log`** (`no-console` is an ESLint error outside tests). -- Define new telemetry events in `telemetry/constants.ts`. User-facing commands are - instrumented automatically by the `telemetry.registerCommand` wrapper (section 6). -- **`EventTypes` in `telemetry/constants.ts` is the only source of truth for an - event's schema.** Each field carries a `comment`; `telemetry.json` is generated - from the class by `scripts/generateTelemetry.ts` and is gitignored. Never - hand-maintain a field list elsewhere — including in prose docs, which go stale - silently while the generated file stays correct. - ---- - -## 4a. Where module documentation goes - -Most code needs no prose doc — a class doc-comment and good names are enough. -When a module genuinely needs one (a design rationale, a privacy posture, a -protocol), write a **`README.md` in the module's folder**, next to the code it -describes. - -- **`README.md`, not a descriptive name.** `src/telemetry/README.md`, not - `src/telemetry/PYTHON_SETUP_TELEMETRY.md`. A file named for one feature inside a - shared folder is undiscoverable — nothing links to it, and the next person adds - a second one rather than extending it. `README.md` is the name tooling and - humans already look for, and GitHub renders it when browsing the folder. -- **Document the _why_, not the _what_.** Schemas, field lists, and signatures - belong in the code (or, for telemetry, in generated output). A doc that restates - them acquires a second source of truth that drifts. Record the decisions and - trade-offs that have nowhere else to live. -- **No top-level `docs/`.** It is gitignored in this repo (local scratch only), so - anything committed must live beside the code or in an existing root file - (`README.md`, `CONTRIBUTING.md`, this file). - -### Why colocation - -Surveyed when this convention was written (August 2026): - -| Project | Practice | -| ----------------------------------- | --------------------------------------------------------------------------------------------------------------- | -| [microsoft/vscode][vsc] | Colocated docs, **no top-level `docs/`**: ~29 architecture docs under `src/` (e.g. `src/vs/sessions/LAYOUT.md`) | -| [facebook/react][react] | Colocated `README.md` per package (`packages/react-reconciler/README.md`) | -| [rust-lang/rust][rust] | Colocated `README.md` per crate, a few lines, pointing at the external dev guide | -| [vscode-pull-request-github][vscpr] | Centralized `documentation/` directory | -| [kubernetes][k8s], [eslint][eslint] | Centralized `docs/` + external enhancement proposals | - -Both colocation and centralization are well-established; there is no universal -answer. We colocate because this repo has no committed `docs/` tree, and we -standardize on `README.md` (React/Rust style) rather than VS Code's -descriptive-filename style because the latter only stays navigable when a folder -is one subsystem — `src/telemetry/` serves every feature. - -**Telemetry specifically:** VS Code documents events _inline in TypeScript_ — -GDPR classification, purpose, and comment in a type beside the emit call — with no -per-event markdown ([`telemetry.instructions.md`][vsctel]). Our `EventTypes` -`comment` fields are the same idea, which is why prose docs must not duplicate -them. - -[vsc]: https://github.com/microsoft/vscode/tree/main/src/vs/sessions -[vsctel]: https://github.com/microsoft/vscode/blob/main/.github/instructions/telemetry.instructions.md -[react]: https://github.com/facebook/react/blob/main/packages/react-reconciler/README.md -[rust]: https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/README.md -[vscpr]: https://github.com/microsoft/vscode-pull-request-github/tree/main/documentation -[k8s]: https://github.com/kubernetes/enhancements -[eslint]: https://github.com/eslint/eslint/tree/main/docs +- Log through the named loggers in `logger/` (`Loggers.Extension`, …) — **never + `console.log`** (`no-console` is an ESLint error outside tests). +- Define new telemetry events in `telemetry/constants.ts`. User-facing commands are + instrumented automatically by the `telemetry.registerCommand` wrapper (section 6). --- @@ -262,12 +201,12 @@ The Databricks SDK (`@databricks/sdk-experimental`) is the biggest unguarded cross-cutting dependency in the codebase — dozens of files import it, and many reach the `WorkspaceClient` / `apiClient` directly. -- **Reach the workspace client through the connection seam** (`ConnectionManager`), - not by constructing your own client in a feature. -- **Never import through deep `/dist/...` paths** (`.../dist/apis/…`, - `.../dist/retries/…`). They're not a stable entry point — import from the package - root. Keeping SDK access behind one seam is also what turns a future SDK migration - into a bounded change instead of a repo-wide edit. +- **Reach the workspace client through the connection seam** (`ConnectionManager`), + not by constructing your own client in a feature. +- **Never import through deep `/dist/...` paths** (`.../dist/apis/…`, + `.../dist/retries/…`). They're not a stable entry point — import from the package + root. Keeping SDK access behind one seam is also what turns a future SDK migration + into a bounded change instead of a repo-wide edit. --- @@ -299,29 +238,29 @@ reach the `WorkspaceClient` / `apiClient` directly. ## 7. Imports & exports -- **Double quotes** for imports (Prettier-enforced). -- Order: external packages first (`vscode`, `@databricks/sdk-experimental`, …), - then local relative imports. -- Use **`import type { … }`** for type-only imports and to break dependency cycles. -- For utility folders, follow the **namespace-barrel** pattern — - `export * as FileUtils from "./fileUtils"` in `index.ts`, consumed as - `import {FileUtils} from "./utils"`. -- For feature barrels, re-export only the intended public surface; avoid blanket - `export *` of internal files. Blanket `export *` barrels hurt navigation — - "go to definition" and grep-for-usage land on the barrel instead of the real file, - adding an indirection hop with no encapsulation benefit. **Note:** most existing - `index.ts` files still use `export *` (10 of 12 today); those should migrate to - selective re-exports (or be dropped where under-used), not be treated as the - pattern to copy. +- **Double quotes** for imports (Prettier-enforced). +- Order: external packages first (`vscode`, `@databricks/sdk-experimental`, …), + then local relative imports. +- Use **`import type { … }`** for type-only imports and to break dependency cycles. +- For utility folders, follow the **namespace-barrel** pattern — + `export * as FileUtils from "./fileUtils"` in `index.ts`, consumed as + `import {FileUtils} from "./utils"`. +- For feature barrels, re-export only the intended public surface; avoid blanket + `export *` of internal files. Blanket `export *` barrels hurt navigation — + "go to definition" and grep-for-usage land on the barrel instead of the real file, + adding an indirection hop with no encapsulation benefit. **Note:** most existing + `index.ts` files still use `export *` (10 of 12 today); those should migrate to + selective re-exports (or be dropped where under-used), not be treated as the + pattern to copy. --- ## 8. Tests -- **Co-locate** unit tests: `X.test.ts` beside `X.ts` (Mocha via - `@vscode/test-electron`). Prefer testing `Model`/`Manager`/util logic — it's the - most testable, since UI is isolated behind `vscode-objs/`. -- Use the right suffix for the right kind of test: +- **Co-locate** unit tests: `X.test.ts` beside `X.ts` (Mocha via + `@vscode/test-electron`). Prefer testing `Model`/`Manager`/util logic — it's the + most testable, since UI is isolated behind `vscode-objs/`. +- Use the right suffix for the right kind of test: | Suffix | Kind | Where | | ------------ | -------------------------------- | -------------------- | @@ -330,7 +269,7 @@ reach the `WorkspaceClient` / `apiClient` directly. | `*.e2e.ts` | end-to-end (WebdriverIO) | `test/e2e/` | | `*_test.py` | Python unit | `test/python/` | -- Mock with `ts-mockito`. Never commit `.only` — `no-only-tests` is an error. +- Mock with `ts-mockito`. Never commit `.only` — `no-only-tests` is an error. --- @@ -339,19 +278,19 @@ reach the `WorkspaceClient` / `apiClient` directly. Formatting is enforced by Prettier + ESLint; `yarn fix` auto-applies most of it. The choices that affect how you write: -- 4-space indentation, double quotes, semicolons required. -- `{a: 1}` — **no** space inside braces (`bracketSpacing: false`). -- `(x) => …` — always parenthesize arrow params. -- Trailing commas where ES5 allows. -- Prefer `===` / `!==` and always use curly braces (`curly`). +- 4-space indentation, double quotes, semicolons required. +- `{a: 1}` — **no** space inside braces (`bracketSpacing: false`). +- `(x) => …` — always parenthesize arrow params. +- Trailing commas where ES5 allows. +- Prefer `===` / `!==` and always use curly braces (`curly`). --- ## Issues -- extension.ts is becoming too big -- Large feature folders mix many role-suffixes (logic / presentation / I/O) flat at - one level, which is hard to navigate — and co-located tests double the file count. - Today `run/` (14 files), `bundle/` (12), `language/` (11), and `ui/unity-catalog/` - (10) sit flat at the top level, across 17 distinct suffixes repo-wide. Needs a - grouping convention (see the section 3 role-suffix rule). +- extension.ts is becoming too big +- Large feature folders mix many role-suffixes (logic / presentation / I/O) flat at + one level, which is hard to navigate — and co-located tests double the file count. + Today `run/` (14 files), `bundle/` (12), `language/` (11), and `ui/unity-catalog/` + (10) sit flat at the top level, across 17 distinct suffixes repo-wide. Needs a + grouping convention (see the section 3 role-suffix rule).