feat: update connectionString appName param - [MCP-68] #406

blva · 2025-07-28T15:28:30Z

Proposed changes

Refactored telemetry to use centralized device ID utility
Extended appName format to appName--deviceId--clientName
Created src/helpers/deviceId.ts utility for device ID retrieval
Updated setAppNameParamIfMissing function to support extended format
Modified Session and Atlas connect tools to use new format
Added unit tests and integration tests
Decoupled connectionString validation from connection

Checklist

I have signed the MongoDB CLA

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

src/telemetry/telemetry.ts

src/helpers/connectionOptions.ts

src/helpers/deviceId.ts

tests/unit/telemetry.test.ts

gagik · 2025-08-01T11:41:02Z

src/telemetry/telemetry.ts


    private constructor(
        private readonly session: Session,
        private readonly userConfig: UserConfig,
        private readonly commonProperties: CommonProperties,
-        { eventCache, getRawMachineId }: { eventCache: EventCache; getRawMachineId: () => Promise<string> }
+        { eventCache }: { eventCache: EventCache }


nit: I think it'd still be good to pass getDeviceIdForConnection for explicit DI. It also helps us avoid mocking the file which is a bit of JavaScript magic

done! thanks

gagik

It's important in my opinion to avoid double device ID resolution

gagik · 2025-08-01T11:45:14Z

src/helpers/connectionOptions.ts

    }

+    const deviceId = components.deviceId || (await getDeviceIdForConnection());


we shouldn't end up awaiting device ID a second time, can we organize it so this device ID gets passed from one point of entry and we never fallback to this function? deviceId doesn't get passed in connectToMongoDB right now.

I refactored this quite a bit into a singleton so that we avoid double resolution, let me know what you think and if I'm missing any gaps!

src/helpers/connectionOptions.ts

src/helpers/deviceId.ts

blva · 2025-08-18T16:03:54Z

should be ready for review

This reverts commit ae1d6d0.

gagik · 2025-08-19T13:15:55Z

src/common/logger.ts

@@ -30,8 +31,8 @@ export const LogId = {
    telemetryEmitStart: mongoLogId(1_002_003),
    telemetryEmitSuccess: mongoLogId(1_002_004),
    telemetryMetadataError: mongoLogId(1_002_005),
-    telemetryDeviceIdFailure: mongoLogId(1_002_006),
-    telemetryDeviceIdTimeout: mongoLogId(1_002_007),
+    deviceIdResolutionError: mongoLogId(1_002_006),


very nit: we can still keep the telemetry prefix since this is still what it's related to (and we have this prefix group pattern right now)

but now resolution could fail upon connection tool since this will now be used in two places, no?

src/helpers/deviceId.ts

gagik · 2025-08-19T13:21:01Z

src/helpers/deviceId.ts

+        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, timeout?: number): DeviceIdService {
+        if (DeviceIdService.instance) {
+            return DeviceIdService.instance;
+        }
+        DeviceIdService.instance = new DeviceIdService(logger, timeout ?? DEVICE_ID_TIMEOUT);
+        return DeviceIdService.instance;
+    }


I generally prefer to avoid side-effects in constructors i.e. this.startDeviceIdCalculation()
Since we already have an init pattern, we could make this constructor private and have one create(...) will also call the startDeviceIdCalculation(); after creating it.

Telemetry service is example of that

mongodb-mcp-server/src/telemetry/telemetry.ts

Line 37 in 0e28212

static create(

cool! I think there's no harm in clarifying this one, but bundling it with your comment below are yo suggesting to do something like telemetry but still maintain a singleton? or just use the same approach and inject the logger object on getDeviceId calls?

either is fine I think, we can also have a late setter for the logger

gagik · 2025-08-19T13:29:06Z

src/helpers/deviceId.ts

+    /**
+     * Checks if the DeviceIdService is initialized.
+     * @returns True if the DeviceIdService is initialized, false otherwise
+     */
+    public static isInitialized(): boolean {
+        return DeviceIdService.instance !== undefined;
+    }
+
+    /**
+     * Gets the singleton instance of DeviceIdService.
+     * @returns The DeviceIdService instance
+     */
+    public static getInstance(): DeviceIdService {
+        if (!DeviceIdService.instance) {
+            throw new Error("DeviceIdService not initialized");
+        }
+        return DeviceIdService.instance;
+    }


I think this singleton instance pattern discourages explicit dependency injection which makes it too easy to end up accessing this service prematurely, thus needing these checks to make sure it's initialized. I get it from perspective of only doing the resolution once so we could still keep the static instance as far as create goes and maybe throw an error if someone tries to re-create the service.

But other than that, the getInstance is only used in tests right now and I think for that it'd have been preferable to create the DeviceIdService inside the test and pass it to whatever is being tested instead

gagik · 2025-08-21T08:17:25Z

src/helpers/deviceId.ts

+        this.abortController = new AbortController();
+        this.deviceIdPromise = this.calculateDeviceId();
+        // start the promise upon setup
+        void this.deviceIdPromise;


the promise is already "started" from the function being called, this doesn't do anything

gagik · 2025-08-21T08:18:33Z

src/helpers/deviceId.ts

+     * Sets up the device ID calculation promise and abort controller.
+     */
+    private setup(): void {
+        this.abortController = new AbortController();


it'd be good to have the abortcontroller defined inside the calculateDeviceId as it's directly tied to it

gagik · 2025-08-21T08:18:57Z

src/helpers/deviceId.ts

+    }
+
+    public static create(logger: LoggerBase, timeout?: number): DeviceId {
+        if (this.instance) {


nice! yeah this is what I meant. more radically, we can also throw an error to enforce that this creation is only done ever

cool! in that case I guess we'd make get() static so that anyone can get the device Id

gagik · 2025-08-21T08:21:20Z

src/helpers/deviceId.ts

+        }
+
+        try {
+            const deviceId = await getDeviceId({


getDeviceId already does plenty of error handling, including the abort error. I don't think this try/catch will ever catch anything

does it automatically assign unknown upon error? If so we can remove

gagik · 2025-08-21T08:26:23Z

src/telemetry/telemetry.ts

        this.commonProperties.is_container_env = containerEnv;

        this.isBufferingEvents = false;
    }

    public async close(): Promise<void> {
-        this.deviceIdAbortController.abort();
        this.isBufferingEvents = false;


would this.deviceId.close make sense here?

I moved it to index.ts so I'm closing it there as well, makes sense?

gagik · 2025-08-21T08:31:18Z

tests/unit/helpers/deviceId.test.ts

+                options.abortSignal?.addEventListener("abort", () => {
+                    clearTimeout(timeout);
+                    const abortError = new Error("Aborted");
+                    abortError.name = "AbortError";


is this a real error that can get thrown? pretty sure this is handled by the getDeviceId function's try/catch so would never reach our own code.

agreed - this is an autogen test that I missed to remove

gagik

Other than above, changes look good to me 🙂 feel free to go ahead as you see fit.

blva added 5 commits July 28, 2025 16:05

feat: update connectionString appName param - [MCP-68]

c5c91e9

Merge remote-tracking branch 'origin/main' into MCP-68

4cf78c2

add removed test

026b91a

update tests

680e1e1

Merge remote-tracking branch 'origin/main' into MCP-68

92aab61

blva force-pushed the MCP-68 branch from 152723c to 92aab61 Compare July 31, 2025 12:30

blva added 3 commits July 31, 2025 13:51

add timeout test

eca40e1

Merge remote-tracking branch 'origin/main' into MCP-68

6cf0ae6

fix

524d965

blva marked this pull request as ready for review July 31, 2025 13:13

Copilot AI review requested due to automatic review settings July 31, 2025 13:13

blva requested a review from a team as a code owner July 31, 2025 13:13

This comment was marked as outdated.

Sign in to view

Update src/helpers/deviceId.ts

72f1ab8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>