Skip to content

Add throttling counters #500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

BelfordZ
Copy link
Contributor

@BelfordZ BelfordZ commented May 22, 2025

User description

Summary

  • record max load type via monitorEvent when throttled
  • cover rate-limiting metrics in tests

Testing

  • npm test

PR Type

Bug fix, Tests


Description

  • Record loadType via monitorEvent when throttling occurs

  • Add unit tests for rate-limiting metrics and throttling behavior

  • Ensure fallback to nestedCountersInstance.countEvent if monitorEvent unavailable

  • Improve observability of throttling events in rate limiting


Changes walkthrough 📝

Relevant files
Bug fix
index.ts
Record throttling events using monitorEvent with loadType

src/rate-limiting/index.ts

  • Use Context.shardus.monitorEvent to record throttling by loadType
  • Fallback to nestedCountersInstance.countEvent if monitorEvent is
    unavailable
  • Improves accuracy and observability of throttling events
  • +5/-1     
    Tests
    index.test.ts
    Add unit tests for rate-limiting throttling metrics           

    test/unit/src/rate-limiting/index.test.ts

  • Add unit tests for RateLimiting.isOverloaded throttling logic
  • Test that monitorEvent is called with correct parameters when
    overloaded
  • Test that monitorEvent is not called when under limit
  • Mock dependencies for isolated testing
  • +56/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 94
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Fallback Logic

    The fallback to nestedCountersInstance.countEvent if monitorEvent is unavailable should be validated to ensure that metrics are still recorded correctly in all deployment scenarios.

    if (Context.shardus && typeof Context.shardus.monitorEvent === 'function') {
      Context.shardus.monitorEvent('loadRelated', 'txRejected:' + loadType, 1, '')
    } else {
      nestedCountersInstance.countEvent('loadRelated', 'txRejected:' + loadType)
    }
    Mock Cleanup

    Ensure that all mocks, especially those on global objects like Math.random, are properly restored after each test to avoid test pollution.

    jest.spyOn(Math, 'random').mockReturnValue(0)
    const result = rl.isOverloaded('tx1')

    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

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

    Successfully merging this pull request may close these issues.

    1 participant