-
Notifications
You must be signed in to change notification settings - Fork 117
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 12 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,55 +7,97 @@ 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.stat(filePath); | ||||||
return true; // File exists | ||||||
himanshusinghs marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
} 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; | ||||||
} | ||||||
for (const file of ["/.dockerenv", "/run/.containerenv", "/var/run/.containerenv"]) { | ||||||
|
||||||
const exists = await fileExists(file); | ||||||
if (exists) { | ||||||
return true; | ||||||
} | ||||||
} | ||||||
|
||||||
export const DEVICE_ID_TIMEOUT = 3000; | ||||||
return false; | ||||||
} | ||||||
|
||||||
export class Telemetry { | ||||||
private isBufferingEvents: boolean = true; | ||||||
/** Resolves when the device ID is retrieved or timeout occurs */ | ||||||
public deviceIdPromise: Promise<string> | undefined; | ||||||
private pendingPromises: number = 2; | ||||||
private deviceIdPromise: Promise<string> | undefined; | ||||||
private containerEnvPromise: Promise<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( | ||||||
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 }); | ||||||
const instance = new Telemetry(session, userConfig, { | ||||||
eventCache, | ||||||
getRawMachineId, | ||||||
getContainerEnv, | ||||||
}); | ||||||
|
||||||
void instance.start(); | ||||||
instance.start(); | ||||||
return instance; | ||||||
} | ||||||
|
||||||
private async start(): Promise<void> { | ||||||
private start(): void { | ||||||
|
||||||
if (!this.isTelemetryEnabled()) { | ||||||
return; | ||||||
} | ||||||
|
||||||
this.deviceIdPromise = getDeviceId({ | ||||||
|
||||||
getMachineId: () => this.getRawMachineId(), | ||||||
onError: (reason, error) => { | ||||||
|
@@ -72,16 +114,16 @@ export class Telemetry { | |||||
} | ||||||
}, | ||||||
abortSignal: this.deviceIdAbortController.signal, | ||||||
}).finally(() => { | ||||||
this.pendingPromises--; | ||||||
}); | ||||||
this.containerEnvPromise = this.getContainerEnv().finally(() => { | ||||||
this.pendingPromises--; | ||||||
}); | ||||||
|
||||||
this.commonProperties.device_id = await this.deviceIdPromise; | ||||||
|
||||||
this.isBufferingEvents = false; | ||||||
} | ||||||
|
||||||
public async close(): Promise<void> { | ||||||
this.deviceIdAbortController.abort(); | ||||||
this.isBufferingEvents = false; | ||||||
await this.emitEvents(this.eventCache.getEvents()); | ||||||
} | ||||||
|
||||||
|
@@ -106,14 +148,16 @@ export class Telemetry { | |||||
* Gets the common properties for events | ||||||
* @returns Object containing common properties for all events | ||||||
*/ | ||||||
public getCommonProperties(): CommonProperties { | ||||||
private async getCommonProperties(): Promise<CommonProperties> { | ||||||
return { | ||||||
...this.commonProperties, | ||||||
...MACHINE_METADATA, | ||||||
mcp_client_version: this.session.agentRunner?.version, | ||||||
mcp_client_name: this.session.agentRunner?.name, | ||||||
session_id: this.session.sessionId, | ||||||
config_atlas_auth: this.session.apiClient.hasCredentials() ? "true" : "false", | ||||||
config_connection_string: this.userConfig.connectionString ? "true" : "false", | ||||||
is_container_env: (await this.containerEnvPromise) ? "true" : "false", | ||||||
|
is_container_env: (await this.containerEnvPromise) ? "true" : "false", | |
is_container_env: await this.containerEnvPromise, |
Copilot uses AI. Check for mistakes.
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.
not compatible TelemetryBoolSet expects strings "true" or "false"
Outdated
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 don't think we should be awaiting here; an explicit unawaited + buffering with an async start like it was before is easier to reason about, it just needs another to await for container inside start
Otherwise there's potentially hundreds of floating functions awaiting device_id or whatever else. We can assume in worst case scenario this isn't instant (and could never even resolve)
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.
on the device ID there is a timeout, on the container env it is just fs/promises
so we can be sure they will resolve. These promises are private, I think the buferring logic only makes sense if we are tagging on finally or something, to me feels unneeded complication. Our current timeout is 3 seconds, no reason to be extra defensive.
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.
it's fair that we don't need to be extra defensive but either way we're still essentially going to end up buffering these events.
The difference is that with awaiting here the "buffering" would happen through a bunch of async functions in parallel in memory stack waiting on these promises as opposed to more explicit buffering where we store the functions we want to execute ourselves and execute them in order after resolving these promises in one place.
With awaiting in common properties approach we'd, for example, not have any guarantees that these functions end up resolving in the same order. Which we don't need to care about, but it's nice to have better idea of how async execution is going to happen and have 1 point of logic where we resolve all the async issues.
With discussions with @nirinchev, we also want to create a shared telemetry service component across devtools so we would not want to deviate too much unless there are good reasons for it (this setup is modeled after mongosh and Compass telemetry).
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.