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

fmenezes · 2025-07-03T17:24:55Z

Proposed changes

changes were initially introduced at #298 which caused #320 and reverted at #330, I'm adding it back without any refactorings

Checklist

I have signed the MongoDB CLA

Copilot

Pull Request Overview

This PR reinstates the previously reverted telemetry and Docker environment detection changes without any additional refactorings.

Re-adds container environment detection alongside device ID retrieval
Updates telemetry types to include a new is_container_env property
Adjusts unit and integration tests to await the combined dataPromise instead of deviceIdPromise

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
tests/unit/telemetry.test.ts	Updated tests to `await telemetry.dataPromise` instead of `deviceIdPromise`
tests/integration/telemetry.test.ts	Updated integration test to await `telemetry.dataPromise`
src/telemetry/types.ts	Added new optional `is_container_env` field to `CommonProperties`
src/telemetry/telemetry.ts	Implemented `isContainerEnv` check, merged device ID and container flag into `dataPromise`

Comments suppressed due to low confidence (3)

src/telemetry/types.ts:69

Add a JSDoc comment to describe the purpose and possible values for the new is_container_env property, improving clarity for API consumers.

    is_container_env?: boolean;

src/telemetry/telemetry.ts:22

[nitpick] The name dataPromise is ambiguous. Consider renaming it to something more descriptive, like deviceDataPromise or initializationPromise.

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

src/telemetry/telemetry.ts:102

Add unit tests to verify that is_container_env is correctly set based on the detected environment, covering both container and non-container scenarios.

        this.commonProperties.is_container_env = containerEnv;

Copilot · 2025-07-03T17:27:04Z

src/telemetry/telemetry.ts

@@ -52,11 +53,32 @@ export class Telemetry {
        return instance;
    }

+    private async isContainerEnv(): Promise<boolean> {


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

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

coveralls · 2025-07-03T17:33:13Z

Pull Request Test Coverage Report for Build 16076959781

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

20 of 24 (83.33%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 74.187%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/telemetry/telemetry.ts	20	24	83.33%

Totals
Change from base Build 16021965229:	-0.1%
Covered Lines:	816
Relevant Lines:	1020

💛 - Coveralls

gagik

Nice, non-blocking subjective stylistic changes you can consider

gagik · 2025-07-04T08:43:14Z

src/telemetry/telemetry.ts

@@ -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 */


Suggested change

/** Resolves when the device ID is retrieved or timeout occurs */

/** Resolves when the setup is complete or a timeout occurs */

gagik · 2025-07-04T08:43:27Z

src/telemetry/telemetry.ts

@@ -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 */
-    public deviceIdPromise: Promise<string> | undefined;
+    public dataPromise: Promise<[string, boolean]> | undefined;


Suggested change

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

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

super nit, just may be a nicer name

gagik · 2025-07-04T08:44:00Z

src/telemetry/telemetry.ts

@@ -52,11 +53,32 @@ export class Telemetry {
        return instance;
    }

+    private async isContainerEnv(): Promise<boolean> {


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

src/telemetry/telemetry.ts

gagik · 2025-07-04T08:48:17Z

tests/unit/telemetry.test.ts

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


Suggested change

await telemetry.dataPromise;

await telemetry.setupPromise;

gagik · 2025-07-04T08:48:27Z

tests/unit/telemetry.test.ts

@@ -219,7 +227,7 @@ describe("Telemetry", () => {
                    expect(telemetry["isBufferingEvents"]).toBe(true);
                    expect(telemetry.getCommonProperties().device_id).toBe(undefined);

-                    await telemetry.deviceIdPromise;
+                    await telemetry.dataPromise;


Suggested change

await telemetry.dataPromise;

await telemetry.setupPromise;

gagik · 2025-07-04T08:48:32Z

tests/unit/telemetry.test.ts

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

-                    await telemetry.deviceIdPromise;
+                    await telemetry.dataPromise;


Suggested change

await telemetry.dataPromise;

await telemetry.setupPromise;

gagik · 2025-07-04T08:48:36Z

tests/unit/telemetry.test.ts

@@ -263,7 +271,7 @@ describe("Telemetry", () => {

                    jest.advanceTimersByTime(DEVICE_ID_TIMEOUT);

-                    await telemetry.deviceIdPromise;
+                    await telemetry.dataPromise;


Suggested change

await telemetry.dataPromise;

await telemetry.setupPromise;

gagik · 2025-07-04T08:51:39Z

src/telemetry/telemetry.ts

+
+        return exists.includes(true);
+    }
+
    private async start(): Promise<void> {


Suggested change

private async start(): Promise<void> {

private async setup(): Promise<void> {

gagik · 2025-07-04T08:53:16Z

src/telemetry/telemetry.ts

+                            break;
+                    }
+                },
+                abortSignal: this.deviceIdAbortController.signal,


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

sounds like a good improvement for a next PR

chore: reinstate telemetry/docker change after revert (MCP-49)

1ea261a

fmenezes marked this pull request as ready for review July 3, 2025 17:26

Copilot AI review requested due to automatic review settings July 3, 2025 17:26

fmenezes requested a review from a team as a code owner July 3, 2025 17:26

Copilot AI reviewed Jul 3, 2025

View reviewed changes

fix: format

c5dbd61

fix: tests

dd6dadd

gagik approved these changes Jul 4, 2025

View reviewed changes

gagik reviewed Jul 4, 2025

View reviewed changes

gagik changed the title ~~chore: reinstate telemetry/docker change after revert (MCP-49)~~ chore: reinstate telemetry/docker change after revert MCP-49 Jul 4, 2025

fmenezes added 2 commits July 4, 2025 16:27

chore: rename

8ac8ef3

fix: comment

905959c

fmenezes enabled auto-merge (squash) July 4, 2025 15:30

fmenezes merged commit 5b7ba55 into main Jul 4, 2025
16 of 17 checks passed

fmenezes deleted the MCP-49 branch July 4, 2025 15:34

	/** Resolves when the device ID is retrieved or timeout occurs */
	/** Resolves when the setup is complete or a timeout occurs */

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

	private async start(): Promise<void> {
	private async setup(): Promise<void> {

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

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

Uh oh!

Conversation

fmenezes commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16076959781

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

gagik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmenezes commented Jul 3, 2025 •

edited

Loading

coveralls commented Jul 3, 2025 •

edited

Loading