chore: [MCP-2] add is_container_env to telemetry #298
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,30 +7,69 @@ 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"; | ||||||
|
|
||||||
| type EventResult = { | ||||||
| success: boolean; | ||||||
| error?: Error; | ||||||
| }; | ||||||
|
|
||||||
| async function fileExists(filePath: string): Promise<boolean> { | ||||||
| try { | ||||||
| await fs.stat(filePath); | ||||||
| return true; // File exists | ||||||
himanshusinghs marked this conversation as resolved.
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 | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| async function isContainerized(): Promise<boolean> { | ||||||
| for (const file of ["/.dockerenv", "/run/.containerenv", "/var/run/.containerenv"]) { | ||||||
|
There was a problem hiding this comment. do the |
||||||
| const exists = await fileExists(file); | ||||||
| if (exists) { | ||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
| return !!process.env.container; | ||||||
|
There was a problem hiding this comment. This is fairly minor as this is clearly not on a hot path, but would it make sense to first check for the env variable since that is cheapest? And then for the files, we can check them in parallel rather than 1 by 1. |
||||||
| } | ||||||
|
|
||||||
| export class Telemetry { | ||||||
| /** Resolves when the device ID is retrieved or timeout occurs */ | ||||||
| private bufferingEvents: number = 2; | ||||||
|
There was a problem hiding this comment. This name no longer makes much sense. How about |
||||||
| public deviceIdPromise: Promise<string> | undefined; | ||||||
| public 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, | ||||||
| getContainerEnv, | ||||||
| }: { | ||||||
| eventCache: EventCache; | ||||||
| getRawMachineId: () => Promise<string>; | ||||||
| getContainerEnv: () => Promise<boolean>; | ||||||
| } | ||||||
| ) { | ||||||
| this.eventCache = eventCache; | ||||||
| this.getRawMachineId = getRawMachineId; | ||||||
| this.getContainerEnv = getContainerEnv; | ||||||
| } | ||||||
|
|
||||||
| static create( | ||||||
|
|
@@ -40,22 +79,29 @@ export class Telemetry { | |||||
| commonProperties = { ...MACHINE_METADATA }, | ||||||
| eventCache = EventCache.getInstance(), | ||||||
| getRawMachineId = () => nodeMachineId.machineId(true), | ||||||
| getContainerEnv = isContainerized, | ||||||
| }: { | ||||||
| commonProperties?: CommonProperties; | ||||||
| eventCache?: EventCache; | ||||||
| getRawMachineId?: () => Promise<string>; | ||||||
| getContainerEnv?: () => Promise<boolean>; | ||||||
| } = {} | ||||||
| ): Telemetry { | ||||||
| const instance = new Telemetry(session, userConfig, commonProperties, { | ||||||
| eventCache, | ||||||
| getRawMachineId, | ||||||
| getContainerEnv, | ||||||
| }); | ||||||
|
|
||||||
| instance.start(); | ||||||
| return instance; | ||||||
| } | ||||||
|
|
||||||
| private start(): void { | ||||||
|
There was a problem hiding this comment. it's usually always cleaner to use async function then wrap them in There was a problem hiding this comment. idea here was to start the promises and not wait for them to finish There was a problem hiding this comment. what would be the benefit of that? we're already spawning an async function without waiting for it to finish (with We'd want to buffer events beforehand so we can make sure telemetry stuff we send is with device_id resolved if possible |
||||||
| if (!this.isTelemetryEnabled()) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| this.deviceIdPromise = getDeviceId({ | ||||||
|
There was a problem hiding this comment. you are probably looking for Promise.all (or Promise.allSettled)? |
||||||
| getMachineId: () => this.getRawMachineId(), | ||||||
| onError: (reason, error) => { | ||||||
|
|
@@ -72,16 +118,16 @@ export class Telemetry { | |||||
| } | ||||||
| }, | ||||||
| abortSignal: this.deviceIdAbortController.signal, | ||||||
| }).finally(() => { | ||||||
| this.bufferingEvents--; | ||||||
| }); | ||||||
| this.containerEnvPromise = this.getContainerEnv().finally(() => { | ||||||
| this.bufferingEvents--; | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| public async close(): Promise<void> { | ||||||
| this.deviceIdAbortController.abort(); | ||||||
| await this.emitEvents(this.eventCache.getEvents()); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -117,6 +163,14 @@ export class Telemetry { | |||||
| }; | ||||||
| } | ||||||
|
|
||||||
| public async getAsyncCommonProperties(): Promise<CommonProperties> { | ||||||
|
There was a problem hiding this comment. I don't have super strong feelings here, but I'm not a huge fan of making methods public just for the sake of tests. While I agree it's convenient to test the internals here, this and |
||||||
| return { | ||||||
| ...this.getCommonProperties(), | ||||||
| is_container_env: (await this.containerEnvPromise) ? "true" : "false", | ||||||
|
There was a problem hiding this comment. [nitpick] Consider returning a boolean for is_container_env in getAsyncCommonProperties (e.g., true/false) instead of a string to more directly reflect its boolean nature, if this change is compatible with TelemetryBoolSet.
Suggested change
Copilot uses AI. Check for mistakes. There was a problem hiding this comment. not compatible TelemetryBoolSet expects strings "true" or "false" |
||||||
| device_id: await this.deviceIdPromise, | ||||||
|
There was a problem hiding this comment. 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 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. on the device ID there is a timeout, on the container env it is just There was a problem hiding this comment. 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). |
||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Checks if telemetry is currently enabled | ||||||
| * This is a method rather than a constant to capture runtime config changes | ||||||
|
|
@@ -134,12 +188,16 @@ export class Telemetry { | |||||
| return !doNotTrack; | ||||||
| } | ||||||
|
|
||||||
| public isBufferingEvents(): boolean { | ||||||
| return this.bufferingEvents > 0; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Attempts to emit events through authenticated and unauthenticated clients | ||||||
| * Falls back to caching if both attempts fail | ||||||
| */ | ||||||
| private async emit(events: BaseEvent[]): Promise<void> { | ||||||
| if (this.isBufferingEvents()) { | ||||||
| this.eventCache.appendEvents(events); | ||||||
| return; | ||||||
| } | ||||||
|
|
@@ -177,10 +235,11 @@ export class Telemetry { | |||||
| */ | ||||||
| private async sendEvents(client: ApiClient, events: BaseEvent[]): Promise<EventResult> { | ||||||
| try { | ||||||
| const commonProperties = await this.getAsyncCommonProperties(); | ||||||
| await client.sendEvents( | ||||||
| events.map((event) => ({ | ||||||
| ...event, | ||||||
| properties: { ...commonProperties, ...event.properties }, | ||||||
| })) | ||||||
| ); | ||||||
| return { success: true }; | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.