Skip to content
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

feat: rewrite MCP server in Go #29

Merged
merged 35 commits into from
Mar 12, 2025
Merged

feat: rewrite MCP server in Go #29

merged 35 commits into from
Mar 12, 2025

Conversation

sd2k
Copy link
Collaborator

@sd2k sd2k commented Mar 10, 2025

This PR removes the existing Python implementation of the MCP server and adds a Go version.

The server can be started in either stdio or SSE mode based on the arguments. We still need to add some more flags for e.g. host and port of SSE mode.

Tools are factored into a separate package which can be imported from elsewhere. Using a similar method to the mcp-golang SDK, we use reflection to inspect the types of the functions and generate tool definitions, used for the list_tools functionality.

TODO:

  • add flags for host/port when running SSE mode
  • update README
  • add/update a CONTRIBUTING doc which describes how to add new tools
  • re-add some integration tests (using docker-compose) and possibly cloud tests (using the mcptests Grafana cloud dev instance)
  • update CI to stop running Python tests
  • document why we're using a fork of mcp-go
  • rename client.go to something more appropriate

Maybe later?

  • add a way to disable certain categories of tools

@sd2k sd2k requested a review from csmarchbanks March 10, 2025 17:07
@sd2k sd2k added the enhancement New feature or request label Mar 10, 2025
This will allow tools such as go build/install to know the name of the
generated binary, and allow additional binaries in the future.
@sd2k
Copy link
Collaborator Author

sd2k commented Mar 11, 2025

To make the migration easier for users I was hoping we could continue with our Python package and shove a platform-specific Go binary inside the package wheel, and use a tiny main.py which forwards to the Go executable. I think we can do that in a future PR though.

@csmarchbanks
Copy link
Contributor

I think that can be a future PR, and I am not sure if it is really necessary either. If we can have easy instructions for installing the go binary then having a Python wrapper would just make things more complicated.

@sd2k sd2k marked this pull request as ready for review March 11, 2025 21:59
@sd2k sd2k requested a review from a team as a code owner March 11, 2025 21:59
Turns out that we need v1/ as a suffix, with the training slash, in the
Incident URL for the client to work. I added both a cloud test that can
be run locally to make sure everything works, as well as a unit test to
test this specific issue.
Copy link
Contributor

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed one small issue with incident not working, but otherwise looks good. If my most recent commit looks ok let's get this in!

csmarchbanks and others added 4 commits March 11, 2025 21:13
List incidents and list datasources can use a lot more context than
necessary when returning results. For dashboards use a custom struct
that only returns a few fields. For incidents use the preview incident
endpoint which returns a more concise format.
Limit the amount of data returned for list tools
@sd2k sd2k enabled auto-merge March 12, 2025 09:19
@sd2k sd2k merged commit 87de73d into main Mar 12, 2025
3 checks passed
@sd2k sd2k deleted the golang branch March 12, 2025 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants