chadtmiller.com/knotless

Switch repository

Loading…

What changes

Each MCP tool call now logs a line, mcp tool, with the tool's name (gen_ai.tool.name), the caller's DID and duration_ms. It is at info, or at warn when slow and not asked to wait. mcp tool failed is as before, at warn. Both lines, and the calls and slow statements a tool makes, are now in the trace of the /mcp request. serve puts the trace on the request as an extension (telemetry::Trace), and the MCP server runs each tool in it. getLogs gains mcp tool among its messages' known values, and docs/observability.md says what the line holds.

Why

rmcp calls a tool in a task of its own, out of reach of the task-local trace. A slow /mcp request's trace held only its request line. On prod, one took 110 seconds, and its trace said nothing of which tool ran or what it waited on.

How it was checked

Left to CI: the MCP tests, which call tools that pass and fail, and the coverage gate.

Shortcomings

Request lines still log no query string, as the privacy policy promises.

Changeset: none

🤖 Generated with Claude Code

This hands the /mcp request's trace to the tool task through a telemetry::Trace request extension. It also logs an mcp tool line for every call, at warn when the call failed, or was slow and not asked to wait. The extension is set in serve's observe and read from rmcp's Parts, and trace_from(None) is the fallback when it's missing. The getLogs known value and docs/observability.md are updated to match. I found nothing that must change.

I read the diff only. The pull says its tests are left to CI, and I didn't check them. I couldn't tell whether the MCP tests send requests through serve's observe. If they don't, the extension-present branch is untested. It sits on one line, so the line-coverage gate shouldn't be affected.

The only change since my last review is in crates/knotless-mcp/src/http.rs. The map_or_else fallback becomes .map(..).unwrap_or(telemetry::trace_from(None)). Behaviour is the same: use the request's trace if the extension is present, else make a fresh one. unwrap_or is eager, so trace_from(None) now runs on every call, even when the extension is there. That is a small waste, not a bug. unwrap_or_else(|| telemetry::trace_from(None)) would avoid it. The commit message says the point was to drop an uncovered closure, so that is a trade-off I can accept. I found nothing that must change.

The only change since my last review adds Trace::or_new in telemetry.rs. It returns the given trace's id, or a new hex(16) id, and http.rs now calls it. This fixes the eager unwrap_or I mentioned, because a new id is only made when none was handed on. It matches what trace_from(None) did, since both end in hex(16). The new unit test traces_handed_on covers both branches. I found nothing that must change.