Skip to content

Commit 721bceb

Browse files
jaggederestclaude
jaggederest
and
claude
committed
feat: implement centralized logging adapter
Implement a centralized logging system to improve debugging capabilities and separate logging concerns from the Storage class. - Add logger module with configurable debug/info levels - Support runtime configuration via coder.verbose setting - Provide OutputChannelAdapter for VS Code integration - Add ArrayAdapter for isolated unit testing - Include performance benchmarks in tests - Maintain backward compatibility via Storage delegation - Update all components to use centralized logger The logger responds to configuration changes without requiring extension restart and includes source location tracking for debug messages. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
1 parent 6938bc9 commit 721bceb

14 files changed

+802
-152
lines changed

‎src/api.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ export function makeCoderSdk(
105105
restClient.getAxiosInstance().interceptors.response.use(
106106
(r) => r,
107107
async (err) => {
108-
throw await CertificateError.maybeWrap(err, baseUrl, storage);
108+
throw await CertificateError.maybeWrap(err, baseUrl);
109109
},
110110
);
111111

‎src/commands.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import * as vscode from "vscode";
1010
import { makeCoderSdk, needToken } from "./api";
1111
import { extractAgents } from "./api-helper";
1212
import { CertificateError } from "./error";
13+
import { logger } from "./logger";
1314
import { Storage } from "./storage";
1415
import { toRemoteAuthority, toSafeHost } from "./util";
1516
import { OpenableTreeItem } from "./workspacesProvider";
@@ -245,9 +246,7 @@ export class Commands {
245246
} catch (err) {
246247
const message = getErrorMessage(err, "no response from the server");
247248
if (isAutologin) {
248-
this.storage.writeToCoderOutputChannel(
249-
`Failed to log in to Coder server: ${message}`,
250-
);
249+
logger.info(`Failed to log in to Coder server: ${message}`);
251250
} else {
252251
this.vscodeProposed.window.showErrorMessage(
253252
"Failed to log in to Coder server",

‎src/error.test.ts

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,19 @@ const isElectron =
1919
// TODO: Remove the vscode mock once we revert the testing framework.
2020
beforeAll(() => {
2121
vi.mock("vscode", () => {
22-
return {};
22+
return {
23+
workspace: {
24+
getConfiguration: vi.fn().mockReturnValue({
25+
get: vi.fn().mockReturnValue(false),
26+
}),
27+
onDidChangeConfiguration: vi.fn().mockReturnValue({
28+
dispose: vi.fn(),
29+
}),
30+
},
31+
};
2332
});
2433
});
2534

26-
const logger = {
27-
writeToCoderOutputChannel(message: string) {
28-
throw new Error(message);
29-
},
30-
};
31-
3235
const disposers: (() => void)[] = [];
3336
afterAll(() => {
3437
disposers.forEach((d) => d());
@@ -89,7 +92,7 @@ it("detects partial chains", async () => {
8992
try {
9093
await request;
9194
} catch (error) {
92-
const wrapped = await CertificateError.maybeWrap(error, address, logger);
95+
const wrapped = await CertificateError.maybeWrap(error, address);
9396
expect(wrapped instanceof CertificateError).toBeTruthy();
9497
expect((wrapped as CertificateError).x509Err).toBe(X509_ERR.PARTIAL_CHAIN);
9598
}
@@ -126,7 +129,7 @@ it("detects self-signed certificates without signing capability", async () => {
126129
try {
127130
await request;
128131
} catch (error) {
129-
const wrapped = await CertificateError.maybeWrap(error, address, logger);
132+
const wrapped = await CertificateError.maybeWrap(error, address);
130133
expect(wrapped instanceof CertificateError).toBeTruthy();
131134
expect((wrapped as CertificateError).x509Err).toBe(X509_ERR.NON_SIGNING);
132135
}
@@ -157,7 +160,7 @@ it("detects self-signed certificates", async () => {
157160
try {
158161
await request;
159162
} catch (error) {
160-
const wrapped = await CertificateError.maybeWrap(error, address, logger);
163+
const wrapped = await CertificateError.maybeWrap(error, address);
161164
expect(wrapped instanceof CertificateError).toBeTruthy();
162165
expect((wrapped as CertificateError).x509Err).toBe(X509_ERR.UNTRUSTED_LEAF);
163166
}
@@ -200,7 +203,7 @@ it("detects an untrusted chain", async () => {
200203
try {
201204
await request;
202205
} catch (error) {
203-
const wrapped = await CertificateError.maybeWrap(error, address, logger);
206+
const wrapped = await CertificateError.maybeWrap(error, address);
204207
expect(wrapped instanceof CertificateError).toBeTruthy();
205208
expect((wrapped as CertificateError).x509Err).toBe(
206209
X509_ERR.UNTRUSTED_CHAIN,
@@ -247,7 +250,7 @@ it("falls back with different error", async () => {
247250
try {
248251
await request;
249252
} catch (error) {
250-
const wrapped = await CertificateError.maybeWrap(error, "1", logger);
253+
const wrapped = await CertificateError.maybeWrap(error, "1");
251254
expect(wrapped instanceof CertificateError).toBeFalsy();
252255
expect((wrapped as Error).message).toMatch(/failed with status code 500/);
253256
}

‎src/error.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { isApiError, isApiErrorResponse } from "coder/site/src/api/errors";
33
import * as forge from "node-forge";
44
import * as tls from "tls";
55
import * as vscode from "vscode";
6+
import { logger } from "./logger";
67

78
// X509_ERR_CODE represents error codes as returned from BoringSSL/OpenSSL.
89
export enum X509_ERR_CODE {
@@ -21,10 +22,6 @@ export enum X509_ERR {
2122
UNTRUSTED_CHAIN = "Your Coder deployment's certificate chain does not appear to be trusted by this system. The root of the certificate chain must be added to this system's trust store. ",
2223
}
2324

24-
export interface Logger {
25-
writeToCoderOutputChannel(message: string): void;
26-
}
27-
2825
interface KeyUsage {
2926
keyCertSign: boolean;
3027
}
@@ -47,7 +44,6 @@ export class CertificateError extends Error {
4744
static async maybeWrap<T>(
4845
err: T,
4946
address: string,
50-
logger: Logger,
5147
): Promise<CertificateError | T> {
5248
if (isAxiosError(err)) {
5349
switch (err.code) {
@@ -59,7 +55,7 @@ export class CertificateError extends Error {
5955
await CertificateError.determineVerifyErrorCause(address);
6056
return new CertificateError(err.message, cause);
6157
} catch (error) {
62-
logger.writeToCoderOutputChannel(
58+
logger.debug(
6359
`Failed to parse certificate from ${address}: ${error}`,
6460
);
6561
break;

‎src/extension.ts

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { makeCoderSdk, needToken } from "./api";
77
import { errToStr } from "./api-helper";
88
import { Commands } from "./commands";
99
import { CertificateError, getErrorDetail } from "./error";
10+
import { logger } from "./logger";
1011
import { Remote } from "./remote";
1112
import { Storage } from "./storage";
1213
import { toSafeHost } from "./util";
@@ -48,6 +49,10 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
4849
}
4950

5051
const output = vscode.window.createOutputChannel("Coder");
52+
53+
// Initialize logger with the output channel
54+
logger.initialize(output);
55+
5156
const storage = new Storage(
5257
output,
5358
ctx.globalState,
@@ -317,7 +322,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
317322
}
318323
} catch (ex) {
319324
if (ex instanceof CertificateError) {
320-
storage.writeToCoderOutputChannel(ex.x509Err || ex.message);
325+
logger.info(ex.x509Err || ex.message);
321326
await ex.showModal("Failed to open workspace");
322327
} else if (isAxiosError(ex)) {
323328
const msg = getErrorMessage(ex, "None");
@@ -326,7 +331,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
326331
const method = ex.config?.method?.toUpperCase() || "request";
327332
const status = ex.response?.status || "None";
328333
const message = `API ${method} to '${urlString}' failed.\nStatus code: ${status}\nMessage: ${msg}\nDetail: ${detail}`;
329-
storage.writeToCoderOutputChannel(message);
334+
logger.info(message);
330335
await vscodeProposed.window.showErrorMessage(
331336
"Failed to open workspace",
332337
{
@@ -337,7 +342,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
337342
);
338343
} else {
339344
const message = errToStr(ex, "No error message was provided");
340-
storage.writeToCoderOutputChannel(message);
345+
logger.info(message);
341346
await vscodeProposed.window.showErrorMessage(
342347
"Failed to open workspace",
343348
{
@@ -356,14 +361,12 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
356361
// See if the plugin client is authenticated.
357362
const baseUrl = restClient.getAxiosInstance().defaults.baseURL;
358363
if (baseUrl) {
359-
storage.writeToCoderOutputChannel(
360-
`Logged in to ${baseUrl}; checking credentials`,
361-
);
364+
logger.info(`Logged in to ${baseUrl}; checking credentials`);
362365
restClient
363366
.getAuthenticatedUser()
364367
.then(async (user) => {
365368
if (user && user.roles) {
366-
storage.writeToCoderOutputChannel("Credentials are valid");
369+
logger.info("Credentials are valid");
367370
vscode.commands.executeCommand(
368371
"setContext",
369372
"coder.authenticated",
@@ -381,17 +384,13 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
381384
myWorkspacesProvider.fetchAndRefresh();
382385
allWorkspacesProvider.fetchAndRefresh();
383386
} else {
384-
storage.writeToCoderOutputChannel(
385-
`No error, but got unexpected response: ${user}`,
386-
);
387+
logger.info(`No error, but got unexpected response: ${user}`);
387388
}
388389
})
389390
.catch((error) => {
390391
// This should be a failure to make the request, like the header command
391392
// errored.
392-
storage.writeToCoderOutputChannel(
393-
`Failed to check user authentication: ${error.message}`,
394-
);
393+
logger.info(`Failed to check user authentication: ${error.message}`);
395394
vscode.window.showErrorMessage(
396395
`Failed to check user authentication: ${error.message}`,
397396
);
@@ -400,7 +399,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<void> {
400399
vscode.commands.executeCommand("setContext", "coder.loaded", true);
401400
});
402401
} else {
403-
storage.writeToCoderOutputChannel("Not currently logged in");
402+
logger.info("Not currently logged in");
404403
vscode.commands.executeCommand("setContext", "coder.loaded", true);
405404

406405
// Handle autologin, if not already logged in.

‎src/headers.test.ts

Lines changed: 55 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,86 +1,94 @@
11
import * as os from "os";
2-
import { it, expect, describe, beforeEach, afterEach, vi } from "vitest";
2+
import {
3+
it,
4+
expect,
5+
describe,
6+
beforeEach,
7+
afterEach,
8+
vi,
9+
beforeAll,
10+
} from "vitest";
311
import { WorkspaceConfiguration } from "vscode";
412
import { getHeaderCommand, getHeaders } from "./headers";
513

6-
const logger = {
7-
writeToCoderOutputChannel() {
8-
// no-op
9-
},
10-
};
14+
// Mock vscode module before importing anything that uses logger
15+
beforeAll(() => {
16+
vi.mock("vscode", () => {
17+
return {
18+
workspace: {
19+
getConfiguration: vi.fn().mockReturnValue({
20+
get: vi.fn().mockReturnValue(false),
21+
}),
22+
onDidChangeConfiguration: vi.fn().mockReturnValue({
23+
dispose: vi.fn(),
24+
}),
25+
},
26+
};
27+
});
28+
});
1129

1230
it("should return no headers", async () => {
13-
await expect(getHeaders(undefined, undefined, logger)).resolves.toStrictEqual(
14-
{},
15-
);
16-
await expect(
17-
getHeaders("localhost", undefined, logger),
18-
).resolves.toStrictEqual({});
19-
await expect(getHeaders(undefined, "command", logger)).resolves.toStrictEqual(
20-
{},
21-
);
22-
await expect(getHeaders("localhost", "", logger)).resolves.toStrictEqual({});
23-
await expect(getHeaders("", "command", logger)).resolves.toStrictEqual({});
24-
await expect(getHeaders("localhost", " ", logger)).resolves.toStrictEqual(
25-
{},
26-
);
27-
await expect(getHeaders(" ", "command", logger)).resolves.toStrictEqual({});
28-
await expect(
29-
getHeaders("localhost", "printf ''", logger),
30-
).resolves.toStrictEqual({});
31+
await expect(getHeaders(undefined, undefined)).resolves.toStrictEqual({});
32+
await expect(getHeaders("localhost", undefined)).resolves.toStrictEqual({});
33+
await expect(getHeaders(undefined, "command")).resolves.toStrictEqual({});
34+
await expect(getHeaders("localhost", "")).resolves.toStrictEqual({});
35+
await expect(getHeaders("", "command")).resolves.toStrictEqual({});
36+
await expect(getHeaders("localhost", " ")).resolves.toStrictEqual({});
37+
await expect(getHeaders(" ", "command")).resolves.toStrictEqual({});
38+
await expect(getHeaders("localhost", "printf ''")).resolves.toStrictEqual({});
3139
});
3240

3341
it("should return headers", async () => {
3442
await expect(
35-
getHeaders("localhost", "printf 'foo=bar\\nbaz=qux'", logger),
43+
getHeaders("localhost", "printf 'foo=bar\\nbaz=qux'"),
3644
).resolves.toStrictEqual({
3745
foo: "bar",
3846
baz: "qux",
3947
});
4048
await expect(
41-
getHeaders("localhost", "printf 'foo=bar\\r\\nbaz=qux'", logger),
49+
getHeaders("localhost", "printf 'foo=bar\\r\\nbaz=qux'"),
4250
).resolves.toStrictEqual({
4351
foo: "bar",
4452
baz: "qux",
4553
});
4654
await expect(
47-
getHeaders("localhost", "printf 'foo=bar\\r\\n'", logger),
55+
getHeaders("localhost", "printf 'foo=bar\\r\\n'"),
4856
).resolves.toStrictEqual({ foo: "bar" });
4957
await expect(
50-
getHeaders("localhost", "printf 'foo=bar'", logger),
58+
getHeaders("localhost", "printf 'foo=bar'"),
5159
).resolves.toStrictEqual({ foo: "bar" });
5260
await expect(
53-
getHeaders("localhost", "printf 'foo=bar='", logger),
61+
getHeaders("localhost", "printf 'foo=bar='"),
5462
).resolves.toStrictEqual({ foo: "bar=" });
5563
await expect(
56-
getHeaders("localhost", "printf 'foo=bar=baz'", logger),
64+
getHeaders("localhost", "printf 'foo=bar=baz'"),
5765
).resolves.toStrictEqual({ foo: "bar=baz" });
58-
await expect(
59-
getHeaders("localhost", "printf 'foo='", logger),
60-
).resolves.toStrictEqual({ foo: "" });
66+
await expect(getHeaders("localhost", "printf 'foo='")).resolves.toStrictEqual(
67+
{ foo: "" },
68+
);
6169
});
6270

6371
it("should error on malformed or empty lines", async () => {
6472
await expect(
65-
getHeaders("localhost", "printf 'foo=bar\\r\\n\\r\\n'", logger),
66-
).rejects.toMatch(/Malformed/);
67-
await expect(
68-
getHeaders("localhost", "printf '\\r\\nfoo=bar'", logger),
73+
getHeaders("localhost", "printf 'foo=bar\\r\\n\\r\\n'"),
6974
).rejects.toMatch(/Malformed/);
7075
await expect(
71-
getHeaders("localhost", "printf '=foo'", logger),
76+
getHeaders("localhost", "printf '\\r\\nfoo=bar'"),
7277
).rejects.toMatch(/Malformed/);
73-
await expect(getHeaders("localhost", "printf 'foo'", logger)).rejects.toMatch(
78+
await expect(getHeaders("localhost", "printf '=foo'")).rejects.toMatch(
79+
/Malformed/,
80+
);
81+
await expect(getHeaders("localhost", "printf 'foo'")).rejects.toMatch(
82+
/Malformed/,
83+
);
84+
await expect(getHeaders("localhost", "printf ' =foo'")).rejects.toMatch(
85+
/Malformed/,
86+
);
87+
await expect(getHeaders("localhost", "printf 'foo =bar'")).rejects.toMatch(
7488
/Malformed/,
7589
);
7690
await expect(
77-
getHeaders("localhost", "printf ' =foo'", logger),
78-
).rejects.toMatch(/Malformed/);
79-
await expect(
80-
getHeaders("localhost", "printf 'foo =bar'", logger),
81-
).rejects.toMatch(/Malformed/);
82-
await expect(
83-
getHeaders("localhost", "printf 'foo foo=bar'", logger),
91+
getHeaders("localhost", "printf 'foo foo=bar'"),
8492
).rejects.toMatch(/Malformed/);
8593
});
8694

@@ -92,13 +100,12 @@ it("should have access to environment variables", async () => {
92100
os.platform() === "win32"
93101
? "printf url=%CODER_URL%"
94102
: "printf url=$CODER_URL",
95-
logger,
96103
),
97104
).resolves.toStrictEqual({ url: coderUrl });
98105
});
99106

100107
it("should error on non-zero exit", async () => {
101-
await expect(getHeaders("localhost", "exit 10", logger)).rejects.toMatch(
108+
await expect(getHeaders("localhost", "exit 10")).rejects.toMatch(
102109
/exited unexpectedly with code 10/,
103110
);
104111
});

0 commit comments

Comments
 (0)