Skip to content

chore: reinstate telemetry/docker change after revert MCP-49 #339

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 5 commits into from
Jul 4, 2025
Merged

chore: reinstate telemetry/docker change after revert MCP-49 #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ea261a
chore: reinstate telemetry/docker change after revert (MCP-49)
fmenezes Jul 3, 2025
c5dbd61
fix: format
fmenezes Jul 3, 2025
dd6dadd
fix: tests
fmenezes Jul 3, 2025
8ac8ef3
chore: rename
fmenezes Jul 4, 2025
905959c
fix: comment
fmenezes Jul 4, 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
76 changes: 53 additions & 23 deletions src/telemetry/telemetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ 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;
Expand All @@ -17,8 +18,8 @@ export const DEVICE_ID_TIMEOUT = 3000;

export class Telemetry {
private isBufferingEvents: boolean = true;
/** Resolves when the device ID is retrieved or timeout occurs */
public deviceIdPromise: Promise<string> | undefined;
/** Resolves when the setup is complete or a timeout occurs */
public setupPromise: Promise<[string, boolean]> | undefined;
private deviceIdAbortController = new AbortController();
private eventCache: EventCache;
private getRawMachineId: () => Promise<string>;
Expand Down Expand Up @@ -48,33 +49,62 @@ export class Telemetry {
): Telemetry {
const instance = new Telemetry(session, userConfig, commonProperties, { eventCache, getRawMachineId });

void instance.start();
void instance.setup();
return instance;
}

private async start(): Promise<void> {
if (!this.isTelemetryEnabled()) {
return;
private async isContainerEnv(): Promise<boolean> {
Copy link
Preview

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using Promise.any instead of Promise.all when checking file existence to short-circuit on the first success and avoid unnecessary filesystem checks.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't have thought of this myself but that is a good point 🤖

if (process.platform !== "linux") {
return false; // we only support linux containers for now
}

if (process.env.container) {
return true;
}
this.deviceIdPromise = getDeviceId({
getMachineId: () => this.getRawMachineId(),
onError: (reason, error) => {
switch (reason) {
case "resolutionError":
logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error));
break;
case "timeout":
logger.debug(LogId.telemetryDeviceIdTimeout, "telemetry", "Device ID retrieval timed out");
break;
case "abort":
// No need to log in the case of aborts
break;

const exists = await Promise.all(
["/.dockerenv", "/run/.containerenv", "/var/run/.containerenv"].map(async (file) => {
try {
await fs.access(file);
return true;
} catch {
return false;
}
},
abortSignal: this.deviceIdAbortController.signal,
});
})
);

this.commonProperties.device_id = await this.deviceIdPromise;
return exists.includes(true);
}

private async setup(): Promise<void> {
if (!this.isTelemetryEnabled()) {
return;
}
this.setupPromise = Promise.all([
getDeviceId({
getMachineId: () => this.getRawMachineId(),
onError: (reason, error) => {
switch (reason) {
case "resolutionError":
logger.debug(LogId.telemetryDeviceIdFailure, "telemetry", String(error));
break;
case "timeout":
logger.debug(LogId.telemetryDeviceIdTimeout, "telemetry", "Device ID retrieval timed out");
break;
case "abort":
// No need to log in the case of aborts
break;
}
},
abortSignal: this.deviceIdAbortController.signal,
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we turn this abortcontroller into a generalized abort controller and use it with isContainerEnv as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds like a good improvement for a next PR

}),
this.isContainerEnv(),
]);

const [deviceId, containerEnv] = await this.setupPromise;

this.commonProperties.device_id = deviceId;
this.commonProperties.is_container_env = containerEnv;

this.isBufferingEvents = false;
}
Expand Down
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 @@ -66,6 +66,7 @@ export type CommonStaticProperties = {
*/
export type CommonProperties = {
device_id?: string;
is_container_env?: boolean;
mcp_client_version?: string;
mcp_client_name?: string;
config_atlas_auth?: TelemetryBoolSet;
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/telemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe("Telemetry", () => {
expect(telemetry.getCommonProperties().device_id).toBe(undefined);
expect(telemetry["isBufferingEvents"]).toBe(true);

await telemetry.deviceIdPromise;
await telemetry.setupPromise;

expect(telemetry.getCommonProperties().device_id).toBe(actualHashedId);
expect(telemetry["isBufferingEvents"]).toBe(false);
Expand Down
16 changes: 12 additions & 4 deletions tests/unit/telemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ describe("Telemetry", () => {
it("should send events successfully", async () => {
const testEvent = createTestEvent();

await telemetry.setupPromise;

await telemetry.emitEvents([testEvent]);

verifyMockCalls({
Expand All @@ -152,6 +154,8 @@ describe("Telemetry", () => {

const testEvent = createTestEvent();

await telemetry.setupPromise;

await telemetry.emitEvents([testEvent]);

verifyMockCalls({
Expand All @@ -175,6 +179,8 @@ describe("Telemetry", () => {
// Set up mock to return cached events
mockEventCache.getEvents.mockReturnValueOnce([cachedEvent]);

await telemetry.setupPromise;

await telemetry.emitEvents([newEvent]);

verifyMockCalls({
Expand All @@ -184,7 +190,9 @@ describe("Telemetry", () => {
});
});

it("should correctly add common properties to events", () => {
it("should correctly add common properties to events", async () => {
await telemetry.setupPromise;

const commonProps = telemetry.getCommonProperties();

// Use explicit type assertion
Expand Down Expand Up @@ -219,7 +227,7 @@ describe("Telemetry", () => {
expect(telemetry["isBufferingEvents"]).toBe(true);
expect(telemetry.getCommonProperties().device_id).toBe(undefined);

await telemetry.deviceIdPromise;
await telemetry.setupPromise;

expect(telemetry["isBufferingEvents"]).toBe(false);
expect(telemetry.getCommonProperties().device_id).toBe(hashedMachineId);
Expand All @@ -235,7 +243,7 @@ describe("Telemetry", () => {
expect(telemetry["isBufferingEvents"]).toBe(true);
expect(telemetry.getCommonProperties().device_id).toBe(undefined);

await telemetry.deviceIdPromise;
await telemetry.setupPromise;

expect(telemetry["isBufferingEvents"]).toBe(false);
expect(telemetry.getCommonProperties().device_id).toBe("unknown");
Expand Down Expand Up @@ -263,7 +271,7 @@ describe("Telemetry", () => {

jest.advanceTimersByTime(DEVICE_ID_TIMEOUT);

await telemetry.deviceIdPromise;
await telemetry.setupPromise;

expect(telemetry.getCommonProperties().device_id).toBe("unknown");
expect(telemetry["isBufferingEvents"]).toBe(false);
Expand Down
Loading