Skip to content

Rate limiting observability (metrics and tracing) PR B#5800

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

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

Conversation

@Sanskarzz

Copy link
Copy Markdown
Contributor

Summary

  • Rate-limit metrics alone cannot explain an individual request, so annotate
    the existing request span with the normal allowed or rejected outcome.
  • Keep rejection identity private to the shared limiter and derive bounded
    values from its existing scope and operation dimensions.
  • Move MCPServer telemetry before rate limiting so rejected requests have an
    active span, while preserving the existing vMCP core decorator composition.
  • Add SDK-backed limiter, MCPServer, vMCP, and middleware-ordering coverage and
    document the span attribute contract.

Part of issue #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)

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 all normal outcomes and the Redis-error boundary with an SDK span recorder.
pkg/ratelimit/middleware_test.go Proves MCPServer rejection attributes land on the telemetry request span.
pkg/runner/middleware.go Places telemetry after MCP parsing and before rate limiting.
pkg/runner/middleware_test.go Pins parser, telemetry, and rate-limit execution order.
pkg/vmcp/ratelimit/decorator_test.go Proves the vMCP decorator preserves the ambient span context.
pkg/vmcp/server/server.go Corrects comments describing core rate-limit composition.
docs/observability.md Documents normal-path span attributes and rejection values.

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.
Rate-limit enforcement, responses, Redis keys, retry timing, and fail-open
behavior are unchanged.

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, then rate limit so the
    request span is active at the limiter.
  4. Keep vMCP's existing HTTP telemetry and core rate-limit decorator wiring.
  5. Verify all bounded outcomes with SDK span recorders and update the tracing
    reference.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.29%. Comparing base (1c60f0f) to head (e618f4b).

Files with missing lines Patch % Lines
pkg/runner/middleware.go 77.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5800      +/-   ##
==========================================
- Coverage   71.32%   71.29%   -0.04%     
==========================================
  Files         693      693              
  Lines       70587    70598      +11     
==========================================
- Hits        50347    50332      -15     
- Misses      16605    16631      +26     
  Partials     3635     3635              

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

@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 15, 2026
@jerm-dro

Copy link
Copy Markdown
Contributor

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
Sanskarzz force-pushed the ratelimiting-olly-1 branch from 103b54a to 6d5de2e Compare July 18, 2026 18:36
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 18, 2026
@Sanskarzz
Sanskarzz force-pushed the ratelimiting-olly-1 branch from 6d5de2e to 9ff840f Compare July 18, 2026 19:23
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 18, 2026
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
@Sanskarzz
Sanskarzz force-pushed the ratelimiting-olly-1 branch from 9ff840f to e618f4b Compare July 19, 2026 16:44
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 19, 2026
@Sanskarzz

Copy link
Copy Markdown
Contributor Author

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 aponcedeleonch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change looks good but it needs a rebase 😄

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

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants