-
Notifications
You must be signed in to change notification settings - Fork 1
feat: core telemetry functionality #87
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
note: this is a draft, will do some cleanup and change tests, looking for feedback on the code architecture |
Pull Request Test Coverage Report for Build 14622981460Details
💛 - 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.
Great start 🚀
src/config.ts
Outdated
version: SERVER_VERSION, | ||
mcpServerName: SERVER_NAME, |
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.
I'd move this to MACHINE_METADATA
and maybe get rid of individual global constants
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 use it in the client though, so wanted to keep it here where is common
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.
it's a bit confusing for me as it is inside a configuration, which to me means it is configurable
*/ | ||
export interface ToolEvent extends BaseEvent { | ||
properties: { | ||
command: string; |
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.
right now we use this as the toolname, we also have resources, I think this should contain "tool" | "resource" | "prompt"
and another one maybe called tool_name
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.
yes, we should have a category property where we can set "tool" but the idea is to keep command for all based on the tracking plan
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.
But how can you tell which is which?
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.
category: "tool"
command: "connect" | "list-projects" ...
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.
Awesome work 🚀 one last note about Telemetry being initialized in every tool that may be worth addressing
next: