Skip to content

Rate limiting observability (metrics and tracing) PR C - #6282

Open
Sanskarzz wants to merge 1 commit into
stacklok:mainfrom
Sanskarzz:ratelimiting-olly-3
Open

Rate limiting observability (metrics and tracing) PR C#6282
Sanskarzz wants to merge 1 commit into
stacklok:mainfrom
Sanskarzz:ratelimiting-olly-3

Conversation

@Sanskarzz

Copy link
Copy Markdown
Contributor

Summary

Redis error metrics show that a rate-limit check failed, but they do not show
whether ToolHive applied its fail-open policy and allowed the operation to
continue. This leaves operators unable to distinguish an observed dependency
failure from an enforcement outcome for an individual request.

  • Add toolhive_rate_limit_fail_open, a counter with bounded namespace and
    server attributes, emitted once per failed shared enforcement check.
  • Record fail-open at the shared ratelimit.Allow adapter used by MCPServer and
    VirtualMCPServer, without expanding the public limiter interface or
    duplicating transport-specific instrumentation.
  • Annotate the existing request span with decision=allowed,
    rejected_by=none, and fail_open=true when enforcement proceeds after a
    Redis error.
  • Preserve Redis error metrics, decision-counter semantics, warning logs,
    retry behavior, and the existing mandatory fail-open policy.

Part of #4553

Type of change

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

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Additional verification:

  • git diff --check
  • Concrete closed-Redis coverage for the shared adapter, MCPServer middleware,
    and VirtualMCPServer decorator
  • Repeated-failure coverage proving one fail-open increment per adapter call
  • Normal allowed, rejected, no-applicable-bucket, no-op, and nil-limiter
    coverage proving no fail-open increment

Changes

File Change
pkg/ratelimit/limiter.go Observes fail-open once at the shared enforcement adapter while preserving the original error.
pkg/ratelimit/observability.go Adds the fail-open counter and fail-open request-span outcome.
pkg/ratelimit/observability_test.go Verifies raw Redis errors, exact counter values and labels, repeated failures, normal outcomes, and nil-meter span behavior.
pkg/ratelimit/middleware_test.go Proves MCPServer delegates after Redis failure and preserves its successful request span.
pkg/vmcp/ratelimit/decorator_test.go Proves VirtualMCPServer delegates after Redis failure without creating another span.
docs/observability.md Documents the new counter and fail-open span semantics.

Does this introduce a user-facing change?

Yes. OpenTelemetry and Prometheus users can now identify rate-limit checks that
failed open. Prometheus exports the counter as
toolhive_rate_limit_fail_open_total.

Request admission behavior and client responses are unchanged.

Implementation plan

Approved implementation plan
  1. Keep raw Redis error and latency telemetry in the concrete limiter, where
    the Redis result is known, without claiming that fail-open has occurred.
  2. Add a private observer capability implemented by the built-in limiter; do
    not add a method to the exported Limiter interface.
  3. Invoke that observer once when the shared ratelimit.Allow enforcement
    adapter receives an infrastructure error, then return the original error so
    MCPServer and VirtualMCPServer retain their existing logging and delegation.
  4. Increment toolhive_rate_limit_fail_open with only namespace and server
    attributes and annotate the ambient request span as allowed through
    fail-open.
  5. Do not increment toolhive_rate_limit_decisions for an errored Redis check,
    because Redis did not produce a bucket decision.
  6. Verify the raw-error boundary, exact-once adapter emission, normal outcomes,
    and both production enforcement paths with concrete closed-Redis tests.
  7. Update the observability reference without adding a disruptive Redis-outage
    Kubernetes E2E.

Special notes for reviewers

The package-level ratelimit.Allow helper records fail-open observability but
still returns the original limiter error. This deliberately preserves the
existing HTTP and VirtualMCPServer warning logs and their established behavior
of continuing the request after infrastructure failure.

The fail-open counter describes applied enforcement policy, while
toolhive_rate_limit_redis_errors describes the underlying dependency failure.
It intentionally has no tool, user, request, error, scope, or operation labels.

Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
@Sanskarzz
Sanskarzz force-pushed the ratelimiting-olly-3 branch from c64450a to b6dcf1c Compare August 11, 2026 13:07
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.84%. Comparing base (026422e) to head (b6dcf1c).

Files with missing lines Patch % Lines
pkg/ratelimit/observability.go 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6282      +/-   ##
==========================================
+ Coverage   72.81%   72.84%   +0.02%     
==========================================
  Files         743      743              
  Lines       77647    77666      +19     
==========================================
+ Hits        56538    56572      +34     
+ Misses      17141    17130      -11     
+ Partials     3968     3964       -4     

☔ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant