Expose MCP as toolset, and move prompts and handlers to shared package#20
Conversation
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
pkg/mcp/handlers.go
Outdated
|
|
||
| return mcp.NewToolResultStructured(output, string(jsonResult)), nil | ||
| return handlers.ExecuteRangeQueryHandler(ctx, promClient, handlers.RangeQueryInput{ | ||
| Query: req.GetString("query", ""), |
There was a problem hiding this comment.
I'm not a big fan of this duplication of param extraction. Couldn't we make the handlers.ExecuteRangeQueryHandle accept the api.ToolHandlerParams and here provide just the appropriate implementation for ToolCallRequest? It seems it only requires GetArguments to be implemented.
There was a problem hiding this comment.
We could still keep the wrapper for the output, to keep the ToMCPResult and ToToolsetResult, though to avoid some weird stuff with different output depending on which way one runs the mcp server, I would be for unifying the output as much as possible as well.
pkg/mcp/tools.go
Outdated
| The 'query' parameter MUST use metric names that were returned by list_metrics. | ||
| `), | ||
| mcp.WithDescription(prompts.ExecuteInstantQueryPrompt), | ||
| mcp.WithString("query", |
There was a problem hiding this comment.
Similarly with the duplication on the handlers params request. Could we use the toolets definitions as source of truth and use the WithRawInputSchema to convert it to the required format?
| @@ -1,2 +1,2 @@ | |||
| golang-version=1.24 | |||
| golang-version=1.25 | |||
There was a problem hiding this comment.
may be we can wait till ocp has go 1.25 else ci will fail
There was a problem hiding this comment.
Do you know when OCP will have go 1.25?
There was a problem hiding this comment.
I am still waiting for it for updating other downstream forks, probably by next week I hope
|
triggering the openshift ci to see its working |
|
@saswatamcode: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
|
|
||
| // All tool definitions as a single source of truth | ||
| var ( | ||
| ListMetrics = ToolDef{ |
There was a problem hiding this comment.
What about restructuring the packages a bit so that:
tooldefonly contains the types and generic functionality to define the tools (but nothing about specific tool instance).- rename the
pkg/handlerstopkg/tools/where handlers would be just part (but also added the tool definitions and request input mappings) - the definitions move to
pkg/tools/tools.go - the conversion functions from
bind.gocould move topkg/tools/handlers.go(IMO it would read easier, as it's just a transformation to prepare for the expected handler input). I could imagine other places, but the key part is all the tool-specific things insidepkg/toolspackage.
There was a problem hiding this comment.
also, the prompts could go to pkg/tools to have most of the real meat in one place, and the rest serving more as plumbing to serve it.
There was a problem hiding this comment.
Ok I just removed tooldefs and prompts, in favor of a centralized tools package with the core stuff
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iNecas, saswatamcode The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| restConfig.TLSClientConfig = tlsConfig | ||
|
|
||
| // Create HTTP client with Kubernetes authentication | ||
| rt, err := rest.TransportFor(restConfig) |
There was a problem hiding this comment.
@saswatamcode I am getting:
"failed to get label names: error fetching label names: Post "https://thanos-querier.openshift-monitoring.svc:9091/api/v1/labels": failed to make request: AccessControlRoundTripper failed to get kind for gvr /v1, Resource=labels: no matches for /v1, Resource=labels"
with some hacks I got beyond that.
The rest.TransportFor(...) call seems to inherit the wrapped AccessControlRoundTripper from the Kubernetes MCP server.
While the build-in prometheus client did build its on http.Client, which carries only bearer tokens and
TLS settings.
See: https://github.com/openshift/openshift-mcp-server/blob/main/pkg/prometheus/client.go#L140
No description provided.