Rate limiting observability (metrics and tracing) PR B

[Sanskarzz]

Summary

• Annotate the existing request span with normal rate-limit outcomes so an
  individual request can be correlated with the limiter decision.
• Derive bounded rate_limit.rejected_by values from the limiter's private
  scope and operation dimensions without changing public rate-limit types.
• Start MCPServer telemetry after MCP parsing and before rate limiting so the
  limiter can annotate an active request span.
• Finalize MCP span names, attributes, and metric labels after downstream
  processing, using the parsed request republished by mutating webhooks. This
  attributes allowed requests to the tool and arguments that actually execute.
• Keep rate limiting before webhook calls, preserving cheap rejection of excess
  traffic and the existing vMCP core decorator composition.

Part of #4553

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (focused affected-package tests and full race run; details below)

• E2E tests (task test-e2e)

• Linting (task lint-fix)

• Manual testing (describe below)

• Focused tests passed for pkg/telemetry, pkg/runner, pkg/mcp,
  pkg/webhook/mutating, and pkg/vmcp/server.

• task lint-fix passed with zero issues.

• git diff --check passed.

• task test ran the repository race suite and reported one unrelated local
  environment failure in pkg/client/TestDetectProjectRoot: the local /tmp
  contains a synthetic .git directory. The exact test passed when rerun with
  TMPDIR=/var/tmp.

Changes

File	Change
pkg/ratelimit/limiter.go	Derives the rejecting bucket identifier and records normal span outcomes.
pkg/ratelimit/observability.go	Annotates the ambient request span with bounded rate-limit attributes.
pkg/ratelimit/observability_test.go	Verifies normal outcomes, all rejection identifiers, and the Redis-error boundary.
pkg/ratelimit/middleware_test.go	Proves an MCPServer rejection is recorded on the telemetry request span.
pkg/runner/middleware.go	Orders parser, telemetry, rate limiting, and webhooks while preserving early rejection.
pkg/runner/middleware_test.go	Pins parser, telemetry, rate-limit, mutating, and validating middleware order.
pkg/telemetry/middleware.go	Starts spans early and finalizes MCP identity from the shared post-mutation parse.
pkg/telemetry/integration_test.go	Verifies republished tool identity in span names, attributes, and metrics with telemetry- and audit-owned holders.
pkg/mcp/parser.go	Clarifies which downstream and outer consumers are corrected by republishing.
pkg/webhook/mutating/middleware.go	Documents publishing the final parse to outer audit and telemetry wrappers.
pkg/vmcp/ratelimit/decorator_test.go	Proves the vMCP decorator preserves and annotates the ambient span.
pkg/vmcp/server/server.go	Corrects comments describing HTTP telemetry and core rate-limit composition.
docs/observability.md	Documents normal-path rate-limit span attributes and bounded rejection values.
docs/middleware.md	Documents the final order and the remaining pre-mutation control limitation.

Does this introduce a user-facing change?

Yes. Traces for Redis-backed rate-limited tool calls now expose whether the
request was allowed or rejected and which bounded bucket type rejected it.
For allowed requests rewritten by a mutating webhook, completed telemetry uses
the MCP method, tool, and arguments that proceed to execution.

Rate-limit enforcement, responses, Redis keys, retry timing, and fail-open
behavior are unchanged. Rate limiting intentionally continues to evaluate the
request as received because it runs before outbound webhook calls.

Implementation plan

Approved implementation plan

1. Annotate the ambient request span at the concrete limiter's existing normal
   decision branches; do not create a dedicated rate-limit span.
2. Derive rate_limit.rejected_by from private scope and operation dimensions
   without expanding Limiter, Decision, or RateLimitedError.
3. Order MCPServer middleware as parser, telemetry, rate limit, mutating
   webhook, then validating webhook. This creates the span before limiting and
   rejects excess traffic before webhook round trips.
4. Reuse the shared ParsedRequestHolder and finalize MCP span names,
   attributes, and metrics after the inner chain returns, so republished
   mutations are reflected in completed telemetry.
5. Keep vMCP's existing HTTP telemetry and core rate-limit decorator wiring.
6. Verify bounded limiter outcomes, middleware ordering, and post-mutation
   telemetry with OpenTelemetry SDK recorders, then update the observability
   and middleware documentation.

Special notes for reviewers

Redis failures intentionally receive no normal-outcome attributes in this PR:
the limiter cannot know whether its caller applies fail-open behavior. A
follow-up PR will add the fail-open counter and set
rate_limit.fail_open=true at the shared enforcement adapter.

This PR incorporates the interaction identified after #6136. Telemetry remains
outside rate limiting and webhooks, but it reads the final parsed request from
the shared holder after downstream processing. Therefore, completed spans and
MCP metrics identify the request the backend executes, while rate limiting
continues to debit the request-as-received bucket and avoids webhook calls for
rejected traffic.

[github-actions]

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

---

This review will be automatically dismissed once you add the justification section.

[codecov]

Codecov Report

❌ Patch coverage is 90.24390% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.71%. Comparing base (ee293aa) to head (8dd3ac8).

Files with missing lines	Patch %	Lines
pkg/runner/middleware.go	77.77%	1 Missing and 1 partial ⚠️
pkg/telemetry/middleware.go	90.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5800      +/-   ##
==========================================
+ Coverage   72.69%   72.71%   +0.01%     
==========================================
  Files         742      742              
  Lines       77239    77265      +26     
==========================================
+ Hits        56152    56181      +29     
+ Misses      17126    17124       -2     
+ Partials     3961     3960       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jerm-dro]

Most of this diff is generated swagger docs, not your rate-limiting code. Likely cause: swag isn't pinned, so different runs can produce different output with no real API change.

Can you rerun task docs on your PR branch and push the result? If the diff is still large after that, rebase on #5802 (pins the swag version) and try again.

[Sanskarzz]

Most of this diff is generated swagger docs, not your rate-limiting code. Likely cause: swag isn't pinned, so different runs can produce different output with no real API change.

Can you rerun task docs on your PR branch and push the result? If the diff is still large after that, rebase on #5802 (pins the swag version) and try again.

Thanks, Jeremy. I reran task docs and reproduced the large Swagger-only diff locally. I also tested the pinned swag version from #5802, but my local run still rewrote generated schema names without any API change.
I removed the generated docs/server changes and the temporary pin commit to keep this PR scoped. The PR now contains only the intended rate-limit observability changes, and Docs / Verify Swagger Documentation passes in CI on e618f4b. It looks like the original diff was local generator/environment churn rather than a required API documentation update.

[aponcedeleonch]

the change looks good but it needs a rebase 😄

[jhrozek]

Heads-up on an interaction with #6136 (fixes #6133), since we'd otherwise discover it after one of us merges.

The overlap. #6136 makes the mutating webhook republish the parsed MCP request, because ParsingMiddleware parses once and the webhook was replacing r.Body without refreshing the cached parse — so authz was evaluating Cedar policy against the tool name and arguments that arrived rather than the ones the backend ran. Today telemetry sits after the webhook layers, so republishing fixes telemetry's view too.

This PR moves telemetry to immediately after the parser. That puts telemetry outside the mutation, and republishing can't reach it there: a derived context only flows downstream, and pkg/telemetry/middleware.go reads the parse solely via GetParsedMCPRequest(ctx) (:174, :227, :300, :417, :687, :723) with no ParsedRequestHolder. recordMetrics runs after next returns but against the ctx built at :217, so it's pre-mutation as well.

Net effect once both land in this order: span names, mcp_resource_id, and toolhive_mcp_tool_calls{tool} would attribute a call to the tool the caller requested rather than the one that executed, whenever a webhook rewrites params/name or method. Those two are documented contracts (docs/observability.md:203, :235), which is why I don't think "document it as as-received" is a good out.

No textual conflict. #6136 doesn't touch pkg/runner/middleware.go, so git will merge both cleanly and nothing will flag this. That's the reason I'm commenting rather than waiting.

One option you may not have considered. The requirement here is a live span before the limiter runs, which you can get by moving the limiter later instead of telemetry earlier:

parser → mutating → validating → usagemetrics → telemetry → ratelimit → authz

The limiter's only stated constraint is the one in its own comment at pkg/runner/middleware.go:166-167 — "Positioned after MCP parser (needs tool name from context)". Nothing requires it ahead of the webhooks. I checked the things that could object: vMCP rate limiting is a separate core.VMCP decorator (pkg/vmcp/ratelimit/decorator.go:37), not in the HTTP chain, so it's unaffected; and no existing test pins telemetry/limiter/webhook relative order (AuditWrapsChain:1366, StripAuthOrdering:1406, AWSStsOrdering:870 don't). It would also close the rate-limit half of #6134, where the limiter debits the bucket for the requested tool while the backend runs the mutated one.

But I'm not recommending it outright, because it has a real cost: rate-limited requests would then pay the webhook round-trips before being rejected. For a quota control that's backwards — outbound HTTP per rejected request is amplification on exactly the traffic you're shedding. If webhooks aren't configured it's free, but that's not something to rely on.

So the alternative worth weighing is splitting span creation (early, so the limiter has something to annotate) from MCP attribute population (late, post-mutation) — span.SetName and attribute-setting can both happen after next in principle. More churn inside the telemetry middleware, no cost to rejected requests. Your call, and observability ownership matters more than my opinion here.

If this merges first, #6136 will reconcile — I'd update the ordering list and the limitations bullet in docs/middleware.md, the comment at the republish site, and the RepublishParsedMCPRequest doc comment, which currently names the rate limiter as an uncorrected consumer.

If #6136 merges first (it's small, green, and fixes a live mis-authorization, whereas this one has changes requested), this PR would need those same four spots updated, plus a decision on the telemetry-accuracy question above.

No action needed from you on #6136 itself — flagging so the second one through isn't a surprise.

[Sanskarzz]

@jerm-dro @jhrozek @aponcedeleonch

Apologies for the delayed followup. I was away for a while due to a family medical emergency. I am back now and will review the feedback in the comment and continue working on the PR soon.

[jerm-dro]

@Sanskarzz no need to apologize. I hope all is well, and there's no rush here. We are thankful for your contributions.

[Sanskarzz]

@jhrozek Thanks for flagging the interaction with #6136. I rebased onto the current main and followed the split approach you outlined.
Telemetry starts the request span after parsing and before rate limiting, allowing the limiter to annotate it without making webhook calls for rejected traffic. After the inner chain returns, telemetry uses the shared ParsedRequestHolder updated by RepublishParsedMCPRequest to finalize the span name, MCP attributes, and metric labels.
Allowed requests rewritten by a mutating webhook are therefore attributed to the tool and arguments that actually execute. Rate limiting intentionally continues to evaluate the request as received.
I added regression coverage for both telemetry-owned and audit-owned parsed-request holders. I also updated the middleware ordering documentation, known-limitations section, republish-site comment, RepublishParsedMCPRequest documentation, and the PR description to reflect the final design and verification.