Skip to content

Commit a730571

Browse files
committed
refactor(telemetry): simplify startup telemetry instrumentation
- Bundle activation tracer methods (setAuthState, traceDeploymentInit) into a span-scoped ActivationTracer interface; drop instance-state from ActivationTelemetry - Mirror the same shape for remote.setup: RemoteSetupTracer exposes phase() and is owned by RemoteSetupTelemetry; remote.ts no longer touches Span directly - Build RemoteSetupContext after auth retrieval (no empty-default mutation); group setup args under args; rename baseUrlRaw -> baseUrl - Reuse AuthorityParts from util instead of synthesizing a local alias - Drop redundant post-download recordDownloadedBytes and the BinaryDownloadResult/DownloadResult types; per-chunk progress already records the final value - Adopt createTestTelemetryService helper in commandManager.test.ts
1 parent aff083f commit a730571

6 files changed

Lines changed: 149 additions & 187 deletions

File tree

src/core/cliManager.ts

Lines changed: 19 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -36,16 +36,6 @@ type ResolvedBinary =
3636

3737
type CliDownloadReason = "missing" | "version_mismatch";
3838

39-
interface BinaryDownloadResult {
40-
binPath: string;
41-
downloadedBytes?: number;
42-
}
43-
44-
interface DownloadResult {
45-
status: number;
46-
downloadedBytes: number;
47-
}
48-
4939
export class CliManager {
5040
private readonly binaryLock: BinaryLock;
5141

@@ -243,22 +233,18 @@ export class CliManager {
243233
return await this.telemetry.trace(
244234
"cli.download",
245235
async (span) => {
246-
const recordDownloadedBytes = (downloadedBytes: number): void => {
247-
if (downloadedBytes > 0) {
248-
span.setMeasurement("downloadedBytes", downloadedBytes);
249-
}
250-
};
251-
const downloadResult = await this.performBinaryDownload(
236+
const downloadedBinPath = await this.performBinaryDownload(
252237
restClient,
253238
latestVersion,
254239
downloadBinPath,
255240
progressLogPath,
256-
recordDownloadedBytes,
241+
(downloadedBytes) => {
242+
if (downloadedBytes > 0) {
243+
span.setMeasurement("downloadedBytes", downloadedBytes);
244+
}
245+
},
257246
);
258-
if (downloadResult.downloadedBytes !== undefined) {
259-
recordDownloadedBytes(downloadResult.downloadedBytes);
260-
}
261-
return this.renameToFinalPath(resolved, downloadResult.binPath);
247+
return this.renameToFinalPath(resolved, downloadedBinPath);
262248
},
263249
{ reason: downloadReason },
264250
);
@@ -445,7 +431,7 @@ export class CliManager {
445431
binPath: string,
446432
progressLogPath: string,
447433
recordDownloadedBytes: (downloadedBytes: number) => void,
448-
): Promise<BinaryDownloadResult> {
434+
): Promise<string> {
449435
const cfg = vscode.workspace.getConfiguration("coder");
450436
const tempFile = tempFilePath(binPath, "temp");
451437

@@ -490,7 +476,7 @@ export class CliManager {
490476
};
491477

492478
const client = restClient.getAxiosInstance();
493-
const downloadResult = await this.download(
479+
const status = await this.download(
494480
client,
495481
binSource,
496482
writeStream,
@@ -500,7 +486,7 @@ export class CliManager {
500486
onProgress,
501487
);
502488

503-
switch (downloadResult.status) {
489+
switch (status) {
504490
case 200: {
505491
await downloadProgress.writeProgress(progressLogPath, {
506492
bytesDownloaded: 0,
@@ -531,14 +517,11 @@ export class CliManager {
531517
// Replace existing binary (handles both renames + Windows lock)
532518
await this.replaceExistingBinary(binPath, tempFile);
533519

534-
return {
535-
binPath,
536-
downloadedBytes: downloadResult.downloadedBytes,
537-
};
520+
return binPath;
538521
}
539522
case 304: {
540523
this.output.info("Using existing binary since server returned a 304");
541-
return { binPath };
524+
return binPath;
542525
}
543526
case 404: {
544527
vscode.window
@@ -575,7 +558,7 @@ export class CliManager {
575558
}
576559
const params = new URLSearchParams({
577560
title: `Failed to download binary on \`${cliUtils.goos()}-${cliUtils.goarch()}\``,
578-
body: `Received status code \`${downloadResult.status}\` when downloading the binary.`,
561+
body: `Received status code \`${status}\` when downloading the binary.`,
579562
});
580563
const uri = vscode.Uri.parse(
581564
`https://github.com/coder/vscode-coder/issues/new?${params.toString()}`,
@@ -603,7 +586,7 @@ export class CliManager {
603586
bytesDownloaded: number,
604587
totalBytes: number | null,
605588
) => Promise<void>,
606-
): Promise<DownloadResult> {
589+
): Promise<number> {
607590
const baseUrl = client.defaults.baseURL;
608591

609592
const controller = new AbortController();
@@ -621,8 +604,8 @@ export class CliManager {
621604
});
622605
this.output.info("Got status code", resp.status);
623606

624-
let written = 0;
625607
if (resp.status === 200) {
608+
let written = 0;
626609
const rawContentLength = (resp.headers["content-length"] ??
627610
resp.headers["x-original-content-length"]) as unknown;
628611
const contentLength = Number.parseInt(
@@ -722,10 +705,7 @@ export class CliManager {
722705
this.output.info(`Downloaded ${prettyBytes(written)}`);
723706
}
724707

725-
return {
726-
status: resp.status,
727-
downloadedBytes: written,
728-
};
708+
return resp.status;
729709
}
730710

731711
/**
@@ -815,8 +795,8 @@ export class CliManager {
815795
this.output.info("Downloading signature from", source);
816796
const signaturePath = path.join(cliPath + ".asc");
817797
const writeStream = createWriteStream(signaturePath);
818-
const downloadResult = await this.download(client, source, writeStream);
819-
if (downloadResult.status === 200) {
798+
const status = await this.download(client, source, writeStream);
799+
if (status === 200) {
820800
try {
821801
await pgp.verifySignature(
822802
publicKeys,
@@ -845,7 +825,7 @@ export class CliManager {
845825
this.output.info("Binary will be ran anyway at user request");
846826
}
847827
}
848-
return downloadResult.status;
828+
return status;
849829
}
850830

851831
/**

src/extension.ts

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@ import { ServiceContainer } from "./core/container";
1414
import { DeploymentManager } from "./deployment/deploymentManager";
1515
import { CertificateError } from "./error/certificateError";
1616
import { getErrorDetail, toError } from "./error/errorUtils";
17-
import { ActivationTelemetry } from "./instrumentation/activation";
17+
import {
18+
ActivationTelemetry,
19+
type ActivationTracer,
20+
} from "./instrumentation/activation";
1821
import { OAuthSessionManager } from "./oauth/sessionManager";
1922
import { Remote } from "./remote/remote";
2023
import { getRemoteSshExtension } from "./remote/sshExtension";
@@ -37,15 +40,15 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
3740
serviceContainer.getTelemetryService(),
3841
);
3942

40-
await activationTelemetry.trace(() =>
41-
doActivate(ctx, serviceContainer, activationTelemetry),
43+
await activationTelemetry.trace((tracer) =>
44+
doActivate(ctx, serviceContainer, tracer),
4245
);
4346
}
4447

4548
async function doActivate(
4649
ctx: vscode.ExtensionContext,
4750
serviceContainer: ServiceContainer,
48-
activationTelemetry: ActivationTelemetry,
51+
tracer: ActivationTracer,
4952
): Promise<void> {
5053
// The Remote SSH extension's proposed APIs are used to override the SSH host
5154
// name in VS Code itself. It's visually unappealing having a lengthy name!
@@ -92,9 +95,7 @@ async function doActivate(
9295
const deploymentSessionAuth = deployment
9396
? await secretsManager.getSessionAuth(deployment.safeHostname)
9497
: undefined;
95-
activationTelemetry.setAuthState(
96-
deploymentSessionAuth ? "valid_token" : "none",
97-
);
98+
tracer.setAuthState(deploymentSessionAuth ? "valid_token" : "none");
9899

99100
// Shared handler for auth failures (used by interceptor + session manager)
100101
const handleAuthFailure = (): Promise<void> => {
@@ -396,9 +397,7 @@ async function doActivate(
396397
url: details.url,
397398
token: details.token,
398399
});
399-
activationTelemetry.setAuthState(
400-
deploymentSet ? "valid_token" : "expired",
401-
);
400+
tracer.setAuthState(deploymentSet ? "valid_token" : "expired");
402401

403402
// If a deep link stored a chat agent ID before the
404403
// remote-authority reload, open it now that the
@@ -456,7 +455,7 @@ async function doActivate(
456455
contextManager.set("coder.loaded", true);
457456
} else if (deployment) {
458457
output.info(`Initializing deployment: ${deployment.url}`);
459-
activationTelemetry
458+
tracer
460459
.traceDeploymentInit(() =>
461460
deploymentManager.setDeploymentIfValid(deployment),
462461
)

src/instrumentation/activation.ts

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,34 @@
11
import type { TelemetryService } from "../telemetry/service";
2-
import type { Span } from "../telemetry/span";
32

43
export type ActivationAuthState = "none" | "valid_token" | "expired";
54

6-
export class ActivationTelemetry {
7-
private authState: ActivationAuthState = "none";
8-
private span: Span | undefined;
5+
/** Helpers scoped to the activation trace's lifetime. */
6+
export interface ActivationTracer {
7+
setAuthState(state: ActivationAuthState): void;
8+
traceDeploymentInit(fn: () => Promise<boolean>): Promise<boolean>;
9+
}
910

11+
export class ActivationTelemetry {
1012
public constructor(private readonly telemetry: TelemetryService) {}
1113

12-
public trace<T>(fn: () => Promise<T>): Promise<T> {
14+
public trace<T>(fn: (tracer: ActivationTracer) => Promise<T>): Promise<T> {
1315
return this.telemetry.trace("activation", async (span) => {
14-
this.span = span;
15-
span.setProperty("authState", this.authState);
16-
try {
17-
return await fn();
18-
} finally {
19-
this.span = undefined;
20-
}
21-
});
22-
}
23-
24-
public setAuthState(authState: ActivationAuthState): void {
25-
this.authState = authState;
26-
this.span?.setProperty("authState", authState);
27-
}
28-
29-
public traceDeploymentInit(fn: () => Promise<boolean>): Promise<boolean> {
30-
const initialAuthState = this.authState;
31-
return this.telemetry.trace("activation.deployment_init", async (span) => {
32-
span.setProperty("authState", initialAuthState);
33-
const success = await fn();
34-
span.setProperty("authState", success ? "valid_token" : "expired");
35-
return success;
16+
span.setProperty("authState", "none");
17+
return fn({
18+
setAuthState: (state) => span.setProperty("authState", state),
19+
traceDeploymentInit: (initFn) =>
20+
this.telemetry.trace(
21+
"activation.deployment_init",
22+
async (childSpan) => {
23+
const success = await initFn();
24+
childSpan.setProperty(
25+
"authState",
26+
success ? "valid_token" : "expired",
27+
);
28+
return success;
29+
},
30+
),
31+
});
3632
});
3733
}
3834
}

src/instrumentation/remoteSetup.ts

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Span } from "../telemetry/span";
1+
import type { TelemetryService } from "../telemetry/service";
22

33
export type RemoteSetupPhase =
44
| "auth_retrieval"
@@ -7,13 +7,20 @@ export type RemoteSetupPhase =
77
| "agent_ready"
88
| "ssh_config_write";
99

10+
/** Helpers scoped to the remote.setup trace's lifetime. */
11+
export interface RemoteSetupTracer {
12+
phase<T>(name: RemoteSetupPhase, fn: () => T | PromiseLike<T>): Promise<T>;
13+
}
14+
1015
export class RemoteSetupTelemetry {
11-
public constructor(private readonly span: Span) {}
16+
public constructor(private readonly telemetry: TelemetryService) {}
1217

13-
public phase<T>(
14-
phaseName: RemoteSetupPhase,
15-
fn: () => T | PromiseLike<T>,
16-
): Promise<T> {
17-
return this.span.phase(phaseName, () => Promise.resolve(fn()));
18+
public trace<T>(fn: (tracer: RemoteSetupTracer) => Promise<T>): Promise<T> {
19+
return this.telemetry.trace("remote.setup", (span) =>
20+
fn({
21+
phase: (name, phaseFn) =>
22+
span.phase(name, () => Promise.resolve(phaseFn())),
23+
}),
24+
);
1825
}
1926
}

0 commit comments

Comments
 (0)