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

Checklist

I have signed the MongoDB CLA

coveralls · 2025-07-28T16:42:16Z

Pull Request Test Coverage Report for Build 16673953200

Details

58 of 58 (100.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 81.181%

Totals
Change from base Build 16649286152:	0.2%
Covered Lines:	3301
Relevant Lines:	4029

💛 - Coveralls

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

src/telemetry/telemetry.ts

src/helpers/connectionOptions.ts

fmenezes · 2025-07-31T13:39:33Z

src/helpers/connectionOptions.ts

+    }
+
+    // Build the extended appName format: appName--deviceId--clientName
+    const extendedAppName = `${components.appName}--${deviceId}--${clientName}`;


[q] what if components.appName is empty? should it be unknown like the others?

I see the code we are getting packageInfo stuff, should we not make it a parameter and get package info straight away?

the previous behaviour is that we would set it to appName: ${packageInfo.mcpServerName} ${packageInfo.version}, if it's not defined, so we always send it. I could move that logic here, is that what you mean with the packageInfo question?

how would appName be defined? is it really a parameter? is there an env var?

components.appName seems to be fixed as ${packageInfo.mcpServerName} ${packageInfo.version} always

Copilot

Pull Request Overview

This PR refactors telemetry device ID handling by extracting device ID generation logic into a centralized utility and extending the MongoDB connection string appName parameter format to include device ID and client name components.

Centralizes device ID retrieval logic into a reusable helper utility
Extends appName format from simple name to appName--deviceId--clientName structure
Updates telemetry and connection handling to use the new centralized approach

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/helpers/deviceId.ts	New centralized utility for device ID generation with error handling
src/helpers/connectionOptions.ts	Enhanced appName parameter setting with extended format support
src/common/session.ts	Updated to use extended appName format with client name
src/telemetry/telemetry.ts	Refactored to use centralized device ID utility
src/tools/atlas/connect/connectCluster.ts	Updated Atlas connection to use extended appName format
tests/	Comprehensive test coverage for new functionality

Comments suppressed due to low confidence (2)

tests/unit/helpers/connectionOptions.test.ts:34

The test comment mentions URL normalization but doesn't verify the exact appName value. Consider adding an assertion that explicitly checks the appName parameter value to ensure it remains 'ExistingApp' and hasn't been modified.

            // The ConnectionString library normalizes URLs, so we need to check the content rather than exact equality

tests/unit/helpers/deviceId.test.ts:109

This test uses fake timers but doesn't actually test timeout behavior since the mock immediately calls the onError callback. Consider creating a test that truly exercises the timeout mechanism by having the getDeviceId promise not resolve within the timeout period.

            vi.useFakeTimers();

src/helpers/deviceId.ts

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

fmenezes · 2025-07-31T15:41:26Z

src/helpers/deviceId.ts

+ */
+export async function getDeviceIdForConnection(): Promise<string> {
+    const controller = new AbortController();
+    const timeoutId = setTimeout(() => controller.abort(), DEVICE_ID_TIMEOUT);


why do we need timeout? getDeviceId has a timeout builtin

gagik · 2025-08-01T11:34:01Z

tests/unit/telemetry.test.ts

                });

-                it("should timeout if machine ID resolution takes too long", async () => {


why are we removing the timeout test? I think it is useful to have

didn't remove, moved it to deviceId unit tests

ah saw this now, nevermind, ty!

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

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.

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

src/helpers/connectionOptions.ts

-    if (!searchParams.has("appName") && defaultAppName !== undefined) {
-        searchParams.set("appName", defaultAppName);
+    // Only set appName if it's not already present
+    if (searchParams.has("appName")) {


In Compass / mongosh we only pass the device ID if it's an Atlas host to minimize visibility of this, can we also do this here?

blva added 4 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>

fmenezes reviewed Jul 31, 2025

View reviewed changes

src/telemetry/telemetry.ts Show resolved Hide resolved

fmenezes reviewed Jul 31, 2025

View reviewed changes

src/helpers/connectionOptions.ts Outdated Show resolved Hide resolved

fmenezes reviewed Jul 31, 2025

View reviewed changes

src/helpers/connectionOptions.ts Outdated Show resolved Hide resolved

add buffering update back

c3f0928

fmenezes reviewed Jul 31, 2025

View reviewed changes

address comment

5669609

blva requested a review from Copilot July 31, 2025 14:34

Copilot AI reviewed Jul 31, 2025

View reviewed changes

src/helpers/deviceId.ts Outdated Show resolved Hide resolved

blva and others added 2 commits July 31, 2025 15:46

Update src/helpers/deviceId.ts

ff80ff5

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

fix

251414c

blva requested a review from fmenezes July 31, 2025 15:34

fmenezes reviewed Jul 31, 2025

View reviewed changes

address comment: remove timeout

10f7054

gagik reviewed Aug 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

blva commented Jul 28, 2025 •

edited

Loading

coveralls commented Jul 28, 2025 •

edited

Loading

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmenezes Jul 31, 2025

fmenezes Jul 31, 2025

blva Jul 31, 2025

fmenezes Jul 31, 2025

fmenezes Jul 31, 2025

Copilot AI left a comment

Uh oh!

fmenezes Jul 31, 2025

gagik Aug 1, 2025

blva Aug 1, 2025

gagik Aug 1, 2025

gagik Aug 1, 2025

gagik left a comment •

edited

Loading

gagik Aug 1, 2025 •

edited

Loading

gagik Aug 1, 2025

		});

		it("should timeout if machine ID resolution takes too long", async () => {

		}

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

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

blva commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

coveralls commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16673953200

Details

💛 - Coveralls

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

gagik Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blva commented Jul 28, 2025 •

edited

Loading

coveralls commented Jul 28, 2025 •

edited

Loading

gagik left a comment •

edited

Loading

gagik Aug 1, 2025 •

edited

Loading