Skip to content

chore: [MCP-2] add is_container_env to telemetry #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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
125 changes: 73 additions & 52 deletions src/telemetry/telemetry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,55 +7,91 @@ 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await fs.stat(filePath);
await fs.access(filePath);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't need read access

Copy link
Collaborator Author

@fmenezes fmenezes Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since these files are controlled by docker container I'm not too sure what happens when using non root / non default user over docker (meaning docker run -u <some user>)

Copy link
Collaborator

@gagik gagik Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without read access without permission check access, we'd not be able to check for permission stat either.stat also gives you stats which we're discarding, access is lighter in that sense.

Node Docs example uses access to check for file existance:
https://nodejs.org/api/fs.html#fs_fs_access_path_mode_callback

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
}
}

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 deviceIdPromise: Promise<string> | undefined;
private containerEnvPromise: Promise<boolean> | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably turn this into a single setup promise with Promise.all or something, see below:

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's usually always cleaner to use async function then wrap them in finally-s etc. I'd revert this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idea here was to start the promises and not wait for them to finish

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be the benefit of that? we're already spawning an async function without waiting for it to finish (with void above) so this isn't blocking the main thread.

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({
Copy link
Collaborator

@gagik gagik Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are probably looking for Promise.all (or Promise.allSettled)?

getMachineId: () => this.getRawMachineId(),
onError: (reason, error) => {
Expand All @@ -73,15 +109,11 @@ export class Telemetry {
},
abortSignal: this.deviceIdAbortController.signal,
});

this.commonProperties.device_id = await this.deviceIdPromise;

this.isBufferingEvents = false;
this.containerEnvPromise = this.getContainerEnv();
Copy link
Collaborator

@gagik gagik Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.containerEnvPromise = this.getContainerEnv();
const [deviceId, isContainerEnv] = await Promise.all([this.deviceIdPromise, this.getContainerEnvPromise]);
this.commonProperties.device_id = deviceId;
this.commonProperties.is_container_env = isContainerEnv;

something like this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this approach start would take up to 3s which I think it's acceptable, will change

Copy link
Collaborator

@gagik gagik Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will have identical behavior to awaiting inside the getCommonProperties, we do not await the start() function anywhere so it's essentially same as declaring a combined promise, we instantly initialize the telemetry and start buffering event when emit is run.

In both cases we'd instantly be able to emit events, and in both cases if device ID takes 3 seconds, the eventual emission of events will take 3 seconds.

But with start there's only 1 point where we deal with asynchronous logic, and in the other case we have a bunch of less predictable asyncronous functions listening in into 2 different promises.

}

public async close(): Promise<void> {
this.deviceIdAbortController.abort();
this.isBufferingEvents = false;
await this.emitEvents(this.eventCache.getEvents());
}

Expand All @@ -106,14 +138,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",
Copy link
Preview

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
is_container_env: (await this.containerEnvPromise) ? "true" : "false",
is_container_env: await this.containerEnvPromise,

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

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"

device_id: await this.deviceIdPromise,
Copy link
Collaborator

@gagik gagik Jun 16, 2025

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)

Copy link
Collaborator Author

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.

Copy link
Collaborator

@gagik gagik Jun 16, 2025

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).

};
}

Expand All @@ -139,11 +173,6 @@ export class Telemetry {
* Falls back to caching if both attempts fail
*/
private async emit(events: BaseEvent[]): Promise<void> {
if (this.isBufferingEvents) {
this.eventCache.appendEvents(events);
return;
}

const cachedEvents = this.eventCache.getEvents();
const allEvents = [...cachedEvents, ...events];

Expand All @@ -153,42 +182,34 @@ export class Telemetry {
`Attempting to send ${allEvents.length} events (${cachedEvents.length} cached)`
);

const result = await this.sendEvents(this.session.apiClient, allEvents);
if (result.success) {
try {
await this.sendEvents(this.session.apiClient, allEvents);
this.eventCache.clearEvents();
logger.debug(
LogId.telemetryEmitSuccess,
"telemetry",
`Sent ${allEvents.length} events successfully: ${JSON.stringify(allEvents, null, 2)}`
);
return;
} catch (error: unknown) {
logger.debug(
LogId.telemetryEmitFailure,
"telemetry",
`Error sending event to client: ${error instanceof Error ? error.message : String(error)}`
);
this.eventCache.appendEvents(events);
}

logger.debug(
LogId.telemetryEmitFailure,
"telemetry",
`Error sending event to client: ${result.error instanceof Error ? result.error.message : String(result.error)}`
);
this.eventCache.appendEvents(events);
}

/**
* Attempts to send events through the provided API client
*/
private async sendEvents(client: ApiClient, events: BaseEvent[]): Promise<EventResult> {
try {
await client.sendEvents(
events.map((event) => ({
...event,
properties: { ...this.getCommonProperties(), ...event.properties },
}))
);
return { success: true };
} catch (error) {
return {
success: false,
error: error instanceof Error ? error : new Error(String(error)),
};
}
private async sendEvents(client: ApiClient, events: BaseEvent[]): Promise<void> {
const commonProperties = await this.getCommonProperties();
await client.sendEvents(
events.map((event) => ({
...event,
properties: { ...commonProperties, ...event.properties },
}))
);
}
}
1 change: 1 addition & 0 deletions src/telemetry/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,5 @@ export type CommonProperties = {
config_atlas_auth?: TelemetryBoolSet;
config_connection_string?: TelemetryBoolSet;
session_id?: string;
is_container_env?: TelemetryBoolSet;
} & CommonStaticProperties;
28 changes: 0 additions & 28 deletions tests/integration/telemetry.test.ts

This file was deleted.

Loading
Loading