fix: pass telemetry status to MCP server VSCODE-763#1302
Conversation
There was a problem hiding this comment.
Pull request overview
This PR ensures the MongoDB MCP server inherits the extension’s telemetry enablement state so MCP telemetry is disabled whenever telemetry is disabled at the extension level.
Changes:
- Expose
TelemetryService.isTelemetryFeatureEnabled()(rename from the underscored method) and use it to gate MCP server telemetry. - Pass the extension’s
TelemetryServiceinstance intoMCPControllerand include atelemetry: 'enabled' | 'disabled'value in the MCP server config. - Add/adjust unit tests covering MCP telemetry configuration and the telemetry service rename.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/telemetry/telemetryService.ts | Renames telemetry enablement method and updates internal call site. |
| src/mcp/mcpController.ts | Accepts telemetryService and sets MCP telemetry config based on extension telemetry state. |
| src/mdbExtensionController.ts | Wires the extension’s TelemetryService into the MCPController constructor. |
| src/test/suite/telemetry/telemetryService.test.ts | Updates stubbing to match the renamed telemetry enablement method. |
| src/test/suite/mcp/mcpController.test.ts | Adds tests verifying MCP server config telemetry reflects telemetry enablement. |
| src/test/suite/mdbExtensionController.test.ts | Adds integration-style assertions that MCP uses the extension telemetry service and disables MCP telemetry in test mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { getMCPConfigFromVSCodeSettings } from './mcpConfig'; | ||
| import { DEFAULT_TELEMETRY_APP_NAME } from '../connectionController'; | ||
| import formatError from '../utils/formatError'; | ||
| import { TelemetryService } from '../telemetry'; |
There was a problem hiding this comment.
TelemetryService is only used as a type here, but it's imported as a value. In this codebase, type-only imports are typically written with import type (e.g., src/connectionController.ts:25). Consider switching this to a type-only import to avoid an unnecessary runtime dependency and keep imports consistent.
| import { TelemetryService } from '../telemetry'; | |
| import type { TelemetryService } from '../telemetry'; |
| // Checks user settings and extension running mode | ||
| // to determine whether or not we should track telemetry. | ||
| _isTelemetryFeatureEnabled(): boolean { | ||
| isTelemetryFeatureEnabled(): boolean { | ||
| // If tests run the extension we do not track telemetry. |
There was a problem hiding this comment.
PR description mentions passing TelemetryService.isTelemetryEnabled, but the implementation introduces/uses isTelemetryFeatureEnabled() instead. Please align the wording (or method naming) so it’s clear which API is intended and to avoid confusion for future maintainers.
Description
Passed
TelemetryService.isTelemetryEnableddown to MCP server to ensure we don't send telemetry if it's disabled at the extension level.Motivation and Context
Types of changes