Rate limiting observability (metrics and tracing) PR B

docs/middleware.md

@@ -63,7 +63,7 @@ Example config files:
 ### Known limitations
 
 - **Stale Modern headers.** A mutating webhook patches only the JSON body, so after it renames a tool the `Mcp-Method`/`Mcp-Name` headers forwarded to the backend still describe the original name. `ValidateHeaderConsistency` (`pkg/mcp/revision.go:512`) requires the header and body to agree and returns a `RequestHeaderMismatchError` (`CodeHeaderMismatch`) if they don't, but today that check is only wired into vMCP (`pkg/vmcp/server/classification.go:72`), which never runs the mutating webhook — so nothing in ToolHive catches this yet. A spec-conformant Modern (2026-07-28) backend will reject the mismatched request itself, so this fails closed rather than silently mis-authorizing. Practical guidance: a mutating webhook must not rename tools on the Modern path.
-- **Controls outside the parser see the pre-mutation request.** The tool-call filter (`pkg/mcp/tool_filter.go`) reads the raw request body directly, and the rate limiter runs before the mutating webhook in the chain (see the ordering rules below) — both decide against the request as received, not as mutated. A mutating webhook can therefore rename a call into a tool that `--tools` filtering excluded, and the rate limiter debits the bucket for the requested tool rather than the executed one. This is a known gap, not a regression: the tool filter's position ahead of the MCP parser is deliberate (it needs the raw request), as already noted in the ordering rules below.
+- **Controls evaluated before mutation see the pre-mutation request.** The tool-call filter (`pkg/mcp/tool_filter.go`) reads the raw request body before parsing, and the rate limiter runs after parsing but before the mutating webhook (see the ordering rules below). Both decide against the request as received, not as mutated. A mutating webhook can therefore rename a call into a tool that `--tools` filtering excluded, and the rate limiter debits the bucket for the requested tool rather than the executed one. This is a known gap, not a regression: filtering must happen before excluded calls proceed, and rate limiting intentionally rejects excess traffic before making webhook round trips.
 
 ## Architecture Diagram
 
@@ -1055,11 +1055,11 @@ The middleware chain execution order is critical and controlled by the order in
 5. **Tool Filter Middleware** (if enabled) - Filters available tools in list responses
 6. **Tool Call Filter Middleware** (if enabled) - Filters tool call requests
 7. **MCP Parser Middleware** (always present) - Parses JSON-RPC MCP requests
-8. **Rate Limit Middleware** (if configured) - Enforces per-identity/tool limits using the parsed request
-9. **Mutating Webhook Middleware** (if configured) - Patches the parsed MCP request and republishes the parse
-10. **Validating Webhook Middleware** (if configured) - Approves or denies the (possibly mutated) request
-11. **Usage Metrics Middleware** (if enabled) - Tracks tool call counts
-12. **Telemetry Middleware** (if enabled) - OpenTelemetry instrumentation
+8. **Telemetry Middleware** (if enabled) - Starts the request span; finalizes MCP identity after the inner chain returns
+9. **Rate Limit Middleware** (if configured) - Enforces per-identity/tool limits using the parsed request
+10. **Mutating Webhook Middleware** (if configured) - Patches the parsed MCP request and republishes the parse
+11. **Validating Webhook Middleware** (if configured) - Approves or denies the (possibly mutated) request
+12. **Usage Metrics Middleware** (if enabled) - Tracks tool call counts
 13. **Authorization Middleware** (if enabled) - Cedar policy evaluation
 14. **Header Forward Middleware** (if configured for remote servers) - Injects custom headers
 15. **Recovery Middleware** (always present) - Catches panics
@@ -1072,8 +1072,10 @@ The middleware chain execution order is critical and controlled by the order in
 - Token Exchange must come after Upstream Swap if both are used (can further transform the upstream IdP token)
 - Tool filters should come before MCP Parser to operate on raw requests
 - MCP Parser must come before Authorization (provides structured MCP data)
+- Telemetry must come before Rate Limiting so the limiter can annotate the active request span; telemetry finalizes MCP names, attributes, and metrics after downstream processing from the shared parsed-request holder
+- Rate Limiting must come before webhooks so rejected traffic does not invoke outbound webhook requests
 - Mutating webhooks must come before Validating webhooks and before Authorization, so policy evaluation sees the patched request
-- Middleware that rewrites the request body must republish the parsed request via `mcp.RepublishParsedMCPRequest` and refresh `r.ContentLength` — `ParsingMiddleware` deliberately parses only once, so every later consumer (authorization, audit, telemetry, usage metrics) reads the cached parse rather than re-reading the body
+- Middleware that rewrites the request body must republish the parsed request via `mcp.RepublishParsedMCPRequest` and refresh `r.ContentLength` — `ParsingMiddleware` deliberately parses only once, so downstream consumers read the refreshed context while outer audit and telemetry wrappers read the refreshed `mcp.ParsedRequestHolder` after the inner chain returns
 - Header Forward executes close to the backend handler (innermost position)
 - Recovery is always last in config, making it the innermost wrapper (the chain wraps in reverse config order, so the first entry is the outermost and runs first)
 - Body-size limit and Origin validation stay OUTSIDE audit: oversized bodies must be rejected before audit buffers request data, and origin validation is a pre-auth DNS-rebind guard. Their rejections (413/403) are the only ones not audited.
@@ -1115,4 +1117,4 @@ export LOG_LEVEL=debug
 thv run --transport sse --name my-server my-image:latest
 ```
 
-This will show detailed information about each middleware component's execution and data flow.
+This will show detailed information about each middleware component's execution and data flow.

docs/observability.md

@@ -340,6 +340,33 @@ The `mcp.resource.uri` attribute is set only for the following methods:
 `resources/read`, `resources/subscribe`, `resources/unsubscribe`,
 `notifications/resources/updated`.
 
+### Rate Limit Attributes
+
+Redis-backed rate limit checks annotate the existing request span; they do not
+create a separate span. Normal allowed and rejected outcomes set all three
+attributes below.
+
+| Attribute | Type | Description |
+|-----------|------|-------------|
+| `rate_limit.decision` | string | `"allowed"` or `"rejected"` |
+| `rate_limit.rejected_by` | string | `"none"` for allowed requests, otherwise the bucket that rejected the request |
+| `rate_limit.fail_open` | bool | `false` for normal allowed and rejected outcomes |
+
+The bounded `rate_limit.rejected_by` values are:
+
+| Value | Limiting bucket |
+|-------|-----------------|
+| `shared_server` | Server-wide shared limit |
+| `shared_tool` | Tool-specific shared limit |
+| `per_user_server` | Server-wide per-user limit |
+| `per_user_tool` | Tool-specific per-user limit |
+
+When no configured bucket applies to a tool call, the span records
+`rate_limit.decision="allowed"`, `rate_limit.rejected_by="none"`, and
+`rate_limit.fail_open=false`. Redis check failures do not receive these normal
+outcome attributes. If multiple rate limit checks use the same request span,
+the latest normal outcome replaces earlier values.
+
 ### Tool, Prompt, and Resource Attributes
 
 **For `tools/call`:**

pkg/mcp/parser.go

@@ -153,10 +153,10 @@ func ParsingMiddleware(next http.Handler) http.Handler {
 // the caller is responsible for terminating the request (e.g. writing an
 // error response) instead of proceeding with a stale or absent parse.
 //
-// This only refreshes consumers that read the parse from the request context or
-// from a ParsedRequestHolder. Middleware that inspects the raw body from OUTSIDE
-// ParsingMiddleware — the tool-call filter and the rate limiter — has already
-// decided against the pre-rewrite body and is not corrected by republishing.
+// This refreshes downstream context consumers and outer wrappers that inspect
+// ParsedRequestHolder after the inner chain returns. It cannot change decisions
+// already made before the rewrite: tool-call filtering and rate limiting still
+// evaluate the request as received.
 //
 // The caller must also refresh r.ContentLength when it replaces r.Body, or the
 // reverse proxy will reject the forwarded request.

pkg/ratelimit/limiter.go

@@ -142,13 +142,17 @@ type bucketSpec struct {
 	refillPeriod time.Duration
 }
 
-// limitCheck keeps a bucket paired with its metric dimensions.
+// limitCheck keeps a bucket paired with its observability dimensions.
 type limitCheck struct {
 	bucket        *bucket.TokenBucket
 	scope         string
 	operationType string
 }
 
+func (c limitCheck) rejectionIdentifier() string {
+	return c.scope + "_" + c.operationType
+}
+
 // limiter is the concrete implementation of Limiter.
 type limiter struct {
 	client       redis.Cmdable
@@ -221,6 +225,7 @@ func (l *limiter) Allow(ctx context.Context, toolName, userID string) (*Decision
 	}
 
 	if len(checks) == 0 {
+		recordRateLimitSpanOutcome(ctx, rateLimitDecisionAllowed, rateLimitRejectedByNone)
 		return &Decision{Allowed: true}, nil
 	}
 
@@ -238,13 +243,15 @@ func (l *limiter) Allow(ctx context.Context, toolName, userID string) (*Decision
 	}
 	if rejectedIdx >= 0 {
 		l.telemetry.recordRejected(ctx, checks[rejectedIdx])
+		recordRateLimitSpanOutcome(ctx, rateLimitDecisionRejected, checks[rejectedIdx].rejectionIdentifier())
 		return &Decision{
 			Allowed:    false,
 			RetryAfter: buckets[rejectedIdx].RetryAfter(),
 		}, nil
 	}
 
 	l.telemetry.recordAllowed(ctx, checks)
+	recordRateLimitSpanOutcome(ctx, rateLimitDecisionAllowed, rateLimitRejectedByNone)
 	return &Decision{Allowed: true}, nil
 }
 

pkg/ratelimit/middleware_test.go

@@ -10,18 +10,21 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/alicebob/miniredis/v2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	sdkmetric "go.opentelemetry.io/otel/sdk/metric"
 	"go.uber.org/mock/gomock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	v1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
 	"github.com/stacklok/toolhive/pkg/auth"
 	"github.com/stacklok/toolhive/pkg/mcp"
+	"github.com/stacklok/toolhive/pkg/telemetry"
 	transporttypes "github.com/stacklok/toolhive/pkg/transport/types"
 	transportmocks "github.com/stacklok/toolhive/pkg/transport/types/mocks"
 )
@@ -207,6 +210,56 @@ func TestRateLimitedBodyMarshalFallback(t *testing.T) {
 	assert.NotContains(t, string(got), `"id"`)
 }
 
+func TestRateLimitHandler_AnnotatesTelemetryRequestSpan(t *testing.T) {
+	t.Parallel()
+	client, _ := newTestClient(t)
+	limiter, err := newLimiter(
+		client,
+		"test-ns",
+		"test-server",
+		newSpanTestRateLimitConfig(t, rateLimitScopeShared, rateLimitOperationTool),
+		nil,
+	)
+	require.NoError(t, err)
+
+	decision, err := limiter.Allow(t.Context(), "search", "")
+	require.NoError(t, err)
+	require.True(t, decision.Allowed)
+
+	tracerProvider, recorder := newRateLimitTracerProvider(t)
+	meterProvider := sdkmetric.NewMeterProvider()
+	t.Cleanup(func() {
+		require.NoError(t, meterProvider.Shutdown(context.Background()))
+	})
+	telemetryMiddleware := telemetry.NewHTTPMiddleware(
+		telemetry.Config{},
+		tracerProvider,
+		meterProvider,
+		"test-server",
+		"streamable-http",
+	)
+
+	handler := mcp.ParsingMiddleware(telemetryMiddleware(rateLimitHandler(limiter)(http.HandlerFunc(
+		func(http.ResponseWriter, *http.Request) {
+			t.Fatal("next handler should not be called when rate limited")
+		},
+	))))
+	req := httptest.NewRequest(
+		http.MethodPost,
+		"/mcp",
+		strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"search"}}`),
+	)
+	req.Header.Set("Content-Type", "application/json")
+	w := httptest.NewRecorder()
+
+	handler.ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusTooManyRequests, w.Code)
+	spans := recorder.Ended()
+	require.Len(t, spans, 1)
+	requireRateLimitSpanAttributes(t, spans[0], "rejected", "shared_tool")
+}
+
 func TestRateLimitHandler_RedisErrorFailOpen(t *testing.T) {
 	t.Parallel()
 

pkg/ratelimit/observability.go

@@ -15,6 +15,7 @@ import (
 	"github.com/redis/go-redis/v9"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/metric"
+	"go.opentelemetry.io/otel/trace"
 
 	"github.com/stacklok/toolhive/pkg/telemetry"
 )
@@ -24,6 +25,7 @@ const (
 
 	rateLimitDecisionAllowed  = "allowed"
 	rateLimitDecisionRejected = "rejected"
+	rateLimitRejectedByNone   = "none"
 
 	rateLimitScopeShared  = "shared"
 	rateLimitScopePerUser = "per_user"
@@ -139,6 +141,14 @@ func (t *rateLimitTelemetry) recordCheckLatency(ctx context.Context, duration ti
 	))
 }
 
+func recordRateLimitSpanOutcome(ctx context.Context, decision, rejectedBy string) {
+	trace.SpanFromContext(ctx).SetAttributes(
+		attribute.String("rate_limit.decision", decision),
+		attribute.String("rate_limit.rejected_by", rejectedBy),
+		attribute.Bool("rate_limit.fail_open", false),
+	)
+}
+
 func classifyRedisError(err error) string {
 	if redis.IsAuthError(err) {
 		return redisErrorTypeAuth