chore: expose a hook to specify telemetry hosting mode MCP-166 #501
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR exposes a static hook in the Telemetry class to specify a hosting mode, allowing differentiation between standalone and vscode-hosted servers. This enables better categorization of telemetry data based on the runtime environment.
- Add static
hostingModeproperty to Telemetry class - Include
hosting_modein common telemetry properties when set - Add comprehensive test coverage for the new hosting mode functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/telemetry/telemetry.ts | Adds static hostingMode property and sets it in common properties during telemetry creation |
| src/telemetry/types.ts | Extends CommonProperties type to include optional hosting_mode field |
| tests/unit/telemetry.test.ts | Adds test coverage for hosting mode functionality and removes unnecessary test nesting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| eventCache?: EventCache; | ||
| commonProperties?: CommonProperties; | ||
| } = {} | ||
| ): Telemetry { |
There was a problem hiding this comment.
This assignment will throw a TypeError if commonProperties is undefined. The code should check if commonProperties exists or provide a default object before assignment.
Copilot uses AI. Check for mistakes.
Pull Request Test Coverage Report for Build 17437640718Details
💛 - Coveralls |
| config_atlas_auth?: TelemetryBoolSet; | ||
| config_connection_string?: TelemetryBoolSet; | ||
| session_id?: string; | ||
| hosting_mode?: string; |
There was a problem hiding this comment.
nit: can we set the accepted strings here?
There was a problem hiding this comment.
The MCP server does not know them at this point - essentially, I want this to be a field populated by the application that hosts the server and we don't know beforehand what these applications are.
src/telemetry/telemetry.ts
Outdated
| }; | ||
|
|
||
| export class Telemetry { | ||
| public static hostingMode?: string; |
There was a problem hiding this comment.
should we default to standalone until updated by vscode?
There was a problem hiding this comment.
We could, though we would need to treat empty and standalone as identical for historical purposes.
* main: feat: add more details about atlas connect flow - MCP-124 (#500) chore: extend library interfaces to allow injecting a custom connection error handler MCP-132 (#502) fix: start mcp even if connection fails - [MCP-140] (#503) fix: allow connect tool on readOnly mode (#499) chore: warn about the usage of deprecated cli arguments MCP-107 (#493) ci: add ipAccessList after creating project (#496)
4 participants
Proposed changes
It's best to review this with whitespace disabled since I removed an unnecessary nesting in the telemetry tests. This hook will be used by vscode to set the "hostingMode" so that we can differentiate between standalone and vscode-hosted servers.