-
Notifications
You must be signed in to change notification settings - Fork 133
chore: add is_container_env to telemetry MCP-2 #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 16 commits
a480c94
126994c
bf204bb
3d91b40
fe64649
f73c208
671bbeb
c2a1246
678beee
8d53eea
95b0b06
911fdcb
4703628
931816f
0b60fe2
6ba2346
f4bfbf0
3424e0f
83fd1d9
7e3cdb1
c67877e
814ccb9
1dba800
a0c208c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,56 +7,92 @@ import { MACHINE_METADATA } from "./constants.js"; | |||||
import { EventCache } from "./eventCache.js"; | ||||||
import nodeMachineId from "node-machine-id"; | ||||||
import { getDeviceId } from "@mongodb-js/device-id"; | ||||||
import fs from "fs/promises"; | ||||||
|
||||||
async function fileExists(filePath: string): Promise<boolean> { | ||||||
try { | ||||||
await fs.access(filePath, fs.constants.F_OK); | ||||||
return true; // File exists | ||||||
} catch (e: unknown) { | ||||||
if ( | ||||||
e instanceof Error && | ||||||
( | ||||||
e as Error & { | ||||||
code: string; | ||||||
} | ||||||
).code === "ENOENT" | ||||||
) { | ||||||
return false; // File does not exist | ||||||
} | ||||||
throw e; // Re-throw unexpected errors | ||||||
gagik marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
} | ||||||
|
||||||
type EventResult = { | ||||||
success: boolean; | ||||||
error?: Error; | ||||||
}; | ||||||
async function isContainerized(): Promise<boolean> { | ||||||
if (process.env.container) { | ||||||
return true; | ||||||
} | ||||||
|
||||||
const exists = await Promise.all(["/.dockerenv", "/run/.containerenv", "/var/run/.containerenv"].map(fileExists)); | ||||||
|
||||||
export const DEVICE_ID_TIMEOUT = 3000; | ||||||
return exists.includes(true); | ||||||
} | ||||||
|
||||||
export class Telemetry { | ||||||
private isBufferingEvents: boolean = true; | ||||||
/** Resolves when the device ID is retrieved or timeout occurs */ | ||||||
public deviceIdPromise: Promise<string> | undefined; | ||||||
private deviceId: string | undefined; | ||||||
private containerEnv: boolean | undefined; | ||||||
|
||||||
private deviceIdAbortController = new AbortController(); | ||||||
private eventCache: EventCache; | ||||||
private getRawMachineId: () => Promise<string>; | ||||||
private getContainerEnv: () => Promise<boolean>; | ||||||
|
||||||
private constructor( | ||||||
private readonly session: Session, | ||||||
private readonly userConfig: UserConfig, | ||||||
private readonly commonProperties: CommonProperties, | ||||||
{ eventCache, getRawMachineId }: { eventCache: EventCache; getRawMachineId: () => Promise<string> } | ||||||
{ | ||||||
eventCache, | ||||||
getRawMachineId, | ||||||
getContainerEnv, | ||||||
}: { | ||||||
eventCache: EventCache; | ||||||
getRawMachineId: () => Promise<string>; | ||||||
getContainerEnv: () => Promise<boolean>; | ||||||
} | ||||||
) { | ||||||
this.eventCache = eventCache; | ||||||
this.getRawMachineId = getRawMachineId; | ||||||
this.getContainerEnv = getContainerEnv; | ||||||
} | ||||||
|
||||||
static create( | ||||||
static async create( | ||||||
session: Session, | ||||||
userConfig: UserConfig, | ||||||
{ | ||||||
commonProperties = { ...MACHINE_METADATA }, | ||||||
eventCache = EventCache.getInstance(), | ||||||
getRawMachineId = () => nodeMachineId.machineId(true), | ||||||
getContainerEnv = isContainerized, | ||||||
}: { | ||||||
eventCache?: EventCache; | ||||||
getRawMachineId?: () => Promise<string>; | ||||||
commonProperties?: CommonProperties; | ||||||
getContainerEnv?: () => Promise<boolean>; | ||||||
} = {} | ||||||
): Telemetry { | ||||||
const instance = new Telemetry(session, userConfig, commonProperties, { eventCache, getRawMachineId }); | ||||||
): Promise<Telemetry> { | ||||||
const instance = new Telemetry(session, userConfig, { | ||||||
eventCache, | ||||||
getRawMachineId, | ||||||
getContainerEnv, | ||||||
}); | ||||||
|
||||||
void instance.start(); | ||||||
await instance.start(); | ||||||
|
await instance.start(); | |
void instance.start(); |
This was an important detail in the existing implementation: start was not getting awaited before. This is why the behavior is equivalent to what was trying to be accomplished before with skipping the promise. Telemetry was already not waiting for the device ID resolution.
So the create function does not need to be asynchronous and can be left as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3s is nothing to wait, I think the code looks much better to wait on promises instead of "ignoring" them, I can move to last
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding potentially 3 seconds to the MCP server startup time for no real functional benefit is absolutely non-trivial. We shouldn't be slowing down anything in our logic for the sake of telemetry. This is also not ignoring a promise, this is explicitly spawning a parallel async operation here and the void
keyword exists for that reason.
If you want, you can separate the refactor bits of this PR into its own thing and we can have a larger discussion about those changes but in places like mongosh we absolutely do care about startup time so when consolidating telemetry we'd likely end up with the original structure eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason to think 3 seconds is negligible, but I'm changing the logic to send wait for 3 seconds asynchronously not blocking main flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm changing the logic to send wait for 3 seconds asynchronously not blocking main flow
not sure what you mean
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the creation logic does not be turned async.
I'd say the main change needed is to add
this.commonProperties.is_container_env
intialization intostart
(possibly rename it tosetup
as that naming seems confusing)