refactor and address some comments

Author: blva

src/common/connectionManager.ts

@@ -6,6 +6,8 @@ import { packageInfo } from "./packageInfo.js";
 import ConnectionString from "mongodb-connection-string-url";
 import { MongoClientOptions } from "mongodb";
 import { ErrorCodes, MongoDBError } from "./errors.js";
+import { DeviceIdService } from "../helpers/deviceId.js";
+import { AppNameComponents } from "../helpers/connectionOptions.js";
 
 export interface AtlasClusterConnectionInfo {
     username: string;
@@ -67,11 +69,19 @@ export interface ConnectionManagerEvents {
 
 export class ConnectionManager extends EventEmitter<ConnectionManagerEvents> {
     private state: AnyConnectionState;
+    private deviceId: DeviceIdService;
+    private clientName: string;
 
     constructor() {
         super();
 
         this.state = { tag: "disconnected" };
+        this.deviceId = DeviceIdService.getInstance();
+        this.clientName = "unknown";
+    }
+
+    setClientName(clientName: string): void {
+        this.clientName = clientName;
     }
 
     async connect(settings: ConnectionSettings): Promise<AnyConnectionState> {
@@ -84,14 +94,15 @@ export class ConnectionManager extends EventEmitter<ConnectionManagerEvents> {
         let serviceProvider: NodeDriverServiceProvider;
         try {
             settings = { ...settings };
+            const appNameComponents: AppNameComponents = {
+                appName: `${packageInfo.mcpServerName} ${packageInfo.version}`,
+                deviceId: this.deviceId.getDeviceId(),
+                clientName: this.clientName,
+            };
+
             settings.connectionString = await setAppNameParamIfMissing({
                 connectionString: settings.connectionString,
-                components: {
-                    appName: `${packageInfo.mcpServerName} ${packageInfo.version}`,
-                    // deviceId: deviceId, //TODO: MCP-68 - get deviceId from session
-                    // clientName: this.clientInfo?.name || "unknown",
-                    clientName: "unknown", //TODO: MCP-68 - get client name from session
-                },
+                components: appNameComponents,
             });
 
             serviceProvider = await NodeDriverServiceProvider.connect(settings.connectionString, {

src/common/session.ts

@@ -72,20 +72,22 @@ export class Session extends EventEmitter<SessionEvents> {
 
     setMcpClient(mcpClient: Implementation | undefined): void {
         if (!mcpClient) {
-            this.mcpClient = undefined;
+            this.connectionManager.setClientName("unknown");
             this.logger.debug({
                 id: LogId.serverMcpClientSet,
                 context: "session",
                 message: "MCP client info not found",
             });
-            return;
         }
 
         this.mcpClient = {
-            name: mcpClient.name || "unknown",
-            version: mcpClient.version || "unknown",
-            title: mcpClient.title || "unknown",
+            name: mcpClient?.name || "unknown",
+            version: mcpClient?.version || "unknown",
+            title: mcpClient?.title || "unknown",
         };
+
+        // Set the client name on the connection manager for appName generation
+        this.connectionManager.setClientName(this.mcpClient.name || "unknown");
     }
 
     async disconnect(): Promise<void> {

src/helpers/connectionOptions.ts

@@ -1,10 +1,9 @@
 import { MongoClientOptions } from "mongodb";
 import ConnectionString from "mongodb-connection-string-url";
-import { getDeviceIdForConnection } from "./deviceId.js";
 
 export interface AppNameComponents {
     appName: string;
-    deviceId?: string;
+    deviceId?: Promise<string>;
     clientName?: string;
 }
 
@@ -30,7 +29,8 @@ export async function setAppNameParamIfMissing({
         return connectionStringUrl.toString();
     }
 
-    const deviceId = components.deviceId || (await getDeviceIdForConnection());
+    const deviceId = components.deviceId ? await components.deviceId : "unknown";
+
     const clientName = components.clientName || "unknown";
 
     // Build the extended appName format: appName--deviceId--clientName

src/helpers/deviceId.ts

@@ -1,46 +1,156 @@
 import { getDeviceId } from "@mongodb-js/device-id";
-import nodeMachineId from "node-machine-id";
-import { LogId, CompositeLogger } from "../common/logger.js";
+import { LogId, LoggerBase } from "../common/logger.js";
 
 export const DEVICE_ID_TIMEOUT = 3000;
 
 /**
- * Retrieves the device ID for telemetry purposes.
- * The device ID is generated using the machine ID and additional logic to handle errors.
- *
- * @returns Promise that resolves to the device ID string
- * If an error occurs during retrieval, the function returns "unknown".
- *
- * @example
- * ```typescript
- * const deviceId = await getDeviceIdForConnection(logger);
- * console.log(deviceId); // Outputs the device ID or "unknown" in case of failure
- * ```
+ * Singleton class for managing device ID retrieval and caching.
+ * Starts device ID calculation early and is shared across all services.
  */
-export async function getDeviceIdForConnection(logger: CompositeLogger): Promise<string> {
-    const controller = new AbortController();
-
-    try {
-        const deviceId = await getDeviceId({
-            getMachineId: () => nodeMachineId.machineId(true),
-            onError: (reason, error) => {
-                switch (reason) {
-                    case "resolutionError":
-                        logger.debug(LogId.telemetryDeviceIdFailure, "deviceId", String(error));
-                        break;
-                    case "timeout":
-                        logger.debug(LogId.telemetryDeviceIdTimeout, "deviceId", "Device ID retrieval timed out");
-                        break;
-                    case "abort":
-                        // No need to log in the case of aborts
-                        break;
-                }
-            },
-            abortSignal: controller.signal,
-        });
-        return deviceId;
-    } catch (error) {
-        logger.debug(LogId.telemetryDeviceIdFailure, "deviceId", `Failed to get device ID: ${String(error)}`);
-        return "unknown";
+export class DeviceIdService {
+    private static instance: DeviceIdService | undefined = undefined;
+    private deviceId: string | undefined = undefined;
+    private deviceIdPromise: Promise<string> | undefined = undefined;
+    private abortController: AbortController | undefined = undefined;
+    private logger: LoggerBase;
+    private getMachineId: () => Promise<string>;
+
+    private constructor(logger: LoggerBase, getMachineId: () => Promise<string>) {
+        this.logger = logger;
+        this.getMachineId = getMachineId;
+        // Start device ID calculation immediately
+        this.startDeviceIdCalculation();
+    }
+
+    /**
+     * Initializes the DeviceIdService singleton with a logger.
+     * A separated init method is used to use a single instance of the logger.
+     * @param logger - The logger instance to use
+     * @returns The DeviceIdService instance
+     */
+    public static init(logger: LoggerBase, getMachineId: () => Promise<string>): DeviceIdService {
+        if (DeviceIdService.instance) {
+            return DeviceIdService.instance;
+        }
+        DeviceIdService.instance = new DeviceIdService(logger, getMachineId);
+        return DeviceIdService.instance;
+    }
+
+    /**
+     * Gets the singleton instance of DeviceIdService.
+     * @returns The DeviceIdService instance
+     */
+    public static getInstance(): DeviceIdService {
+        if (!DeviceIdService.instance) {
+            throw Error("DeviceIdService not initialized");
+        }
+        return DeviceIdService.instance;
+    }
+
+    /**
+     * Resets the singleton instance (mainly for testing).
+     */
+    static resetInstance(): void {
+        DeviceIdService.instance = undefined;
+    }
+
+    /**
+     * Starts the device ID calculation process.
+     * This method is called automatically in the constructor.
+     */
+    private startDeviceIdCalculation(): void {
+        if (this.deviceIdPromise) {
+            return;
+        }
+
+        this.abortController = new AbortController();
+        this.deviceIdPromise = this.calculateDeviceId();
+    }
+
+    /**
+     * Gets the device ID, waiting for the calculation to complete if necessary.
+     * @returns Promise that resolves to the device ID string
+     */
+    public async getDeviceId(): Promise<string> {
+        // Return cached value if available
+        if (this.deviceId !== undefined) {
+            return this.deviceId;
+        }
+
+        // If calculation is already in progress, wait for it
+        if (this.deviceIdPromise) {
+            return this.deviceIdPromise;
+        }
+
+        // If somehow we don't have a promise, raise an error
+        throw new Error("Failed to get device ID");
+    }
+    /**
+     * Aborts any ongoing device ID calculation.
+     */
+    abortCalculation(): void {
+        if (this.abortController) {
+            this.abortController.abort();
+            this.abortController = undefined;
+        }
+        this.deviceIdPromise = undefined;
+    }
+
+    /**
+     * Internal method that performs the actual device ID calculation.
+     */
+    private async calculateDeviceId(): Promise<string> {
+        if (!this.abortController) {
+            throw new Error("Device ID calculation not started");
+        }
+
+        try {
+            const deviceId = await getDeviceId({
+                getMachineId: this.getMachineId,
+                onError: (reason, error) => {
+                    switch (reason) {
+                        case "resolutionError":
+                            this.logger.debug({
+                                id: LogId.telemetryDeviceIdFailure,
+                                context: "deviceId",
+                                message: `Device ID resolution error: ${String(error)}`,
+                            });
+                            break;
+                        case "timeout":
+                            this.logger.debug({
+                                id: LogId.telemetryDeviceIdTimeout,
+                                context: "deviceId",
+                                message: "Device ID retrieval timed out",
+                            });
+                            break;
+                        case "abort":
+                            // No need to log in the case of aborts
+                            break;
+                    }
+                },
+                abortSignal: this.abortController.signal,
+            });
+
+            // Cache the result
+            this.deviceId = deviceId;
+            return deviceId;
+        } catch (error) {
+            // Check if this was an abort error
+            if (error instanceof Error && error.name === "AbortError") {
+                throw error; // Re-throw abort errors
+            }
+
+            this.logger.debug({
+                id: LogId.telemetryDeviceIdFailure,
+                context: "deviceId",
+                message: `Failed to get device ID: ${String(error)}`,
+            });
+
+            // Cache the fallback value
+            this.deviceId = "unknown";
+            return "unknown";
+        } finally {
+            this.abortController = undefined;
+        }
     }
 }

src/index.ts

@@ -42,6 +42,8 @@ import { packageInfo } from "./common/packageInfo.js";
 import { StdioRunner } from "./transports/stdio.js";
 import { StreamableHttpRunner } from "./transports/streamableHttp.js";
 import { systemCA } from "@mongodb-js/devtools-proxy-support";
+import { DeviceIdService } from "./helpers/deviceId.js";
+import nodeMachineId from "node-machine-id";
 
 async function main(): Promise<void> {
     systemCA().catch(() => undefined); // load system CA asynchronously as in mongosh
@@ -50,6 +52,7 @@ async function main(): Promise<void> {
     assertVersionMode();
 
     const transportRunner = config.transport === "stdio" ? new StdioRunner(config) : new StreamableHttpRunner(config);
+    const deviceId = DeviceIdService.init(transportRunner.logger, () => nodeMachineId.machineId(true));
 
     const shutdown = (): void => {
         transportRunner.logger.info({
@@ -61,6 +64,7 @@ async function main(): Promise<void> {
         transportRunner
             .close()
             .then(() => {
+                deviceId.abortCalculation();
                 transportRunner.logger.info({
                     id: LogId.serverClosed,
                     context: "server",
@@ -69,6 +73,7 @@ async function main(): Promise<void> {
                 process.exit(0);
             })
             .catch((error: unknown) => {
+                deviceId.abortCalculation();
                 transportRunner.logger.error({
                     id: LogId.serverCloseFailure,
                     context: "server",

src/telemetry/telemetry.ts

@@ -5,8 +5,8 @@ import { LogId } from "../common/logger.js";
 import { ApiClient } from "../common/atlas/apiClient.js";
 import { MACHINE_METADATA } from "./constants.js";
 import { EventCache } from "./eventCache.js";
-import { getDeviceIdForConnection } from "../helpers/deviceId.js";
 import { detectContainerEnv } from "../helpers/container.js";
+import { DeviceIdService } from "../helpers/deviceId.js";
 
 type EventResult = {
     success: boolean;
@@ -18,14 +18,16 @@ export class Telemetry {
     /** Resolves when the setup is complete or a timeout occurs */
     public setupPromise: Promise<[string, boolean]> | undefined;
     private eventCache: EventCache;
+    private deviceId: DeviceIdService;
 
     private constructor(
         private readonly session: Session,
         private readonly userConfig: UserConfig,
         private readonly commonProperties: CommonProperties,
-        { eventCache }: { eventCache: EventCache }
+        { eventCache, deviceId }: { eventCache: EventCache; deviceId: DeviceIdService }
     ) {
         this.eventCache = eventCache;
+        this.deviceId = deviceId;
     }
 
     static create(
@@ -34,12 +36,14 @@ export class Telemetry {
         {
             commonProperties = { ...MACHINE_METADATA },
             eventCache = EventCache.getInstance(),
+            deviceId = DeviceIdService.getInstance(),
         }: {
             eventCache?: EventCache;
+            deviceId?: DeviceIdService;
             commonProperties?: CommonProperties;
         } = {}
     ): Telemetry {
-        const instance = new Telemetry(session, userConfig, commonProperties, { eventCache });
+        const instance = new Telemetry(session, userConfig, commonProperties, { eventCache, deviceId });
 
         void instance.setup();
         return instance;
@@ -49,10 +53,11 @@ export class Telemetry {
         if (!this.isTelemetryEnabled()) {
             return;
         }
-        this.setupPromise = Promise.all([getDeviceIdForConnection(), detectContainerEnv()]);
-        const [deviceId, containerEnv] = await this.setupPromise;
 
-        this.commonProperties.device_id = deviceId;
+        this.setupPromise = Promise.all([this.deviceId.getDeviceId(), detectContainerEnv()]);
+        const [deviceIdValue, containerEnv] = await this.setupPromise;
+
+        this.commonProperties.device_id = deviceIdValue;
         this.commonProperties.is_container_env = containerEnv;
 
         this.isBufferingEvents = false;