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 1 commit
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
33 changes: 29 additions & 4 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 { 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 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,12 +53,33 @@
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) => {

Check failure on line 65 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `⏎············`
try {

Check failure on line 66 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `····`
await fs.access(file);

Check failure on line 67 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Replace `················` with `····················`
return true;

Check failure on line 68 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `····`
} catch {

Check failure on line 69 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `····`
return false;

Check failure on line 70 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `····`
}

Check failure on line 71 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `····`
}));

Check failure on line 72 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Replace `········})` with `············})⏎········`

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({
this.dataPromise = Promise.all([getDeviceId({

Check failure on line 81 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `⏎············`
getMachineId: () => this.getRawMachineId(),

Check failure on line 82 in src/telemetry/telemetry.ts

View workflow job for this annotation

GitHub Actions / check-style 

Insert `····`
onError: (reason, error) => {
switch (reason) {
case "resolutionError":
Expand All @@ -72,9 +94,12 @@
}
},
abortSignal: this.deviceIdAbortController.signal,
});
}), this.isContainerEnv()]);

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

this.commonProperties.device_id = await this.deviceIdPromise;
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
6 changes: 3 additions & 3 deletions tests/unit/telemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,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 +235,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 +263,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