Skip to content

Commit 5df69b2

Browse files
EhabYEhabY
committed
refactor: align auth telemetry conventions
1 parent 7510021 commit 5df69b2

9 files changed

Lines changed: 121 additions & 106 deletions

File tree

‎src/core/cliCredentialManager.ts‎

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@ import {
1111
CredentialCliError,
1212
CredentialFileError,
1313
CredentialTelemetry,
14-
type CredentialCategory,
14+
type CredentialClearRecorder,
1515
type CredentialClearResult,
16-
type CredentialStoreResult,
16+
type CredentialStoreRecorder,
1717
} from "../instrumentation/credentials";
1818
import { isKeyringEnabled } from "../settings/cli";
1919
import { getHeaderArgs } from "../settings/headers";
@@ -81,26 +81,28 @@ export class CliCredentialManager {
8181
token: string,
8282
configs: Pick<WorkspaceConfiguration, "get">,
8383
options?: { signal?: AbortSignal },
84-
): Promise<CredentialStoreResult> {
85-
return this.credentialTelemetry.traceStore(configs, () =>
86-
this.storeTokenInner(url, token, configs, options),
84+
): Promise<void> {
85+
return this.credentialTelemetry.traceStore(configs, (recorder) =>
86+
this.storeTokenInner(url, token, configs, recorder, options),
8787
);
8888
}
8989

9090
private async storeTokenInner(
9191
url: string,
9292
token: string,
9393
configs: Pick<WorkspaceConfiguration, "get">,
94+
recorder: CredentialStoreRecorder,
9495
options?: { signal?: AbortSignal },
95-
): Promise<CredentialStoreResult> {
96+
): Promise<void> {
9697
const binPath = await this.resolveKeyringBinary(
9798
url,
9899
configs,
99100
"keyringAuth",
100101
);
101102
if (!binPath) {
102103
await this.writeCredentialFiles(url, token);
103-
return { category: "file" };
104+
recorder.setCategory("file");
105+
return;
104106
}
105107

106108
const args = [
@@ -114,8 +116,8 @@ export class CliCredentialManager {
114116
env: { ...process.env, CODER_SESSION_TOKEN: token },
115117
signal: options?.signal,
116118
});
119+
recorder.setCategory("keyring");
117120
this.logger.info("Stored token via CLI for", url);
118-
return { category: "keyring" };
119121
} catch (error) {
120122
this.logger.warn("Failed to store token via CLI:", error);
121123
if (isAbortError(error)) {
@@ -176,27 +178,27 @@ export class CliCredentialManager {
176178
configs: Pick<WorkspaceConfiguration, "get">,
177179
options?: { signal?: AbortSignal },
178180
): Promise<CredentialClearResult> {
179-
return this.credentialTelemetry.traceClear(configs, () =>
180-
this.deleteTokenInner(url, configs, options),
181+
return this.credentialTelemetry.traceClear(configs, (recorder) =>
182+
this.deleteTokenInner(url, configs, recorder, options),
181183
);
182184
}
183185

184186
private async deleteTokenInner(
185187
url: string,
186188
configs: Pick<WorkspaceConfiguration, "get">,
189+
recorder: CredentialClearRecorder,
187190
options?: { signal?: AbortSignal },
188191
): Promise<CredentialClearResult> {
189-
const [filesResult, keyringResult] = await Promise.all([
192+
const [_filesResult, keyringResult] = await Promise.all([
190193
this.deleteCredentialFiles(url),
191194
this.deleteKeyringToken(url, configs, options?.signal),
192195
]);
193-
return {
194-
category:
195-
keyringResult.used || keyringResult.failureCategory
196-
? "keyring"
197-
: filesResult.category,
198-
failureCategory: keyringResult.failureCategory,
199-
};
196+
recorder.setCategory(
197+
keyringResult.used || keyringResult.failureCategory ? "keyring" : "file",
198+
);
199+
return keyringResult.failureCategory
200+
? { failureCategory: keyringResult.failureCategory }
201+
: {};
200202
}
201203

202204
/**
@@ -267,9 +269,7 @@ export class CliCredentialManager {
267269
/**
268270
* Delete URL and token files. Best-effort: never throws.
269271
*/
270-
private async deleteCredentialFiles(
271-
url: string,
272-
): Promise<{ category: CredentialCategory }> {
272+
private async deleteCredentialFiles(url: string): Promise<void> {
273273
const safeHostname = toSafeHost(url);
274274
const paths = [
275275
this.pathResolver.getSessionTokenPath(safeHostname),
@@ -282,7 +282,6 @@ export class CliCredentialManager {
282282
}),
283283
),
284284
);
285-
return { category: "file" };
286285
}
287286

288287
/**

‎src/core/cliManager.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -931,13 +931,10 @@ export class CliManager {
931931
}
932932
if (result.cancelled) {
933933
this.output.info("Credential removal cancelled by user");
934-
return { category: "keyring", failureCategory: "aborted" };
934+
return { failureCategory: "aborted" };
935935
}
936936
this.output.warn("Failed to remove credentials:", result.error);
937-
return {
938-
category: isKeyringEnabled(configs) ? "keyring" : "file",
939-
failureCategory: categorizeCredentialError(result.error),
940-
};
937+
return { failureCategory: categorizeCredentialError(result.error) };
941938
}
942939

943940
private handleStoreError(error: unknown): void {

‎src/instrumentation/credentials.ts‎

Lines changed: 72 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -4,50 +4,51 @@ import { isKeyringEnabled } from "../settings/cli";
44
import type { WorkspaceConfiguration } from "vscode";
55

66
import type { TelemetryReporter } from "../telemetry/reporter";
7+
import type { Span } from "../telemetry/span";
78

89
export type CredentialCategory = "keyring" | "file";
910
export type CredentialFailureCategory = "aborted" | "binary" | "cli" | "file";
1011

11-
export interface CredentialStoreResult {
12-
readonly category: CredentialCategory;
12+
export interface CredentialStoreRecorder {
13+
setCategory(category: CredentialCategory): void;
14+
}
15+
16+
export interface CredentialClearRecorder {
17+
setCategory(category: CredentialCategory): void;
1318
}
1419

1520
export interface CredentialClearResult {
16-
readonly category: CredentialCategory;
1721
readonly failureCategory?: CredentialFailureCategory;
1822
}
1923

2024
export class CredentialTelemetry {
2125
public constructor(private readonly telemetry: TelemetryReporter) {}
2226

23-
public async traceStore<T extends CredentialStoreResult>(
27+
public async traceStore<T>(
2428
configs: Pick<WorkspaceConfiguration, "get">,
25-
fn: () => Promise<T>,
29+
fn: (recorder: CredentialStoreRecorder) => Promise<T>,
2630
): Promise<T> {
27-
const keyringEnabled = isKeyringEnabled(configs);
28-
const defaultCategory: CredentialCategory = keyringEnabled
29-
? "keyring"
30-
: "file";
31+
const defaults = defaultCredentialProperties(configs);
3132
let cancellation: unknown;
3233
const result = await this.telemetry.trace(
33-
"auth.credential_stored",
34+
"auth.credential.store",
3435
async (span) => {
3536
try {
36-
const result = await fn();
37-
span.setProperty("category", result.category);
38-
return result;
37+
return await fn(createCredentialRecorder(span));
3938
} catch (error) {
40-
span.setProperty("category", defaultCategory);
41-
span.setProperty("failureCategory", categorizeCredentialError(error));
39+
recordCredentialFailure(span, defaults.category, error);
4240
if (isAbortError(error)) {
4341
span.markAborted();
4442
cancellation = error;
45-
return { category: defaultCategory } as T;
43+
return undefined as T;
4644
}
4745
throw error;
4846
}
4947
},
50-
{ keyringEnabled, category: defaultCategory },
48+
{
49+
keyring_enabled: defaults.keyringEnabled,
50+
category: defaults.category,
51+
},
5152
);
5253
if (cancellation instanceof Error) {
5354
throw cancellation;
@@ -57,43 +58,31 @@ export class CredentialTelemetry {
5758

5859
public async traceClear<T extends CredentialClearResult>(
5960
configs: Pick<WorkspaceConfiguration, "get">,
60-
fn: () => Promise<T>,
61+
fn: (recorder: CredentialClearRecorder) => Promise<T>,
6162
): Promise<T> {
62-
const keyringEnabled = isKeyringEnabled(configs);
63-
const defaultCategory: CredentialCategory = keyringEnabled
64-
? "keyring"
65-
: "file";
63+
const defaults = defaultCredentialProperties(configs);
6664
let cancellation: unknown;
6765
const result = await this.telemetry.trace(
68-
"auth.credential_cleared",
66+
"auth.credential.clear",
6967
async (span) => {
7068
try {
71-
const result = await fn();
72-
span.setProperty("category", result.category);
73-
if (result.failureCategory) {
74-
span.setProperty("failureCategory", result.failureCategory);
75-
if (result.failureCategory === "aborted") {
76-
span.markAborted();
77-
} else {
78-
span.markFailure();
79-
}
80-
}
69+
const result = await fn(createCredentialRecorder(span));
70+
recordClearFailure(span, result.failureCategory);
8171
return result;
8272
} catch (error) {
83-
span.setProperty("category", defaultCategory);
84-
span.setProperty("failureCategory", categorizeCredentialError(error));
73+
recordCredentialFailure(span, defaults.category, error);
8574
if (isAbortError(error)) {
8675
span.markAborted();
8776
cancellation = error;
88-
return {
89-
category: defaultCategory,
90-
failureCategory: "aborted",
91-
} as T;
77+
return { failureCategory: "aborted" } as T;
9278
}
9379
throw error;
9480
}
9581
},
96-
{ keyringEnabled, category: defaultCategory },
82+
{
83+
keyring_enabled: defaults.keyringEnabled,
84+
category: defaults.category,
85+
},
9786
);
9887
if (cancellation instanceof Error) {
9988
throw cancellation;
@@ -102,6 +91,49 @@ export class CredentialTelemetry {
10291
}
10392
}
10493

94+
function defaultCredentialProperties(
95+
configs: Pick<WorkspaceConfiguration, "get">,
96+
): { keyringEnabled: boolean; category: CredentialCategory } {
97+
const keyringEnabled = isKeyringEnabled(configs);
98+
return {
99+
keyringEnabled,
100+
category: keyringEnabled ? "keyring" : "file",
101+
};
102+
}
103+
104+
function createCredentialRecorder(
105+
span: Span,
106+
): CredentialStoreRecorder & CredentialClearRecorder {
107+
return {
108+
setCategory: (category) => span.setProperty("category", category),
109+
};
110+
}
111+
112+
function recordCredentialFailure(
113+
span: Span,
114+
category: CredentialCategory,
115+
error: unknown,
116+
): void {
117+
span.setProperty("category", category);
118+
span.setProperty("failure_category", categorizeCredentialError(error));
119+
}
120+
121+
function recordClearFailure(
122+
span: Span,
123+
failureCategory: CredentialClearResult["failureCategory"],
124+
): void {
125+
if (!failureCategory) {
126+
return;
127+
}
128+
129+
span.setProperty("failure_category", failureCategory);
130+
if (failureCategory === "aborted") {
131+
span.markAborted();
132+
return;
133+
}
134+
span.markFailure();
135+
}
136+
105137
export function categorizeCredentialError(
106138
error: unknown,
107139
): CredentialFailureCategory {

‎src/instrumentation/deployment.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ export class DeploymentTelemetry {
2222
}
2323

2424
public crossWindowDetected(): void {
25-
this.telemetry.log("deployment.cross_window_detected");
25+
this.telemetry.log("deployment.cross_window.detected");
2626
}
2727

2828
public authConfigRecoveryFailed(): void {
29-
this.telemetry.log("deployment.auth_config_recovery_failed");
29+
this.telemetry.log("deployment.auth_config.recovery_failed");
3030
}
3131
}

‎test/mocks/testHelpers.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -444,9 +444,9 @@ export class InMemorySecretStorage implements vscode.SecretStorage {
444444

445445
export function createMockCliCredentialManager(): CliCredentialManager {
446446
return {
447-
storeToken: vi.fn().mockResolvedValue({ category: "file" }),
447+
storeToken: vi.fn().mockResolvedValue(undefined),
448448
readToken: vi.fn().mockResolvedValue(undefined),
449-
deleteToken: vi.fn().mockResolvedValue({ category: "file" }),
449+
deleteToken: vi.fn().mockResolvedValue({}),
450450
} as unknown as CliCredentialManager;
451451
}
452452

‎test/unit/commands.test.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ function createCommandsHarness(options: CommandsHarnessOptions = {}) {
106106

107107
const cliManager = {
108108
clearCredentials: vi.fn(() =>
109-
Promise.resolve(options.credentialResult ?? { category: "file" }),
109+
Promise.resolve(options.credentialResult ?? {}),
110110
),
111111
};
112112

@@ -311,7 +311,6 @@ describe("Commands", () => {
311311
options: {
312312
authenticated: true,
313313
credentialResult: {
314-
category: "keyring",
315314
failureCategory: "aborted",
316315
},
317316
},
@@ -331,7 +330,6 @@ describe("Commands", () => {
331330
options: {
332331
authenticated: true,
333332
credentialResult: {
334-
category: "keyring",
335333
failureCategory: "cli",
336334
},
337335
},

0 commit comments

Comments
 (0)