Skip to content
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

RateLimiter.withCost combinator + Rate Limiter composition tests #2090

Merged
merged 3 commits into from
Feb 11, 2024

Conversation

hsubra89
Copy link
Contributor

@hsubra89 hsubra89 commented Feb 10, 2024

Type

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This patch updates the RateLimiter module to include another constructor which includes a sub-constructor which allows us to specify a cost while maintaining composability with other RateLimiter instances. This enables the use of this module to rate limit for the following cases :

  • Accessing resources with an "API Credit" limit over a time window.

  • With composition, accounting for multiple "credit" limits. ( eg: Github API )

  • Accounting for different "costs" per api per provider within a single credit limit.

    • Many providers have different API costs for different endpoints within their ecosystem.

Usage Example

import { Effect, RateLimiter } from "effect";
import { compose } from "effect/Function";

const program = Effect.scoped(
  Effect.gen(function* ($) {
    // Create a rate limiter that has an hourly limit of 1000 credits
    const rateLimiter = yield* $(RateLimiter.make(1000, "1 hours"));
    // Query API costs 1 credit per call ( 1 is the default cost )
    const queryAPIRL = compose(rateLimiter, RateLimiter.withCost(1));
    // Mutation API costs 5 credits per call
    const mutationAPIRL = compose(rateLimiter, RateLimiter.withCost(5));
    // ...
    // Use the pre-defined rate limiters
    yield* $(queryAPIRL(Effect.log("Sample Query")));
    yield* $(mutationAPIRL(Effect.log("Sample Mutation")));

    // Or set a cost on-the-fly
    const history = yield* $(
      rateLimiter(Effect.log("Another query with a different cost")).pipe(
        RateLimiter.withCost(3)
      )
    );
  })
);

Sorry, something went wrong.

Copy link

changeset-bot bot commented Feb 10, 2024

🦋 Changeset detected

Latest commit: 9a30aae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
effect Patch
@effect/cli Patch
@effect/experimental Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/platform Patch
@effect/printer-ansi Patch
@effect/printer Patch
@effect/rpc-http Patch
@effect/rpc Patch
@effect/schema Patch
@effect/typeclass Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hsubra89 hsubra89 changed the title RateLimiter : Sub-constructor to customize cost + Rate Limiter composition tests RateLimiter.withCost combinator + Rate Limiter composition tests Feb 10, 2024
This patch updates the `RateLimiter` to allow setting a custom cost per effect.

- Additional tests covering new logic.
- Tests to verify composition of rate limiters.
- Updated documentation.
@IMax153
Copy link
Member

IMax153 commented Feb 10, 2024

@hsubra89 - apologies for the back-and-forth with the RateLimiter. We discovered the updated implementation was utilizing the fixed-window algorithm for token replenishment instead of the token-bucket algorithm.

The fix is in #2097. Please update this MR once the bugfix is merged.

@hsubra89
Copy link
Contributor Author

@hsubra89 - apologies for the back-and-forth with the RateLimiter. We discovered the updated implementation was utilizing the fixed-window algorithm for token replenishment instead of the token-bucket algorithm.

The fix is in #2097. Please update this MR once the bugfix is merged.

No problem. I did notice that when the implementation was changed but didn't look into it too deeply.

@hsubra89
Copy link
Contributor Author

Depends on #2097

Copy link
Contributor

@tim-smart tim-smart left a comment

Choose a reason for hiding this comment

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

I think we should merge this first, as the other PR will need to add more tests to make sure cost is well supported.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@hsubra89 hsubra89 requested a review from tim-smart February 11, 2024 21:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@tim-smart tim-smart merged commit efd41d8 into Effect-TS:main Feb 11, 2024
12 checks passed
@github-actions github-actions bot mentioned this pull request Feb 11, 2024
@hsubra89 hsubra89 deleted the rate-limiter-cost branch February 11, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants