-
Notifications
You must be signed in to change notification settings - Fork 111
chore: expose a hook to specify telemetry hosting mode MCP-166 #501
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we set the accepted strings here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we default to standalone until updated by vscode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🚀
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.