Skip to content

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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Jun 19, 2025
Merged

chore: add is_container_env to telemetry MCP-2 #298

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a480c94
MCP-2: add is_container_env to telemetry
fmenezes Jun 13, 2025
126994c
fix: tests
fmenezes Jun 13, 2025
bf204bb
fix: tests
fmenezes Jun 13, 2025
3d91b40
fix: tests
fmenezes Jun 13, 2025
fe64649
fix: tests
fmenezes Jun 13, 2025
f73c208
fix: cleanup
fmenezes Jun 13, 2025
671bbeb
fix: check
fmenezes Jun 13, 2025
c2a1246
fix: cleanup
fmenezes Jun 13, 2025
678beee
fix: address comments
fmenezes Jun 16, 2025
8d53eea
fix: address comments
fmenezes Jun 16, 2025
95b0b06
fix: merge getCommonProperties and getAsyncCommonProperties
fmenezes Jun 16, 2025
911fdcb
fix: tests
fmenezes Jun 16, 2025
4703628
fix: parallel promises
fmenezes Jun 16, 2025
931816f
fix: remove counters
fmenezes Jun 16, 2025
0b60fe2
fix: address comments
fmenezes Jun 16, 2025
6ba2346
fix: address comments
fmenezes Jun 16, 2025
f4bfbf0
move async into closer to sending
fmenezes Jun 19, 2025
3424e0f
fix
fmenezes Jun 19, 2025
83fd1d9
fix close
fmenezes Jun 19, 2025
7e3cdb1
fix test
fmenezes Jun 19, 2025
c67877e
fix test
fmenezes Jun 19, 2025
814ccb9
fix lint
fmenezes Jun 19, 2025
1dba800
parallel and test
fmenezes Jun 19, 2025
a0c208c
fix: lint
fmenezes Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 73 additions & 52 deletions src/telemetry/telemetry.ts
View file
Open in desktop
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);
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

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 •
edited
Loading

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 •
edited
Loading

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
View file
Open in desktop
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
View file
Open in desktop

This file was deleted.

Loading
Loading