From 80e71af30c9375f2c4a8e013ea26ce46b1bef324 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 10:48:48 +0200 Subject: [PATCH 01/15] fix --- package-lock.json | 35 +++++---------- package.json | 2 +- src/index.ts | 4 ++ src/logger.ts | 1 + src/server.ts | 5 ++- src/telemetry/constants.ts | 2 - src/telemetry/eventCache.ts | 2 +- src/telemetry/telemetry.ts | 82 +++++++++++++++++++++++++++++++++--- src/telemetry/types.ts | 1 - tests/integration/helpers.ts | 5 +++ tests/unit/telemetry.test.ts | 10 ++++- 11 files changed, 109 insertions(+), 40 deletions(-) diff --git a/package-lock.json b/package-lock.json index 98bf1bd3..9a6dd662 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,7 +18,7 @@ "mongodb-log-writer": "^2.4.1", "mongodb-redact": "^1.1.6", "mongodb-schema": "^12.6.2", - "native-machine-id": "^0.1.0", + "node-machine-id": "1.1.12", "openapi-fetch": "^0.13.5", "simple-oauth2": "^5.1.0", "yargs-parser": "^21.1.1", @@ -6271,6 +6271,7 @@ "resolved": "https://registry.npmjs.org/bindings/-/bindings-1.5.0.tgz", "integrity": "sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==", "license": "MIT", + "optional": true, "dependencies": { "file-uri-to-path": "1.0.0" } @@ -8533,7 +8534,8 @@ "version": "1.0.0", "resolved": "https://registry.npmjs.org/file-uri-to-path/-/file-uri-to-path-1.0.0.tgz", "integrity": "sha512-0Zt+s3L7Vf1biwWZ29aARiVYLx7iMGnEUl9x33fbB/j3jR81u/O2LbqK+Bm1CDSNDKVtJ/YjwY7TUd5SkeLQLw==", - "license": "MIT" + "license": "MIT", + "optional": true }, "node_modules/filelist": { "version": "1.0.4", @@ -11225,29 +11227,6 @@ "license": "MIT", "optional": true }, - "node_modules/native-machine-id": { - "version": "0.1.0", - "resolved": "https://registry.npmjs.org/native-machine-id/-/native-machine-id-0.1.0.tgz", - "integrity": "sha512-Po7OPcXGsWZ/o+n93ZOhmF3G5RQsEUMTnVddX45u5GfoEnk803ba7lhztwMkDaPhUFHy5FpXLiytIFitVxMkTA==", - "hasInstallScript": true, - "license": "Apache-2.0", - "dependencies": { - "bindings": "^1.5.0", - "node-addon-api": "^8.0.0" - }, - "bin": { - "native-machine-id": "dist/bin/machine-id.js" - } - }, - "node_modules/native-machine-id/node_modules/node-addon-api": { - "version": "8.3.1", - "resolved": "https://registry.npmjs.org/node-addon-api/-/node-addon-api-8.3.1.tgz", - "integrity": "sha512-lytcDEdxKjGJPTLEfW4mYMigRezMlyJY8W4wxJK8zE533Jlb8L8dRuObJFWg2P+AuOIxoCgKF+2Oq4d4Zd0OUA==", - "license": "MIT", - "engines": { - "node": "^18 || ^20 || >= 21" - } - }, "node_modules/natural-compare": { "version": "1.4.0", "resolved": "https://registry.npmjs.org/natural-compare/-/natural-compare-1.4.0.tgz", @@ -11358,6 +11337,12 @@ "dev": true, "license": "MIT" }, + "node_modules/node-machine-id": { + "version": "1.1.12", + "resolved": "https://registry.npmjs.org/node-machine-id/-/node-machine-id-1.1.12.tgz", + "integrity": "sha512-QNABxbrPa3qEIfrE6GOJ7BYIuignnJw7iQ2YPbc3Nla1HzRJjXzZOiikfF8m7eAMfichLt3M4VgLOetqgDmgGQ==", + "license": "MIT" + }, "node_modules/node-readfiles": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/node-readfiles/-/node-readfiles-0.2.0.tgz", diff --git a/package.json b/package.json index a0090a40..125b8e03 100644 --- a/package.json +++ b/package.json @@ -69,7 +69,7 @@ "mongodb-log-writer": "^2.4.1", "mongodb-redact": "^1.1.6", "mongodb-schema": "^12.6.2", - "native-machine-id": "^0.1.0", + "node-machine-id": "1.1.12", "openapi-fetch": "^0.13.5", "simple-oauth2": "^5.1.0", "yargs-parser": "^21.1.1", diff --git a/src/index.ts b/src/index.ts index 9ab92038..972ffb63 100644 --- a/src/index.ts +++ b/src/index.ts @@ -7,6 +7,7 @@ import { config } from "./config.js"; import { Session } from "./session.js"; import { Server } from "./server.js"; import { packageInfo } from "./packageInfo.js"; +import { Telemetry } from "./telemetry/telemetry.js"; try { const session = new Session({ @@ -19,9 +20,12 @@ try { version: packageInfo.version, }); + const telemetry = Telemetry.create(session); + const server = new Server({ mcpServer, session, + telemetry, userConfig: config, }); diff --git a/src/logger.ts b/src/logger.ts index 534bfb80..f9a3a752 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -17,6 +17,7 @@ export const LogId = { telemetryEmitFailure: mongoLogId(1_002_002), telemetryEmitStart: mongoLogId(1_002_003), telemetryEmitSuccess: mongoLogId(1_002_004), + telemetryMachineIdFailure: mongoLogId(1_002_005), toolExecute: mongoLogId(1_003_001), toolExecuteFailure: mongoLogId(1_003_002), diff --git a/src/server.ts b/src/server.ts index b11ba31d..95d8ca1f 100644 --- a/src/server.ts +++ b/src/server.ts @@ -16,6 +16,7 @@ export interface ServerOptions { session: Session; userConfig: UserConfig; mcpServer: McpServer; + telemetry: Telemetry; } export class Server { @@ -25,10 +26,10 @@ export class Server { public readonly userConfig: UserConfig; private readonly startTime: number; - constructor({ session, mcpServer, userConfig }: ServerOptions) { + constructor({ session, mcpServer, userConfig, telemetry }: ServerOptions) { this.startTime = Date.now(); this.session = session; - this.telemetry = new Telemetry(session); + this.telemetry = telemetry; this.mcpServer = mcpServer; this.userConfig = userConfig; } diff --git a/src/telemetry/constants.ts b/src/telemetry/constants.ts index 7ae139b5..67a13686 100644 --- a/src/telemetry/constants.ts +++ b/src/telemetry/constants.ts @@ -1,11 +1,9 @@ -import { getMachineIdSync } from "native-machine-id"; import { packageInfo } from "../packageInfo.js"; import { type CommonStaticProperties } from "./types.js"; /** * Machine-specific metadata formatted for telemetry */ export const MACHINE_METADATA: CommonStaticProperties = { - device_id: getMachineIdSync(), mcp_server_version: packageInfo.version, mcp_server_name: packageInfo.mcpServerName, platform: process.platform, diff --git a/src/telemetry/eventCache.ts b/src/telemetry/eventCache.ts index 141e9b78..26fc1f82 100644 --- a/src/telemetry/eventCache.ts +++ b/src/telemetry/eventCache.ts @@ -1,5 +1,5 @@ -import { BaseEvent } from "./types.js"; import { LRUCache } from "lru-cache"; +import { BaseEvent } from "./types.js"; /** * Singleton class for in-memory telemetry event caching diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 53431232..0e2f8f9b 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -5,6 +5,8 @@ import logger, { LogId } from "../logger.js"; import { ApiClient } from "../common/atlas/apiClient.js"; import { MACHINE_METADATA } from "./constants.js"; import { EventCache } from "./eventCache.js"; +import { createHmac } from "crypto"; +import { machineId } from "node-machine-id"; type EventResult = { success: boolean; @@ -12,15 +14,76 @@ type EventResult = { }; export class Telemetry { - private readonly commonProperties: CommonProperties; + private isBufferingEvents: boolean = true; + private resolveDeviceId: (deviceId: string) => void = () => {}; - constructor( + private constructor( private readonly session: Session, - private readonly eventCache: EventCache = EventCache.getInstance() - ) { - this.commonProperties = { - ...MACHINE_METADATA, - }; + private readonly commonProperties: CommonProperties, + private readonly eventCache: EventCache + ) {} + + static create( + session: Session, + commonProperties: CommonProperties = MACHINE_METADATA, + eventCache: EventCache = EventCache.getInstance() + ): Telemetry { + const instance = new Telemetry(session, commonProperties, eventCache); + + void instance.start(); + return instance; + } + + private async start(): Promise { + this.commonProperties.device_id = await this.getDeviceId(); + + this.isBufferingEvents = false; + await this.emitEvents(this.eventCache.getEvents()); + } + + public async close(): Promise { + this.resolveDeviceId("unknown"); + this.isBufferingEvents = false; + await this.emitEvents(this.eventCache.getEvents()); + } + + private async machineIdWithTimeout(): Promise { + try { + return Promise.race([ + machineId(true), + new Promise((resolve, reject) => { + this.resolveDeviceId = resolve; + setTimeout(() => { + reject(new Error("Timeout getting machine ID")); + }, 3000); + }), + ]); + } catch (error) { + logger.debug(LogId.telemetryMachineIdFailure, "telemetry", `Error getting machine ID: ${String(error)}`); + return "unknown"; + } + } + + /** + * @returns A hashed, unique identifier for the running device or `undefined` if not known. + */ + private async getDeviceId(): Promise { + if (this.commonProperties.device_id) { + return this.commonProperties.device_id; + } + + // Create a hashed format from the all uppercase version of the machine ID + // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. + + const originalId = (await this.machineIdWithTimeout()).toUpperCase(); + + const hmac = createHmac("sha256", originalId); + + /** This matches the message used to create the hashes in Atlas CLI */ + const DEVICE_ID_HASH_MESSAGE = "atlascli"; + + hmac.update(DEVICE_ID_HASH_MESSAGE); + return hmac.digest("hex"); } /** @@ -84,6 +147,11 @@ export class Telemetry { * Falls back to caching if both attempts fail */ private async emit(events: BaseEvent[]): Promise { + if (this.isBufferingEvents) { + this.eventCache.appendEvents(events); + return; + } + const cachedEvents = this.eventCache.getEvents(); const allEvents = [...cachedEvents, ...events]; diff --git a/src/telemetry/types.ts b/src/telemetry/types.ts index 76e1d4ae..d77cc010 100644 --- a/src/telemetry/types.ts +++ b/src/telemetry/types.ts @@ -53,7 +53,6 @@ export type ServerEvent = TelemetryEvent; * Interface for static properties, they can be fetched once and reused. */ export type CommonStaticProperties = { - device_id?: string; mcp_server_version: string; mcp_server_name: string; platform: string; diff --git a/tests/integration/helpers.ts b/tests/integration/helpers.ts index c57deda8..b89be4ae 100644 --- a/tests/integration/helpers.ts +++ b/tests/integration/helpers.ts @@ -5,6 +5,7 @@ import { UserConfig } from "../../src/config.js"; import { McpError } from "@modelcontextprotocol/sdk/types.js"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { Session } from "../../src/session.js"; +import { Telemetry } from "../../src/telemetry/telemetry.js"; interface ParameterInfo { name: string; @@ -52,9 +53,13 @@ export function setupIntegrationTest(getUserConfig: () => UserConfig): Integrati }); userConfig.telemetry = "disabled"; + + const telemetry = Telemetry.create(session); + mcpServer = new Server({ session, userConfig, + telemetry, mcpServer: new McpServer({ name: "test-server", version: "1.2.3", diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 5b37da8e..c6724397 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -4,6 +4,7 @@ import { Telemetry } from "../../src/telemetry/telemetry.js"; import { BaseEvent, TelemetryResult } from "../../src/telemetry/types.js"; import { EventCache } from "../../src/telemetry/eventCache.js"; import { config } from "../../src/config.js"; +import { MACHINE_METADATA } from "../../src/telemetry/constants.js"; // Mock the ApiClient to avoid real API calls jest.mock("../../src/common/atlas/apiClient.js"); @@ -113,7 +114,14 @@ describe("Telemetry", () => { } as unknown as Session; // Create the telemetry instance with mocked dependencies - telemetry = new Telemetry(session, mockEventCache); + telemetry = Telemetry.create( + session, + { + ...MACHINE_METADATA, + device_id: "test-device-id", + }, + mockEventCache + ); config.telemetry = "enabled"; }); From 9022a615697aa13f4550a53eba6622fa859d9a69 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 13:29:56 +0200 Subject: [PATCH 02/15] fix: use non-native dependency and defer machine ID resolution --- src/deferred-promise.ts | 56 +++++++ src/telemetry/telemetry.ts | 60 ++++---- tests/integration/telemetry.test.ts | 27 ++++ tests/unit/deferred-promise.test.ts | 61 ++++++++ tests/unit/telemetry.test.ts | 221 ++++++++++++++++++++-------- 5 files changed, 328 insertions(+), 97 deletions(-) create mode 100644 src/deferred-promise.ts create mode 100644 tests/integration/telemetry.test.ts create mode 100644 tests/unit/deferred-promise.test.ts diff --git a/src/deferred-promise.ts b/src/deferred-promise.ts new file mode 100644 index 00000000..cae56e3d --- /dev/null +++ b/src/deferred-promise.ts @@ -0,0 +1,56 @@ +type DeferredPromiseOptions = { + timeout?: number; +}; + +/** Creates a promise and exposes its resolve and reject methods, with an optional timeout. */ +export class DeferredPromise extends Promise { + resolve!: (value: T) => void; + reject!: (reason: unknown) => void; + private timeoutId?: NodeJS.Timeout; + + constructor(resolver: (resolve: (value: T) => void, reject: (reason: Error) => void) => void, timeout?: number) { + let resolveFn: (value: T) => void; + let rejectFn: (reason?: unknown) => void; + + super((resolve, reject) => { + resolveFn = resolve; + rejectFn = reject; + }); + + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + this.resolve = resolveFn!; + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + this.reject = rejectFn!; + + if (timeout !== undefined) { + this.timeoutId = setTimeout(() => { + this.reject(new Error("Promise timed out")); + }, timeout); + } + + if (resolver) { + resolver( + (value: T) => { + if (this.timeoutId) clearTimeout(this.timeoutId); + this.resolve(value); + }, + (reason: Error) => { + if (this.timeoutId) clearTimeout(this.timeoutId); + this.reject(reason); + } + ); + } + } + + static fromPromise(promise: Promise, options: DeferredPromiseOptions = {}): DeferredPromise { + return new DeferredPromise((resolve, reject) => { + promise + .then((value) => { + resolve(value); + }) + .catch((reason) => { + reject(reason as Error); + }); + }, options.timeout); + } +} diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 0e2f8f9b..fee7980e 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -7,15 +7,19 @@ import { MACHINE_METADATA } from "./constants.js"; import { EventCache } from "./eventCache.js"; import { createHmac } from "crypto"; import { machineId } from "node-machine-id"; +import { DeferredPromise } from "../deferred-promise.js"; type EventResult = { success: boolean; error?: Error; }; +export const DEVICE_ID_TIMEOUT = 3000; + export class Telemetry { private isBufferingEvents: boolean = true; - private resolveDeviceId: (deviceId: string) => void = () => {}; + /** Resolves when the device ID is retrieved or timeout occurs */ + public deviceIdPromise: DeferredPromise | undefined; private constructor( private readonly session: Session, @@ -25,7 +29,7 @@ export class Telemetry { static create( session: Session, - commonProperties: CommonProperties = MACHINE_METADATA, + commonProperties: CommonProperties = { ...MACHINE_METADATA }, eventCache: EventCache = EventCache.getInstance() ): Telemetry { const instance = new Telemetry(session, commonProperties, eventCache); @@ -35,55 +39,42 @@ export class Telemetry { } private async start(): Promise { - this.commonProperties.device_id = await this.getDeviceId(); + this.deviceIdPromise = DeferredPromise.fromPromise(this.getDeviceId()); + this.commonProperties.device_id = await this.deviceIdPromise; this.isBufferingEvents = false; - await this.emitEvents(this.eventCache.getEvents()); } public async close(): Promise { - this.resolveDeviceId("unknown"); + this.deviceIdPromise?.resolve("unknown"); this.isBufferingEvents = false; await this.emitEvents(this.eventCache.getEvents()); } - private async machineIdWithTimeout(): Promise { - try { - return Promise.race([ - machineId(true), - new Promise((resolve, reject) => { - this.resolveDeviceId = resolve; - setTimeout(() => { - reject(new Error("Timeout getting machine ID")); - }, 3000); - }), - ]); - } catch (error) { - logger.debug(LogId.telemetryMachineIdFailure, "telemetry", `Error getting machine ID: ${String(error)}`); - return "unknown"; - } - } - /** * @returns A hashed, unique identifier for the running device or `undefined` if not known. */ private async getDeviceId(): Promise { - if (this.commonProperties.device_id) { - return this.commonProperties.device_id; - } - - // Create a hashed format from the all uppercase version of the machine ID - // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. + try { + if (this.commonProperties.device_id) { + return this.commonProperties.device_id; + } - const originalId = (await this.machineIdWithTimeout()).toUpperCase(); + const originalId = await DeferredPromise.fromPromise(machineId(true), { timeout: DEVICE_ID_TIMEOUT }); - const hmac = createHmac("sha256", originalId); + // Create a hashed format from the all uppercase version of the machine ID + // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. + const hmac = createHmac("sha256", originalId.toUpperCase()); - /** This matches the message used to create the hashes in Atlas CLI */ - const DEVICE_ID_HASH_MESSAGE = "atlascli"; + /** This matches the message used to create the hashes in Atlas CLI */ + const DEVICE_ID_HASH_MESSAGE = "atlascli"; - hmac.update(DEVICE_ID_HASH_MESSAGE); - return hmac.digest("hex"); + hmac.update(DEVICE_ID_HASH_MESSAGE); + return hmac.digest("hex"); + } catch (error) { + logger.debug(LogId.telemetryMachineIdFailure, "telemetry", String(error)); + return "unknown"; + } } /** @@ -134,6 +125,7 @@ export class Telemetry { public getCommonProperties(): CommonProperties { return { ...this.commonProperties, + device_id: this.commonProperties.device_id, mcp_client_version: this.session.agentRunner?.version, mcp_client_name: this.session.agentRunner?.name, session_id: this.session.sessionId, diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts new file mode 100644 index 00000000..4cdbd399 --- /dev/null +++ b/tests/integration/telemetry.test.ts @@ -0,0 +1,27 @@ +import { createHmac } from "crypto"; +import { machineId } from "node-machine-id"; +import { Telemetry } from "../../src/telemetry/telemetry.js"; +import { Session } from "../../src/session.js"; + +describe("Telemetry", () => { + it("should resolve the actual machine ID", async () => { + const actualId = await machineId(true); + // Should be a UUID + expect(actualId).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i); + const actualHashedId = createHmac("sha256", actualId.toUpperCase()).update("atlascli").digest("hex"); + + const telemetry = Telemetry.create( + new Session({ + apiBaseUrl: "", + }) + ); + + expect(telemetry.getCommonProperties().device_id).toBe(undefined); + expect(telemetry["isBufferingEvents"]).toBe(true); + + await telemetry.deviceIdPromise; + + expect(telemetry.getCommonProperties().device_id).toBe(actualHashedId); + expect(telemetry["isBufferingEvents"]).toBe(false); + }); +}); diff --git a/tests/unit/deferred-promise.test.ts b/tests/unit/deferred-promise.test.ts new file mode 100644 index 00000000..fe6ce12c --- /dev/null +++ b/tests/unit/deferred-promise.test.ts @@ -0,0 +1,61 @@ +import { DeferredPromise } from "../../src/deferred-promise.js"; + +describe("DeferredPromise", () => { + it("should resolve with the correct value", async () => { + const deferred = new DeferredPromise((resolve) => { + resolve("resolved value"); + }); + + await expect(deferred).resolves.toEqual("resolved value"); + }); + + it("should reject with the correct error", async () => { + const deferred = new DeferredPromise((_, reject) => { + reject(new Error("rejected error")); + }); + + await expect(deferred).rejects.toThrow("rejected error"); + }); + + it("should timeout if not resolved or rejected within the specified time", async () => { + const deferred = new DeferredPromise(() => { + // Do not resolve or reject + }, 10); + + await expect(deferred).rejects.toThrow("Promise timed out"); + }); + + it("should clear the timeout when resolved", async () => { + jest.useFakeTimers(); + + const deferred = new DeferredPromise((resolve) => { + setTimeout(() => resolve("resolved value"), 100); + }, 200); + + const promise = deferred.then((value) => { + expect(value).toBe("resolved value"); + }); + + jest.advanceTimersByTime(100); + await promise; + + jest.useRealTimers(); + }); + + it("should clear the timeout when rejected", async () => { + jest.useFakeTimers(); + + const deferred = new DeferredPromise((_, reject) => { + setTimeout(() => reject(new Error("rejected error")), 100); + }, 200); + + const promise = deferred.catch((error) => { + expect(error).toEqual(new Error("rejected error")); + }); + + jest.advanceTimersByTime(100); + await promise; + + jest.useRealTimers(); + }); +}); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index c6724397..27f58430 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -1,6 +1,6 @@ import { ApiClient } from "../../src/common/atlas/apiClient.js"; import { Session } from "../../src/session.js"; -import { Telemetry } from "../../src/telemetry/telemetry.js"; +import { DEVICE_ID_TIMEOUT, Telemetry } from "../../src/telemetry/telemetry.js"; import { BaseEvent, TelemetryResult } from "../../src/telemetry/types.js"; import { EventCache } from "../../src/telemetry/eventCache.js"; import { config } from "../../src/config.js"; @@ -14,6 +14,15 @@ const MockApiClient = ApiClient as jest.MockedClass; jest.mock("../../src/telemetry/eventCache.js"); const MockEventCache = EventCache as jest.MockedClass; +// Mock node-machine-id to simulate machine ID resolution +jest.mock("node-machine-id", () => ({ + machineId: jest.fn(), +})); + +import * as nodeMachineId from "node-machine-id"; +import { createHmac } from "crypto"; +import logger, { LogId } from "../../src/logger.js"; + describe("Telemetry", () => { let mockApiClient: jest.Mocked; let mockEventCache: jest.Mocked; @@ -118,7 +127,6 @@ describe("Telemetry", () => { session, { ...MACHINE_METADATA, - device_id: "test-device-id", }, mockEventCache ); @@ -126,104 +134,191 @@ describe("Telemetry", () => { config.telemetry = "enabled"; }); - describe("when telemetry is enabled", () => { - it("should send events successfully", async () => { - const testEvent = createTestEvent(); + describe("sending events", () => { + beforeEach(() => { + (nodeMachineId.machineId as jest.Mock).mockResolvedValue("test-machine-id"); + }); + + describe("when telemetry is enabled", () => { + it("should send events successfully", async () => { + const testEvent = createTestEvent(); + + await telemetry.emitEvents([testEvent]); + + verifyMockCalls({ + sendEventsCalls: 1, + clearEventsCalls: 1, + sendEventsCalledWith: [testEvent], + }); + }); + + it("should cache events when sending fails", async () => { + mockApiClient.sendEvents.mockRejectedValueOnce(new Error("API error")); + + const testEvent = createTestEvent(); + + await telemetry.emitEvents([testEvent]); - await telemetry.emitEvents([testEvent]); + verifyMockCalls({ + sendEventsCalls: 1, + appendEventsCalls: 1, + appendEventsCalledWith: [testEvent], + }); + }); + + it("should include cached events when sending", async () => { + const cachedEvent = createTestEvent({ + command: "cached-command", + component: "cached-component", + }); + + const newEvent = createTestEvent({ + command: "new-command", + component: "new-component", + }); - verifyMockCalls({ - sendEventsCalls: 1, - clearEventsCalls: 1, - sendEventsCalledWith: [testEvent], + // Set up mock to return cached events + mockEventCache.getEvents.mockReturnValueOnce([cachedEvent]); + + await telemetry.emitEvents([newEvent]); + + verifyMockCalls({ + sendEventsCalls: 1, + clearEventsCalls: 1, + sendEventsCalledWith: [cachedEvent, newEvent], + }); }); }); - it("should cache events when sending fails", async () => { - mockApiClient.sendEvents.mockRejectedValueOnce(new Error("API error")); + describe("when telemetry is disabled", () => { + beforeEach(() => { + config.telemetry = "disabled"; + }); - const testEvent = createTestEvent(); + it("should not send events", async () => { + const testEvent = createTestEvent(); - await telemetry.emitEvents([testEvent]); + await telemetry.emitEvents([testEvent]); - verifyMockCalls({ - sendEventsCalls: 1, - appendEventsCalls: 1, - appendEventsCalledWith: [testEvent], + verifyMockCalls(); }); }); - it("should include cached events when sending", async () => { - const cachedEvent = createTestEvent({ - command: "cached-command", - component: "cached-component", + it("should correctly add common properties to events", () => { + const commonProps = telemetry.getCommonProperties(); + + // Use explicit type assertion + const expectedProps: Record = { + mcp_client_version: "1.0.0", + mcp_client_name: "test-agent", + session_id: "test-session-id", + config_atlas_auth: "true", + config_connection_string: expect.any(String) as unknown as string, + }; + + expect(commonProps).toMatchObject(expectedProps); + }); + + describe("when DO_NOT_TRACK environment variable is set", () => { + let originalEnv: string | undefined; + + beforeEach(() => { + originalEnv = process.env.DO_NOT_TRACK; + process.env.DO_NOT_TRACK = "1"; }); - const newEvent = createTestEvent({ - command: "new-command", - component: "new-component", + afterEach(() => { + process.env.DO_NOT_TRACK = originalEnv; }); - // Set up mock to return cached events - mockEventCache.getEvents.mockReturnValueOnce([cachedEvent]); + it("should not send events", async () => { + const testEvent = createTestEvent(); - await telemetry.emitEvents([newEvent]); + await telemetry.emitEvents([testEvent]); - verifyMockCalls({ - sendEventsCalls: 1, - clearEventsCalls: 1, - sendEventsCalledWith: [cachedEvent, newEvent], + verifyMockCalls(); }); }); }); - describe("when telemetry is disabled", () => { + describe("machine ID resolution", () => { + const machineId = "test-machine-id"; + const hashedMachineId = createHmac("sha256", machineId.toUpperCase()).update("atlascli").digest("hex"); + beforeEach(() => { - config.telemetry = "disabled"; + jest.useFakeTimers(); + jest.clearAllMocks(); + }); + + afterEach(() => { + jest.useRealTimers(); + jest.clearAllMocks(); }); - it("should not send events", async () => { - const testEvent = createTestEvent(); + it("should successfully resolve the machine ID", async () => { + (nodeMachineId.machineId as jest.Mock).mockResolvedValue(machineId); + telemetry = Telemetry.create(session); + + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - await telemetry.emitEvents([testEvent]); + await telemetry.deviceIdPromise; - verifyMockCalls(); + expect(telemetry["isBufferingEvents"]).toBe(false); + expect(telemetry.getCommonProperties().device_id).toBe(hashedMachineId); }); - }); - it("should correctly add common properties to events", () => { - const commonProps = telemetry.getCommonProperties(); + it("should handle machine ID resolution failure", async () => { + const loggerSpy = jest.spyOn(logger, "debug"); - // Use explicit type assertion - const expectedProps: Record = { - mcp_client_version: "1.0.0", - mcp_client_name: "test-agent", - session_id: "test-session-id", - config_atlas_auth: "true", - config_connection_string: expect.any(String) as unknown as string, - }; + (nodeMachineId.machineId as jest.Mock).mockRejectedValue(new Error("Failed to get device ID")); - expect(commonProps).toMatchObject(expectedProps); - }); + telemetry = Telemetry.create(session); - describe("when DO_NOT_TRACK environment variable is set", () => { - let originalEnv: string | undefined; + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - beforeEach(() => { - originalEnv = process.env.DO_NOT_TRACK; - process.env.DO_NOT_TRACK = "1"; - }); + await telemetry.deviceIdPromise; - afterEach(() => { - process.env.DO_NOT_TRACK = originalEnv; + expect(telemetry["isBufferingEvents"]).toBe(false); + expect(telemetry.getCommonProperties().device_id).toBe("unknown"); + + expect(loggerSpy).toHaveBeenCalledWith( + LogId.telemetryMachineIdFailure, + "telemetry", + "Error: Failed to get device ID" + ); }); - it("should not send events", async () => { - const testEvent = createTestEvent(); + it("should timeout if machine ID resolution takes too long", async () => { + const loggerSpy = jest.spyOn(logger, "debug"); + + (nodeMachineId.machineId as jest.Mock).mockImplementation(() => { + return new Promise(() => {}); + }); + + telemetry = Telemetry.create(session); - await telemetry.emitEvents([testEvent]); + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - verifyMockCalls(); + jest.advanceTimersByTime(DEVICE_ID_TIMEOUT / 2); + + // Make sure the timeout doesn't happen prematurely. + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); + + jest.advanceTimersByTime(DEVICE_ID_TIMEOUT); + + await telemetry.deviceIdPromise; + + expect(telemetry.getCommonProperties().device_id).toBe("unknown"); + expect(telemetry["isBufferingEvents"]).toBe(false); + expect(loggerSpy).toHaveBeenCalledWith( + LogId.telemetryMachineIdFailure, + "telemetry", + "Error: Promise timed out" + ); }); }); }); From 2dfb0a1167730bdf2f8f169494f4014828b87368 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 13:49:49 +0200 Subject: [PATCH 03/15] fix: always handle process rejections --- src/logger.ts | 2 +- src/telemetry/telemetry.ts | 9 +++++++-- tests/unit/telemetry.test.ts | 4 ++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/logger.ts b/src/logger.ts index f9a3a752..52443e86 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -17,7 +17,7 @@ export const LogId = { telemetryEmitFailure: mongoLogId(1_002_002), telemetryEmitStart: mongoLogId(1_002_003), telemetryEmitSuccess: mongoLogId(1_002_004), - telemetryMachineIdFailure: mongoLogId(1_002_005), + telemetryDeviceIdFailure: mongoLogId(1_002_005), toolExecute: mongoLogId(1_003_001), toolExecuteFailure: mongoLogId(1_003_002), diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index fee7980e..c5307d1f 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -40,7 +40,12 @@ export class Telemetry { private async start(): Promise { this.deviceIdPromise = DeferredPromise.fromPromise(this.getDeviceId()); - this.commonProperties.device_id = await this.deviceIdPromise; + try { + this.commonProperties.device_id = await this.deviceIdPromise; + } catch (error) { + logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error)); + this.commonProperties.device_id = "unknown"; + } this.isBufferingEvents = false; } @@ -72,7 +77,7 @@ export class Telemetry { hmac.update(DEVICE_ID_HASH_MESSAGE); return hmac.digest("hex"); } catch (error) { - logger.debug(LogId.telemetryMachineIdFailure, "telemetry", String(error)); + logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error)); return "unknown"; } } diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 27f58430..3682b861 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -284,7 +284,7 @@ describe("Telemetry", () => { expect(telemetry.getCommonProperties().device_id).toBe("unknown"); expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryMachineIdFailure, + LogId.telemetryDeviceIdFailure, "telemetry", "Error: Failed to get device ID" ); @@ -315,7 +315,7 @@ describe("Telemetry", () => { expect(telemetry.getCommonProperties().device_id).toBe("unknown"); expect(telemetry["isBufferingEvents"]).toBe(false); expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryMachineIdFailure, + LogId.telemetryDeviceIdFailure, "telemetry", "Error: Promise timed out" ); From ab2909e412b7164c8d08695e3453642aa69b249e Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 13:54:17 +0200 Subject: [PATCH 04/15] fix: close telemetry --- src/server.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/server.ts b/src/server.ts index 95d8ca1f..c0fc060d 100644 --- a/src/server.ts +++ b/src/server.ts @@ -94,6 +94,7 @@ export class Server { } async close(): Promise { + await this.telemetry.close(); await this.session.close(); await this.mcpServer.close(); } From 79c8d16464a1a2eb57dae7e91c79975d120555b5 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 14:12:41 +0200 Subject: [PATCH 05/15] fix: remove UUID check --- tests/integration/telemetry.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index 4cdbd399..8e3a5da6 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -6,8 +6,6 @@ import { Session } from "../../src/session.js"; describe("Telemetry", () => { it("should resolve the actual machine ID", async () => { const actualId = await machineId(true); - // Should be a UUID - expect(actualId).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i); const actualHashedId = createHmac("sha256", actualId.toUpperCase()).update("atlascli").digest("hex"); const telemetry = Telemetry.create( From cbb905c0cdd60fcbed931ccc15ac3e0314756614 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 14:54:07 +0200 Subject: [PATCH 06/15] fix: resolve instead of reject in case of timeout --- src/deferred-promise.ts | 19 ++++++++----- src/logger.ts | 1 + src/telemetry/telemetry.ts | 17 +++++------ tests/unit/deferred-promise.test.ts | 44 ++++++++++++++++++----------- tests/unit/telemetry.test.ts | 4 +-- 5 files changed, 51 insertions(+), 34 deletions(-) diff --git a/src/deferred-promise.ts b/src/deferred-promise.ts index cae56e3d..4e66adab 100644 --- a/src/deferred-promise.ts +++ b/src/deferred-promise.ts @@ -1,14 +1,19 @@ -type DeferredPromiseOptions = { +type DeferredPromiseOptions = { timeout?: number; + onTimeout?: (resolve: (value: T) => void, reject: (reason: Error) => void) => void; }; /** Creates a promise and exposes its resolve and reject methods, with an optional timeout. */ +// eslint-disable-next-line @typescript-eslint/no-empty-object-type export class DeferredPromise extends Promise { resolve!: (value: T) => void; reject!: (reason: unknown) => void; private timeoutId?: NodeJS.Timeout; - constructor(resolver: (resolve: (value: T) => void, reject: (reason: Error) => void) => void, timeout?: number) { + constructor( + executor: (resolve: (value: T) => void, reject: (reason: Error) => void) => void, + { timeout, onTimeout }: DeferredPromiseOptions = {} + ) { let resolveFn: (value: T) => void; let rejectFn: (reason?: unknown) => void; @@ -24,12 +29,12 @@ export class DeferredPromise extends Promise { if (timeout !== undefined) { this.timeoutId = setTimeout(() => { - this.reject(new Error("Promise timed out")); + onTimeout?.(this.resolve, this.reject); }, timeout); } - if (resolver) { - resolver( + if (executor) { + executor( (value: T) => { if (this.timeoutId) clearTimeout(this.timeoutId); this.resolve(value); @@ -42,7 +47,7 @@ export class DeferredPromise extends Promise { } } - static fromPromise(promise: Promise, options: DeferredPromiseOptions = {}): DeferredPromise { + static fromPromise(promise: Promise, options: DeferredPromiseOptions = {}): DeferredPromise { return new DeferredPromise((resolve, reject) => { promise .then((value) => { @@ -51,6 +56,6 @@ export class DeferredPromise extends Promise { .catch((reason) => { reject(reason as Error); }); - }, options.timeout); + }, options); } } diff --git a/src/logger.ts b/src/logger.ts index 52443e86..7dc5de5b 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -18,6 +18,7 @@ export const LogId = { telemetryEmitStart: mongoLogId(1_002_003), telemetryEmitSuccess: mongoLogId(1_002_004), telemetryDeviceIdFailure: mongoLogId(1_002_005), + telemetryDeviceIdTimeout: mongoLogId(1_002_006), toolExecute: mongoLogId(1_003_001), toolExecuteFailure: mongoLogId(1_003_002), diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index c5307d1f..55b78b48 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -39,13 +39,14 @@ export class Telemetry { } private async start(): Promise { - this.deviceIdPromise = DeferredPromise.fromPromise(this.getDeviceId()); - try { - this.commonProperties.device_id = await this.deviceIdPromise; - } catch (error) { - logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error)); - this.commonProperties.device_id = "unknown"; - } + this.deviceIdPromise = DeferredPromise.fromPromise(this.getDeviceId(), { + timeout: DEVICE_ID_TIMEOUT, + onTimeout: (resolve) => { + resolve("unknown"); + logger.debug(LogId.telemetryDeviceIdTimeout, "telemetry", "Device ID retrieval timed out"); + }, + }); + this.commonProperties.device_id = await this.deviceIdPromise; this.isBufferingEvents = false; } @@ -65,7 +66,7 @@ export class Telemetry { return this.commonProperties.device_id; } - const originalId = await DeferredPromise.fromPromise(machineId(true), { timeout: DEVICE_ID_TIMEOUT }); + const originalId = await machineId(true); // Create a hashed format from the all uppercase version of the machine ID // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. diff --git a/tests/unit/deferred-promise.test.ts b/tests/unit/deferred-promise.test.ts index fe6ce12c..e57f3d84 100644 --- a/tests/unit/deferred-promise.test.ts +++ b/tests/unit/deferred-promise.test.ts @@ -1,6 +1,13 @@ import { DeferredPromise } from "../../src/deferred-promise.js"; describe("DeferredPromise", () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); + }); + it("should resolve with the correct value", async () => { const deferred = new DeferredPromise((resolve) => { resolve("resolved value"); @@ -18,19 +25,25 @@ describe("DeferredPromise", () => { }); it("should timeout if not resolved or rejected within the specified time", async () => { - const deferred = new DeferredPromise(() => { - // Do not resolve or reject - }, 10); + const deferred = new DeferredPromise( + () => { + // Do not resolve or reject + }, + { timeout: 100, onTimeout: (resolve, reject) => reject(new Error("Promise timed out")) } + ); + + jest.advanceTimersByTime(100); await expect(deferred).rejects.toThrow("Promise timed out"); }); it("should clear the timeout when resolved", async () => { - jest.useFakeTimers(); - - const deferred = new DeferredPromise((resolve) => { - setTimeout(() => resolve("resolved value"), 100); - }, 200); + const deferred = new DeferredPromise( + (resolve) => { + setTimeout(() => resolve("resolved value"), 100); + }, + { timeout: 200 } + ); const promise = deferred.then((value) => { expect(value).toBe("resolved value"); @@ -38,16 +51,15 @@ describe("DeferredPromise", () => { jest.advanceTimersByTime(100); await promise; - - jest.useRealTimers(); }); it("should clear the timeout when rejected", async () => { - jest.useFakeTimers(); - - const deferred = new DeferredPromise((_, reject) => { - setTimeout(() => reject(new Error("rejected error")), 100); - }, 200); + const deferred = new DeferredPromise( + (_, reject) => { + setTimeout(() => reject(new Error("rejected error")), 100); + }, + { timeout: 200, onTimeout: (resolve, reject) => reject(new Error("Promise timed out")) } + ); const promise = deferred.catch((error) => { expect(error).toEqual(new Error("rejected error")); @@ -55,7 +67,5 @@ describe("DeferredPromise", () => { jest.advanceTimersByTime(100); await promise; - - jest.useRealTimers(); }); }); diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 3682b861..1a0715c8 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -315,9 +315,9 @@ describe("Telemetry", () => { expect(telemetry.getCommonProperties().device_id).toBe("unknown"); expect(telemetry["isBufferingEvents"]).toBe(false); expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, + LogId.telemetryDeviceIdTimeout, "telemetry", - "Error: Promise timed out" + "Device ID retrieval timed out" ); }); }); From 9be3e12d579ee976f2d4d1aa9bd403e56a77f4c4 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 14:56:10 +0200 Subject: [PATCH 07/15] fix: remove ESLint warnings --- src/deferred-promise.ts | 1 - tests/integration/tools/mongodb/mongodbHelpers.ts | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/deferred-promise.ts b/src/deferred-promise.ts index 4e66adab..e1900ea3 100644 --- a/src/deferred-promise.ts +++ b/src/deferred-promise.ts @@ -4,7 +4,6 @@ type DeferredPromiseOptions = { }; /** Creates a promise and exposes its resolve and reject methods, with an optional timeout. */ -// eslint-disable-next-line @typescript-eslint/no-empty-object-type export class DeferredPromise extends Promise { resolve!: (value: T) => void; reject!: (reason: unknown) => void; diff --git a/tests/integration/tools/mongodb/mongodbHelpers.ts b/tests/integration/tools/mongodb/mongodbHelpers.ts index 2b4ea6a0..96dd7bff 100644 --- a/tests/integration/tools/mongodb/mongodbHelpers.ts +++ b/tests/integration/tools/mongodb/mongodbHelpers.ts @@ -73,8 +73,6 @@ export function setupMongoDBIntegrationTest(): MongoDBIntegrationTest { let dbsDir = path.join(tmpDir, "mongodb-runner", "dbs"); for (let i = 0; i < 10; i++) { try { - // TODO: Fix this type once mongodb-runner is updated. - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call mongoCluster = await MongoCluster.start({ tmpDir: dbsDir, logDir: path.join(tmpDir, "mongodb-runner", "logs"), From 5348a3a764031fa7e462b20315133ea97456cbd8 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 15:48:18 +0200 Subject: [PATCH 08/15] fix: use npm with inspect --- .github/workflows/code_health.yaml | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/.github/workflows/code_health.yaml b/.github/workflows/code_health.yaml index 46e95044..46256d17 100644 --- a/.github/workflows/code_health.yaml +++ b/.github/workflows/code_health.yaml @@ -74,13 +74,8 @@ jobs: node-version-file: package.json cache: "npm" - name: Install dependencies & build - run: npm ci - - name: Remove dev dependencies - run: | - rm -rf node_modules - npm pkg set scripts.prepare="exit 0" - npm install --omit=dev - - run: npx -y @modelcontextprotocol/inspector --cli --method tools/list -- node dist/index.js --connectionString "mongodb://localhost" + run: npm ci --omit=dev --include=@modelcontextprotocol/inspector + - run: npm run inspect -- --cli --method tools/list --connectionString "mongodb://localhost" coverage: name: Report Coverage From c869d639659e7d7f79a6544488bf165413505592 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 16:25:11 +0200 Subject: [PATCH 09/15] fix: remove jest types --- .github/workflows/code_health.yaml | 9 +++++++-- src/telemetry/telemetry.ts | 7 +++++-- tsconfig.build.json | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.github/workflows/code_health.yaml b/.github/workflows/code_health.yaml index 46256d17..46e95044 100644 --- a/.github/workflows/code_health.yaml +++ b/.github/workflows/code_health.yaml @@ -74,8 +74,13 @@ jobs: node-version-file: package.json cache: "npm" - name: Install dependencies & build - run: npm ci --omit=dev --include=@modelcontextprotocol/inspector - - run: npm run inspect -- --cli --method tools/list --connectionString "mongodb://localhost" + run: npm ci + - name: Remove dev dependencies + run: | + rm -rf node_modules + npm pkg set scripts.prepare="exit 0" + npm install --omit=dev + - run: npx -y @modelcontextprotocol/inspector --cli --method tools/list -- node dist/index.js --connectionString "mongodb://localhost" coverage: name: Report Coverage diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 55b78b48..9f0f66d2 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -6,7 +6,7 @@ import { ApiClient } from "../common/atlas/apiClient.js"; import { MACHINE_METADATA } from "./constants.js"; import { EventCache } from "./eventCache.js"; import { createHmac } from "crypto"; -import { machineId } from "node-machine-id"; +import nodeMachineId from "node-machine-id"; import { DeferredPromise } from "../deferred-promise.js"; type EventResult = { @@ -39,6 +39,9 @@ export class Telemetry { } private async start(): Promise { + if (!Telemetry.isTelemetryEnabled()) { + return; + } this.deviceIdPromise = DeferredPromise.fromPromise(this.getDeviceId(), { timeout: DEVICE_ID_TIMEOUT, onTimeout: (resolve) => { @@ -66,7 +69,7 @@ export class Telemetry { return this.commonProperties.device_id; } - const originalId = await machineId(true); + const originalId = await nodeMachineId.machineId(true); // Create a hashed format from the all uppercase version of the machine ID // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. diff --git a/tsconfig.build.json b/tsconfig.build.json index dd65f91d..1fe57f10 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -8,7 +8,7 @@ "strict": true, "strictNullChecks": true, "esModuleInterop": true, - "types": ["node", "jest"], + "types": ["node"], "sourceMap": true, "skipLibCheck": true, "resolveJsonModule": true, From e0339a452e96887499178d61449f60e46f4c5b36 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 17:52:17 +0200 Subject: [PATCH 10/15] fix: use proper ESM with jest We did not have a full ESM configuration for jest and therefore had a discrepancy between our build vs test. This discrepancy leads to issues with CommonJS modules. --- eslint.config.js | 2 +- jest.config.js => jest.config.ts | 2 +- package.json | 2 +- tests/integration/tools/mongodb/mongodbHelpers.ts | 3 +++ tests/unit/telemetry.test.ts | 12 ++++++++++-- tsconfig.jest.json | 2 -- 6 files changed, 16 insertions(+), 7 deletions(-) rename jest.config.js => jest.config.ts (88%) diff --git a/eslint.config.js b/eslint.config.js index c46b8bd4..b42518a5 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -48,7 +48,7 @@ export default defineConfig([ "coverage", "global.d.ts", "eslint.config.js", - "jest.config.js", + "jest.config.ts", ]), eslintPluginPrettierRecommended, ]); diff --git a/jest.config.js b/jest.config.ts similarity index 88% rename from jest.config.js rename to jest.config.ts index 5b06aaed..7fb7ce67 100644 --- a/jest.config.js +++ b/jest.config.ts @@ -12,7 +12,7 @@ export default { "ts-jest", { useESM: true, - tsconfig: "tsconfig.jest.json", // Use specific tsconfig file for Jest + tsconfig: "tsconfig.jest.json", }, ], }, diff --git a/package.json b/package.json index a0090a40..2fc7b2e8 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "check:types": "tsc --noEmit --project tsconfig.json", "reformat": "prettier --write .", "generate": "./scripts/generate.sh", - "test": "jest --coverage" + "test": "node --experimental-vm-modules ./node_modules/.bin/jest --coverage" }, "license": "Apache-2.0", "devDependencies": { diff --git a/tests/integration/tools/mongodb/mongodbHelpers.ts b/tests/integration/tools/mongodb/mongodbHelpers.ts index 087d675c..39ae86fa 100644 --- a/tests/integration/tools/mongodb/mongodbHelpers.ts +++ b/tests/integration/tools/mongodb/mongodbHelpers.ts @@ -1,10 +1,13 @@ import { MongoCluster } from "mongodb-runner"; import path from "path"; +import { fileURLToPath } from "url"; import fs from "fs/promises"; import { MongoClient, ObjectId } from "mongodb"; import { getResponseContent, IntegrationTest, setupIntegrationTest, defaultTestConfig } from "../../helpers.js"; import { UserConfig } from "../../../../src/config.js"; +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + interface MongoDBIntegrationTest { mongoClient: () => MongoClient; connectionString: () => string; diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 4165b60c..bdb06326 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -4,6 +4,7 @@ import { Telemetry } from "../../src/telemetry/telemetry.js"; import { BaseEvent, TelemetryResult } from "../../src/telemetry/types.js"; import { EventCache } from "../../src/telemetry/eventCache.js"; import { config } from "../../src/config.js"; +import { jest } from "@jest/globals"; // Mock the ApiClient to avoid real API calls jest.mock("../../src/common/atlas/apiClient.js"); @@ -93,14 +94,19 @@ describe("Telemetry", () => { // Setup mocked API client mockApiClient = new MockApiClient({ baseUrl: "" }) as jest.Mocked; - mockApiClient.sendEvents = jest.fn().mockResolvedValue(undefined); - mockApiClient.hasCredentials = jest.fn().mockReturnValue(true); + //@ts-expect-error This is a workaround + mockApiClient.sendEvents = jest.fn<() => undefined>().mockResolvedValue(undefined); + mockApiClient.hasCredentials = jest.fn<() => boolean>().mockReturnValue(true); // Setup mocked EventCache mockEventCache = new MockEventCache() as jest.Mocked; + //@ts-expect-error This is a workaround mockEventCache.getEvents = jest.fn().mockReturnValue([]); + //@ts-expect-error This is a workaround mockEventCache.clearEvents = jest.fn().mockResolvedValue(undefined); + //@ts-expect-error This is a workaround mockEventCache.appendEvents = jest.fn().mockResolvedValue(undefined); + //@ts-expect-error This is a workaround MockEventCache.getInstance = jest.fn().mockReturnValue(mockEventCache); // Create a simplified session with our mocked API client @@ -108,7 +114,9 @@ describe("Telemetry", () => { apiClient: mockApiClient, sessionId: "test-session-id", agentRunner: { name: "test-agent", version: "1.0.0" } as const, + //@ts-expect-error This is a workaround close: jest.fn().mockResolvedValue(undefined), + //@ts-expect-error This is a workaround setAgentRunner: jest.fn().mockResolvedValue(undefined), } as unknown as Session; diff --git a/tsconfig.jest.json b/tsconfig.jest.json index ad44307b..e3a80ccc 100644 --- a/tsconfig.jest.json +++ b/tsconfig.jest.json @@ -1,8 +1,6 @@ { "extends": "./tsconfig.build.json", "compilerOptions": { - "module": "esnext", - "target": "esnext", "isolatedModules": true, "allowSyntheticDefaultImports": true, "types": ["jest", "jest-extended"] From 189e97d1780c0c0f636c2e8ffd42b3f06dee126a Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 18:08:27 +0200 Subject: [PATCH 11/15] wip --- package.json | 2 +- src/index.ts | 2 +- src/telemetry/telemetry.ts | 3 ++- tests/integration/telemetry.test.ts | 9 ++++--- tests/unit/deferred-promise.test.ts | 1 + tests/unit/telemetry.test.ts | 37 +++++++++++++++++------------ 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index debfee4b..6affd77b 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,7 @@ "check:types": "tsc --noEmit --project tsconfig.json", "reformat": "prettier --write .", "generate": "./scripts/generate.sh", - "test": "node --experimental-vm-modules ./node_modules/.bin/jest --coverage" + "test": "node --experimental-vm-modules node_modules/jest/bin/jest.js --coverage" }, "license": "Apache-2.0", "devDependencies": { diff --git a/src/index.ts b/src/index.ts index 972ffb63..20a60e53 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,7 +20,7 @@ try { version: packageInfo.version, }); - const telemetry = Telemetry.create(session); + const telemetry = Telemetry.create(session, config); const server = new Server({ mcpServer, diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index b95a2661..9210b928 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -71,7 +71,8 @@ export class Telemetry { return this.commonProperties.device_id; } - const originalId = await nodeMachineId.machineId(true); + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access + const originalId: string = await nodeMachineId.machineId(true); // Create a hashed format from the all uppercase version of the machine ID // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index 8e3a5da6..8e68a697 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -1,17 +1,20 @@ import { createHmac } from "crypto"; -import { machineId } from "node-machine-id"; +import nodeMachineId from "node-machine-id"; import { Telemetry } from "../../src/telemetry/telemetry.js"; import { Session } from "../../src/session.js"; +import { config } from "../../src/config.js"; describe("Telemetry", () => { it("should resolve the actual machine ID", async () => { - const actualId = await machineId(true); + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call + const actualId: string = await nodeMachineId.machineId(true); const actualHashedId = createHmac("sha256", actualId.toUpperCase()).update("atlascli").digest("hex"); const telemetry = Telemetry.create( new Session({ apiBaseUrl: "", - }) + }), + config ); expect(telemetry.getCommonProperties().device_id).toBe(undefined); diff --git a/tests/unit/deferred-promise.test.ts b/tests/unit/deferred-promise.test.ts index e57f3d84..c6011af1 100644 --- a/tests/unit/deferred-promise.test.ts +++ b/tests/unit/deferred-promise.test.ts @@ -1,4 +1,5 @@ import { DeferredPromise } from "../../src/deferred-promise.js"; +import { jest } from "@jest/globals"; describe("DeferredPromise", () => { beforeEach(() => { diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index 5c0eea52..dfb9e6d6 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -1,3 +1,8 @@ +// Mock node-machine-id to simulate machine ID resolution +jest.mock("node-machine-id", () => ({ + machineId: jest.fn(), +})); + import { ApiClient } from "../../src/common/atlas/apiClient.js"; import { Session } from "../../src/session.js"; import { DEVICE_ID_TIMEOUT, Telemetry } from "../../src/telemetry/telemetry.js"; @@ -6,6 +11,9 @@ import { EventCache } from "../../src/telemetry/eventCache.js"; import { config } from "../../src/config.js"; import { MACHINE_METADATA } from "../../src/telemetry/constants.js"; import { jest } from "@jest/globals"; +import { createHmac } from "crypto"; +import logger, { LogId } from "../../src/logger.js"; +import nodeMachineId from "node-machine-id"; // Mock the ApiClient to avoid real API calls jest.mock("../../src/common/atlas/apiClient.js"); @@ -15,14 +23,7 @@ const MockApiClient = ApiClient as jest.MockedClass; jest.mock("../../src/telemetry/eventCache.js"); const MockEventCache = EventCache as jest.MockedClass; -// Mock node-machine-id to simulate machine ID resolution -jest.mock("node-machine-id", () => ({ - machineId: jest.fn(), -})); - -import * as nodeMachineId from "node-machine-id"; -import { createHmac } from "crypto"; -import logger, { LogId } from "../../src/logger.js"; +const mockMachineId = jest.spyOn(nodeMachineId, "machineId"); describe("Telemetry", () => { let mockApiClient: jest.Mocked; @@ -145,7 +146,9 @@ describe("Telemetry", () => { describe("sending events", () => { beforeEach(() => { - (nodeMachineId.machineId as jest.Mock).mockResolvedValue("test-machine-id"); + // @ts-expect-error This is a workaround + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + mockMachineId.mockResolvedValue("test-machine-id"); }); describe("when telemetry is enabled", () => { @@ -265,8 +268,9 @@ describe("Telemetry", () => { }); it("should successfully resolve the machine ID", async () => { - (nodeMachineId.machineId as jest.Mock).mockResolvedValue(machineId); - telemetry = Telemetry.create(session); + // @ts-expect-error This is a workaround + mockMachineId.mockResolvedValue(machineId); + telemetry = Telemetry.create(session, config); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -280,9 +284,11 @@ describe("Telemetry", () => { it("should handle machine ID resolution failure", async () => { const loggerSpy = jest.spyOn(logger, "debug"); - (nodeMachineId.machineId as jest.Mock).mockRejectedValue(new Error("Failed to get device ID")); + // @ts-expect-error This is a workaround + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + mockMachineId.mockRejectedValue(new Error("Failed to get device ID")); - telemetry = Telemetry.create(session); + telemetry = Telemetry.create(session, config); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); @@ -302,11 +308,12 @@ describe("Telemetry", () => { it("should timeout if machine ID resolution takes too long", async () => { const loggerSpy = jest.spyOn(logger, "debug"); - (nodeMachineId.machineId as jest.Mock).mockImplementation(() => { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + mockMachineId.mockImplementation(() => { return new Promise(() => {}); }); - telemetry = Telemetry.create(session); + telemetry = Telemetry.create(session, config); expect(telemetry["isBufferingEvents"]).toBe(true); expect(telemetry.getCommonProperties().device_id).toBe(undefined); From 8575d9a14308f7bdc8e072dccd901bcd7fba2446 Mon Sep 17 00:00:00 2001 From: gagik Date: Tue, 29 Apr 2025 19:31:33 +0200 Subject: [PATCH 12/15] fix: adjust mocks to fit ESM --- src/telemetry/telemetry.ts | 27 +++- tests/integration/telemetry.test.ts | 5 +- tests/unit/telemetry.test.ts | 226 +++++++++++++--------------- 3 files changed, 125 insertions(+), 133 deletions(-) diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index 9210b928..aea161f4 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -20,21 +20,35 @@ export class Telemetry { private isBufferingEvents: boolean = true; /** Resolves when the device ID is retrieved or timeout occurs */ public deviceIdPromise: DeferredPromise | undefined; + private eventCache: EventCache; + private getRawMachineId: () => Promise; private constructor( private readonly session: Session, private readonly userConfig: UserConfig, private readonly commonProperties: CommonProperties, - private readonly eventCache: EventCache - ) {} + { eventCache, getRawMachineId }: { eventCache: EventCache; getRawMachineId: () => Promise } + ) { + this.eventCache = eventCache; + this.getRawMachineId = getRawMachineId; + } static create( session: Session, userConfig: UserConfig, - commonProperties: CommonProperties = { ...MACHINE_METADATA }, - eventCache: EventCache = EventCache.getInstance() + { + commonProperties = { ...MACHINE_METADATA }, + eventCache = EventCache.getInstance(), + + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access + getRawMachineId = () => nodeMachineId.machineId(true), + }: { + eventCache?: EventCache; + getRawMachineId?: () => Promise; + commonProperties?: CommonProperties; + } = {} ): Telemetry { - const instance = new Telemetry(session, userConfig, commonProperties, eventCache); + const instance = new Telemetry(session, userConfig, commonProperties, { eventCache, getRawMachineId }); void instance.start(); return instance; @@ -71,8 +85,7 @@ export class Telemetry { return this.commonProperties.device_id; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access - const originalId: string = await nodeMachineId.machineId(true); + const originalId: string = await this.getRawMachineId(); // Create a hashed format from the all uppercase version of the machine ID // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. diff --git a/tests/integration/telemetry.test.ts b/tests/integration/telemetry.test.ts index 8e68a697..fe8e51ff 100644 --- a/tests/integration/telemetry.test.ts +++ b/tests/integration/telemetry.test.ts @@ -1,13 +1,14 @@ import { createHmac } from "crypto"; -import nodeMachineId from "node-machine-id"; import { Telemetry } from "../../src/telemetry/telemetry.js"; import { Session } from "../../src/session.js"; import { config } from "../../src/config.js"; +import nodeMachineId from "node-machine-id"; describe("Telemetry", () => { it("should resolve the actual machine ID", async () => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access const actualId: string = await nodeMachineId.machineId(true); + const actualHashedId = createHmac("sha256", actualId.toUpperCase()).update("atlascli").digest("hex"); const telemetry = Telemetry.create( diff --git a/tests/unit/telemetry.test.ts b/tests/unit/telemetry.test.ts index dfb9e6d6..969a4ee8 100644 --- a/tests/unit/telemetry.test.ts +++ b/tests/unit/telemetry.test.ts @@ -1,19 +1,12 @@ -// Mock node-machine-id to simulate machine ID resolution -jest.mock("node-machine-id", () => ({ - machineId: jest.fn(), -})); - import { ApiClient } from "../../src/common/atlas/apiClient.js"; import { Session } from "../../src/session.js"; import { DEVICE_ID_TIMEOUT, Telemetry } from "../../src/telemetry/telemetry.js"; import { BaseEvent, TelemetryResult } from "../../src/telemetry/types.js"; import { EventCache } from "../../src/telemetry/eventCache.js"; import { config } from "../../src/config.js"; -import { MACHINE_METADATA } from "../../src/telemetry/constants.js"; import { jest } from "@jest/globals"; -import { createHmac } from "crypto"; import logger, { LogId } from "../../src/logger.js"; -import nodeMachineId from "node-machine-id"; +import { createHmac } from "crypto"; // Mock the ApiClient to avoid real API calls jest.mock("../../src/common/atlas/apiClient.js"); @@ -23,9 +16,10 @@ const MockApiClient = ApiClient as jest.MockedClass; jest.mock("../../src/telemetry/eventCache.js"); const MockEventCache = EventCache as jest.MockedClass; -const mockMachineId = jest.spyOn(nodeMachineId, "machineId"); - describe("Telemetry", () => { + const machineId = "test-machine-id"; + const hashedMachineId = createHmac("sha256", machineId.toUpperCase()).update("atlascli").digest("hex"); + let mockApiClient: jest.Mocked; let mockEventCache: jest.Mocked; let session: Session; @@ -131,26 +125,15 @@ describe("Telemetry", () => { setAgentRunner: jest.fn().mockResolvedValue(undefined), } as unknown as Session; - // Create the telemetry instance with mocked dependencies - telemetry = Telemetry.create( - session, - config, - { - ...MACHINE_METADATA, - }, - mockEventCache - ); + telemetry = Telemetry.create(session, config, { + eventCache: mockEventCache, + getRawMachineId: () => Promise.resolve(machineId), + }); config.telemetry = "enabled"; }); describe("sending events", () => { - beforeEach(() => { - // @ts-expect-error This is a workaround - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - mockMachineId.mockResolvedValue("test-machine-id"); - }); - describe("when telemetry is enabled", () => { it("should send events successfully", async () => { const testEvent = createTestEvent(); @@ -200,141 +183,136 @@ describe("Telemetry", () => { sendEventsCalledWith: [cachedEvent, newEvent], }); }); - }); - describe("when telemetry is disabled", () => { - beforeEach(() => { - config.telemetry = "disabled"; - }); + it("should correctly add common properties to events", () => { + const commonProps = telemetry.getCommonProperties(); - it("should not send events", async () => { - const testEvent = createTestEvent(); - - await telemetry.emitEvents([testEvent]); + // Use explicit type assertion + const expectedProps: Record = { + mcp_client_version: "1.0.0", + mcp_client_name: "test-agent", + session_id: "test-session-id", + config_atlas_auth: "true", + config_connection_string: expect.any(String) as unknown as string, + device_id: hashedMachineId, + }; - verifyMockCalls(); + expect(commonProps).toMatchObject(expectedProps); }); - }); - it("should correctly add common properties to events", () => { - const commonProps = telemetry.getCommonProperties(); + describe("machine ID resolution", () => { + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers(); + }); - // Use explicit type assertion - const expectedProps: Record = { - mcp_client_version: "1.0.0", - mcp_client_name: "test-agent", - session_id: "test-session-id", - config_atlas_auth: "true", - config_connection_string: expect.any(String) as unknown as string, - }; + afterEach(() => { + jest.clearAllMocks(); + jest.useRealTimers(); + }); - expect(commonProps).toMatchObject(expectedProps); - }); + it("should successfully resolve the machine ID", async () => { + telemetry = Telemetry.create(session, config, { + getRawMachineId: () => Promise.resolve(machineId), + }); - describe("when DO_NOT_TRACK environment variable is set", () => { - let originalEnv: string | undefined; + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - beforeEach(() => { - originalEnv = process.env.DO_NOT_TRACK; - process.env.DO_NOT_TRACK = "1"; - }); + await telemetry.deviceIdPromise; - afterEach(() => { - process.env.DO_NOT_TRACK = originalEnv; - }); + expect(telemetry["isBufferingEvents"]).toBe(false); + expect(telemetry.getCommonProperties().device_id).toBe(hashedMachineId); + }); - it("should not send events", async () => { - const testEvent = createTestEvent(); + it("should handle machine ID resolution failure", async () => { + const loggerSpy = jest.spyOn(logger, "debug"); - await telemetry.emitEvents([testEvent]); + telemetry = Telemetry.create(session, config, { + getRawMachineId: () => Promise.reject(new Error("Failed to get device ID")), + }); - verifyMockCalls(); - }); - }); - }); + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - describe("machine ID resolution", () => { - const machineId = "test-machine-id"; - const hashedMachineId = createHmac("sha256", machineId.toUpperCase()).update("atlascli").digest("hex"); + await telemetry.deviceIdPromise; - beforeEach(() => { - jest.useFakeTimers(); - jest.clearAllMocks(); - }); + expect(telemetry["isBufferingEvents"]).toBe(false); + expect(telemetry.getCommonProperties().device_id).toBe("unknown"); - afterEach(() => { - jest.useRealTimers(); - jest.clearAllMocks(); - }); + expect(loggerSpy).toHaveBeenCalledWith( + LogId.telemetryDeviceIdFailure, + "telemetry", + "Error: Failed to get device ID" + ); + }); - it("should successfully resolve the machine ID", async () => { - // @ts-expect-error This is a workaround - mockMachineId.mockResolvedValue(machineId); - telemetry = Telemetry.create(session, config); + it("should timeout if machine ID resolution takes too long", async () => { + const loggerSpy = jest.spyOn(logger, "debug"); - expect(telemetry["isBufferingEvents"]).toBe(true); - expect(telemetry.getCommonProperties().device_id).toBe(undefined); + telemetry = Telemetry.create(session, config, { getRawMachineId: () => new Promise(() => {}) }); - await telemetry.deviceIdPromise; + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - expect(telemetry["isBufferingEvents"]).toBe(false); - expect(telemetry.getCommonProperties().device_id).toBe(hashedMachineId); - }); + jest.advanceTimersByTime(DEVICE_ID_TIMEOUT / 2); - it("should handle machine ID resolution failure", async () => { - const loggerSpy = jest.spyOn(logger, "debug"); + // Make sure the timeout doesn't happen prematurely. + expect(telemetry["isBufferingEvents"]).toBe(true); + expect(telemetry.getCommonProperties().device_id).toBe(undefined); - // @ts-expect-error This is a workaround - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - mockMachineId.mockRejectedValue(new Error("Failed to get device ID")); + jest.advanceTimersByTime(DEVICE_ID_TIMEOUT); - telemetry = Telemetry.create(session, config); + await telemetry.deviceIdPromise; - expect(telemetry["isBufferingEvents"]).toBe(true); - expect(telemetry.getCommonProperties().device_id).toBe(undefined); + expect(telemetry.getCommonProperties().device_id).toBe("unknown"); + expect(telemetry["isBufferingEvents"]).toBe(false); + expect(loggerSpy).toHaveBeenCalledWith( + LogId.telemetryDeviceIdTimeout, + "telemetry", + "Device ID retrieval timed out" + ); + }); + }); + }); - await telemetry.deviceIdPromise; + describe("when telemetry is disabled", () => { + beforeEach(() => { + config.telemetry = "disabled"; + }); - expect(telemetry["isBufferingEvents"]).toBe(false); - expect(telemetry.getCommonProperties().device_id).toBe("unknown"); + afterEach(() => { + config.telemetry = "enabled"; + }); - expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryDeviceIdFailure, - "telemetry", - "Error: Failed to get device ID" - ); - }); + it("should not send events", async () => { + const testEvent = createTestEvent(); - it("should timeout if machine ID resolution takes too long", async () => { - const loggerSpy = jest.spyOn(logger, "debug"); + await telemetry.emitEvents([testEvent]); - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - mockMachineId.mockImplementation(() => { - return new Promise(() => {}); + verifyMockCalls(); }); + }); - telemetry = Telemetry.create(session, config); - - expect(telemetry["isBufferingEvents"]).toBe(true); - expect(telemetry.getCommonProperties().device_id).toBe(undefined); + describe("when DO_NOT_TRACK environment variable is set", () => { + let originalEnv: string | undefined; - jest.advanceTimersByTime(DEVICE_ID_TIMEOUT / 2); + beforeEach(() => { + originalEnv = process.env.DO_NOT_TRACK; + process.env.DO_NOT_TRACK = "1"; + }); - // Make sure the timeout doesn't happen prematurely. - expect(telemetry["isBufferingEvents"]).toBe(true); - expect(telemetry.getCommonProperties().device_id).toBe(undefined); + afterEach(() => { + process.env.DO_NOT_TRACK = originalEnv; + }); - jest.advanceTimersByTime(DEVICE_ID_TIMEOUT); + it("should not send events", async () => { + const testEvent = createTestEvent(); - await telemetry.deviceIdPromise; + await telemetry.emitEvents([testEvent]); - expect(telemetry.getCommonProperties().device_id).toBe("unknown"); - expect(telemetry["isBufferingEvents"]).toBe(false); - expect(loggerSpy).toHaveBeenCalledWith( - LogId.telemetryDeviceIdTimeout, - "telemetry", - "Device ID retrieval timed out" - ); + verifyMockCalls(); + }); }); }); }); From 5d981ee1606384d9c6f9ab1d1731b67e973cc2ee Mon Sep 17 00:00:00 2001 From: gagik Date: Wed, 30 Apr 2025 11:11:17 +0200 Subject: [PATCH 13/15] fix: changes from feedback --- src/deferred-promise.ts | 26 ++++++++++++-------------- src/telemetry/telemetry.ts | 3 +-- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/deferred-promise.ts b/src/deferred-promise.ts index e1900ea3..ae9a0113 100644 --- a/src/deferred-promise.ts +++ b/src/deferred-promise.ts @@ -26,24 +26,22 @@ export class DeferredPromise extends Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion this.reject = rejectFn!; - if (timeout !== undefined) { + if (timeout !== undefined && onTimeout) { this.timeoutId = setTimeout(() => { - onTimeout?.(this.resolve, this.reject); + onTimeout(this.resolve, this.reject); }, timeout); } - if (executor) { - executor( - (value: T) => { - if (this.timeoutId) clearTimeout(this.timeoutId); - this.resolve(value); - }, - (reason: Error) => { - if (this.timeoutId) clearTimeout(this.timeoutId); - this.reject(reason); - } - ); - } + executor( + (value: T) => { + if (this.timeoutId) clearTimeout(this.timeoutId); + this.resolve(value); + }, + (reason: Error) => { + if (this.timeoutId) clearTimeout(this.timeoutId); + this.reject(reason); + } + ); } static fromPromise(promise: Promise, options: DeferredPromiseOptions = {}): DeferredPromise { diff --git a/src/telemetry/telemetry.ts b/src/telemetry/telemetry.ts index aea161f4..30a0363b 100644 --- a/src/telemetry/telemetry.ts +++ b/src/telemetry/telemetry.ts @@ -77,7 +77,7 @@ export class Telemetry { } /** - * @returns A hashed, unique identifier for the running device or `undefined` if not known. + * @returns A hashed, unique identifier for the running device or `"unknown"` if not known. */ private async getDeviceId(): Promise { try { @@ -126,7 +126,6 @@ export class Telemetry { public getCommonProperties(): CommonProperties { return { ...this.commonProperties, - device_id: this.commonProperties.device_id, mcp_client_version: this.session.agentRunner?.version, mcp_client_name: this.session.agentRunner?.name, session_id: this.session.sessionId, From bc57a97a9b995394720dad7c8a89bf1d8082607c Mon Sep 17 00:00:00 2001 From: gagik Date: Wed, 30 Apr 2025 11:15:41 +0200 Subject: [PATCH 14/15] fix: remove old implementation --- src/telemetry/constants.ts | 2 +- src/telemetry/device-id.ts | 21 --------------------- 2 files changed, 1 insertion(+), 22 deletions(-) delete mode 100644 src/telemetry/device-id.ts diff --git a/src/telemetry/constants.ts b/src/telemetry/constants.ts index 58e4aec5..998f6e24 100644 --- a/src/telemetry/constants.ts +++ b/src/telemetry/constants.ts @@ -1,6 +1,6 @@ import { packageInfo } from "../packageInfo.js"; import { type CommonStaticProperties } from "./types.js"; -import { getDeviceId } from "./device-id.js"; + /** * Machine-specific metadata formatted for telemetry */ diff --git a/src/telemetry/device-id.ts b/src/telemetry/device-id.ts deleted file mode 100644 index e9c48d63..00000000 --- a/src/telemetry/device-id.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { createHmac } from "crypto"; -import nodeMachineId from "node-machine-id"; -import logger, { LogId } from "../logger.js"; - -export function getDeviceId(): string { - try { - const originalId = nodeMachineId.machineIdSync(true); - // Create a hashed format from the all uppercase version of the machine ID - // to match it exactly with the denisbrodbeck/machineid library that Atlas CLI uses. - const hmac = createHmac("sha256", originalId.toUpperCase()); - - /** This matches the message used to create the hashes in Atlas CLI */ - const DEVICE_ID_HASH_MESSAGE = "atlascli"; - - hmac.update(DEVICE_ID_HASH_MESSAGE); - return hmac.digest("hex"); - } catch (error) { - logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error)); - return "unknown"; - } -} From 8816befecf06fad4ed346dae396f64e534ee909c Mon Sep 17 00:00:00 2001 From: gagik Date: Thu, 1 May 2025 13:37:31 +0200 Subject: [PATCH 15/15] fix: remove assertions --- src/deferred-promise.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/deferred-promise.ts b/src/deferred-promise.ts index ae9a0113..1eb3f6e0 100644 --- a/src/deferred-promise.ts +++ b/src/deferred-promise.ts @@ -5,8 +5,8 @@ type DeferredPromiseOptions = { /** Creates a promise and exposes its resolve and reject methods, with an optional timeout. */ export class DeferredPromise extends Promise { - resolve!: (value: T) => void; - reject!: (reason: unknown) => void; + resolve: (value: T) => void; + reject: (reason: unknown) => void; private timeoutId?: NodeJS.Timeout; constructor(