-
Notifications
You must be signed in to change notification settings - Fork 62
feat(mcp): log tool call (function name + arguments) #194
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Marc Nuri <[email protected]>
@@ -165,3 +167,10 @@ func contextFunc(ctx context.Context, r *http.Request) context.Context { | |||
|
|||
return ctx | |||
} | |||
|
|||
func toolCallLoggingMiddleware(next server.ToolHandlerFunc) server.ToolHandlerFunc { | |||
return func(ctx context.Context, ctr mcp.CallToolRequest) (*mcp.CallToolResult, error) { |
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.
Based on this commit mark3labs/mcp-go@baa7153, CallToolRequest
contains headers. Would you think it would be useful logging some of the values in here like user-agent, etc.?. Would it be useful for us or unnecessary noise?
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.
Maybe with more verbosity, similar to kube-ctl.
- log tool call with 5
- log request headers with 7
But I'm not even sure if this would be that useful for users
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.
Actually this middleware
func RequestMiddleware(next http.Handler) http.Handler { |
In which scenario this middleware will be useful?. Request is seen in the audit logs and verifying that it actually results in tool call?
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.
Actually this middleware...
This works for HTTP but not for STDIO (for example).
IMO is good to have both and also keep them separate.
An HTTP call might eventually become a tool call or not, especially now that we're working on auth (e.g. we get an HTTP request but auth blocks it).
In which scenario this middleware will be useful?.
The user who made the request was building an MCP proxy and wasn't able to see any effects on the MCP server. They wanted to know if the tool was actually being called.
There are multiple places where the calls can be logged (MCP Client, MCP Host, etc.), I guess that this is yet another tracing point.
I understand it can be interesting to trace where the tool call chain was broken.
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.
Good points and make sense to me.
Fixes #174