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 3 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
70 changes: 50 additions & 20 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 @@ -18,7 +19,7 @@ export const DEVICE_ID_TIMEOUT = 3000;
export class Telemetry {
private isBufferingEvents: boolean = true;
/** Resolves when the device ID is retrieved or timeout occurs */
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
/** Resolves when the device ID is retrieved or timeout occurs */
/** Resolves when the setup is complete or a timeout occurs */

public deviceIdPromise: Promise<string> | undefined;
public dataPromise: Promise<[string, 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.

Suggested change
public dataPromise: Promise<[string, boolean]> | undefined;
public setupPromise: Promise<[string, boolean]> | undefined;

super nit, just may be a nicer name

private deviceIdAbortController = new AbortController();
private eventCache: EventCache;
private getRawMachineId: () => Promise<string>;
Expand Down Expand Up @@ -52,29 +53,58 @@ export class Telemetry {
return instance;
}

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

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

return exists.includes(true);
}

private async start(): Promise<void> {
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
private async start(): Promise<void> {
private async setup(): Promise<void> {

if (!this.isTelemetryEnabled()) {
return;
}
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;
}
},
abortSignal: this.deviceIdAbortController.signal,
});

this.commonProperties.device_id = await this.deviceIdPromise;
this.dataPromise = 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.dataPromise;

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.dataPromise;

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.dataPromise;

await telemetry.emitEvents([testEvent]);

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

const testEvent = createTestEvent();

await telemetry.dataPromise;

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.dataPromise;

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.dataPromise;
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 telemetry.dataPromise;
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.dataPromise;
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 telemetry.dataPromise;
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.dataPromise;
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 telemetry.dataPromise;
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.dataPromise;
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 telemetry.dataPromise;
await telemetry.setupPromise;


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