feat: update connectionString appName param - [MCP-68] #406
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/telemetry/telemetry.ts
Outdated
|
|
||
| private constructor( | ||
| private readonly session: Session, | ||
| private readonly userConfig: UserConfig, | ||
| private readonly commonProperties: CommonProperties, | ||
| { eventCache, getRawMachineId }: { eventCache: EventCache; getRawMachineId: () => Promise<string> } | ||
| { eventCache }: { eventCache: EventCache } |
There was a problem hiding this comment.
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
src/helpers/connectionOptions.ts
Outdated
| } | ||
|
|
||
| const deviceId = components.deviceId || (await getDeviceIdForConnection()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| if (!searchParams.has("appName") && defaultAppName !== undefined) { | ||
| searchParams.set("appName", defaultAppName); | ||
| // Only set appName if it's not already present | ||
| if (searchParams.has("appName")) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm confused, the point of deviceID is to get the separate users that don't authenticate, so we can identify them as unique, in that sense I think we should always keep it. This was also requested so I'm inclined to always keep it
There was a problem hiding this comment.
essentially the idea there was to minimize visibility of this app name setting when they connect to i.e. localhost so for users they don't see identifiers appended to the app name they wouldn't expected but it's not a big deal to keep it as is yeah
|
should be ready for review |
This reverts commit ae1d6d0.
| @@ -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), | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
but now resolution could fail upon connection tool since this will now be used in two places, no?
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Telemetry service is example of that
There was a problem hiding this comment.
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?
| /** | ||
| * 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; | ||
| } |
There was a problem hiding this comment.
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
Proposed changes
appName--deviceId--clientNameChecklist