New Annotation to combine different Annotations

[walec51]

Problems with current annotations:

• you cannot specify more then one rate limiter
• you cannot specify the order of decorators (for example specify if rate limiter should take precedent before retries to not count them or should it be after the retryier to count the retries.

Proposal for new annotations:

@Retention(value = RetentionPolicy.RUNTIME)
@Target(value = {ElementType.METHOD})
@Documented
public @interface ResilienceDecorators {

  public Resilience[] value();
}

@Retention(value = RetentionPolicy.RUNTIME)
@Target(value = {ElementType.METHOD})
@Repeatable(ResilienceDecorators.class)
@Documented
public @interface Resilience {

  Retry[] retry() default {};

  RateLimiter[] rateLimiter() default {};

  @interface Retry {

    public static final String DEFAULT_CONFIG = "default";

    public String name();

    public String baseConfig() default DEFAULT_CONFIG;
  }

  @interface RateLimiter {

    static String FIXED_WEIGHT = "__FIXED_WEIGHT__";

    String name();

    int weight() default 1;

    String weightCalculator() default FIXED_WEIGHT;
  }
}

Usage examples on a JAX-RS interface for a REST Proxy Client framework like retrofit or si.mazi.rescu:

@Path("")
@Produces(MediaType.APPLICATION_JSON)
public interface Binance {

  @GET
  @Path("api/v1/exchangeInfo")
  @Resilience(retry = @Retry(name = "exchangeInfo"))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER))
  BinanceExchangeInfo exchangeInfo() throws IOException;

  @GET
  @Path("api/v1/depth")
  @Resilience(retry = @Retry(name = "depth"))
  @Resilience(
      rateLimiter =
          @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER, weightCalculator = "depthWeight"))
  BinanceOrderbook depth(@QueryParam("symbol") String symbol, @QueryParam("limit") Integer limit)
      throws IOException, BinanceException;

  public static int depthWeight(String symbol, Integer limit) {
    if (limit <= 100) {
      return 1;
    } else if (limit <= 500) {
      return 5;
    } else if (limit <= 1000) {
      return 10;
    }
    return 50;
  }

  @GET
  @Path("api/v1/aggTrades")
  @Resilience(retry = @Retry(name = "aggTrades"))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER))
  List<BinanceAggTrades> aggTrades(
      @QueryParam("symbol") String symbol,
      @QueryParam("fromId") Long fromId,
      @QueryParam("startTime") Long startTime,
      @QueryParam("endTime") Long endTime,
      @QueryParam("limit") Integer limit)
      throws IOException, BinanceException;

  @GET
  @Path("api/v1/ticker/24hr")
  @Resilience(retry = @Retry(name = "ticker24h"))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER, weight = 5))
  List<BinanceTicker24h> ticker24h() throws IOException, BinanceException;

  @GET
  @Path("api/v1/ticker/24hr")
  @Resilience(retry = @Retry(name = "ticker24h"))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER))
  BinanceTicker24h ticker24h(@QueryParam("symbol") String symbol)
      throws IOException, BinanceException;

  @GET
  @Path("api/v1/ticker/allPrices")
  @Resilience(retry = @Retry(name = "tickerAllPrices"))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER, weight = 2))
  List<BinancePrice> tickerAllPrices() throws IOException, BinanceException;

  @POST
  @Path("api/v3/order")
  @Resilience(
      retry =
          @Retry(
              name = "newOrder",
              baseConfig = ResilienceRegistries.NON_IDEMPOTENTE_CALLS_RETRY_CONFIG_NAME))
  @Resilience(rateLimiter = @RateLimiter(name = ORDERS_PER_SECOND_RATE_LIMITER))
  @Resilience(rateLimiter = @RateLimiter(name = ORDERS_PER_DAY_RATE_LIMITER))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER))
  BinanceNewOrder newOrder(
      @FormParam("symbol") String symbol,
      @FormParam("side") OrderSide side,
      @FormParam("type") OrderType type,
      @FormParam("timeInForce") TimeInForce timeInForce,
      @FormParam("quantity") BigDecimal quantity,
      @FormParam("price") BigDecimal price,
      @FormParam("newClientOrderId") String newClientOrderId,
      @FormParam("stopPrice") BigDecimal stopPrice,
      @FormParam("icebergQty") BigDecimal icebergQty,
      @FormParam("recvWindow") Long recvWindow,
      @FormParam("timestamp") SynchronizedValueFactory<Long> timestamp,
      @HeaderParam(X_MBX_APIKEY) String apiKey,
      @QueryParam(SIGNATURE) ParamsDigest signature)
      throws IOException, BinanceException;
}

This also showcases the "weight" feature for RateLimiter I'm proposing at: #642

If you would like to see a working implementation of these annotations then take a look at: knowm/XChange#3231

[RobWin]

Hi,
that's true. If you use Spring Boot you can only configure globally the order of the decorators. You can't do it per class.
We always thought about an annotation like:

@Resilience4j(decorators = [
    @Retry(name = "tickerAllPrices"), 
    @CircuitBreaker(name = "tickerAllPrices"),
    @RateLimiter(name = "tickerAllPrices")    
])

But then we thought, if a user needs the flexibility he could just implemented it without annotations:

Supplier<String> decoratedSupplier = Decorators.ofSupplier(supplier)
  .withRetry(Retry.ofDefaults("name"))
  .withCircuitBreaker(CircuitBreaker.ofDefaults("name"))
  .withRateLimiter(RateLimiter.ofDefaults("name"));

String result = Try.ofSupplier(decoratedSupplier)
  .recover(throwable -> "Hello from Recovery").get();

Would you like to create a PR for a new annotation and a Spring Boot aspect?

[walec51]

ok, give me some time to prepare it

[RobWin]

After your nice Bulkhead PR. Would you like to implement this issue as well?

[walec51]

Yes, I'm starting to work on this.

[walec51]

I understand every one is ok with this change?

If any one has concerns then please make them known now. I would like to avoid getting feedback about conceptual issues after spending hours of implementing it.

I'll implement in a backwards compatible manner, one will still be able to use those annotations without @Resilience annotation. I will however state in the JavaDoc that this way is becoming deprecated.

One question as for naming: Should we have a @Resilience annotation or name it @Decorator which would suggest some analogy to the Decorators class?

[RobWin]

Yes, we are ok with this change. But the old usage style should not be deprecated. The user just has two options. But the new option allows better control of the order of the decorators.

[walec51]

I think that having two options in this case will create more confusion then benefits. Using my approach adds very little complexity. By keeping both ways you will have to explain that in one way of doing annotations their order is respected and you can have like two rate limiter but in the other aproach it is not and you cannot. You will also need to document both approaches.

The final decision is up to you but I just wanted to make my opinion known.

[RobWin]

Ok, the feature to allow multiple RateLimiters on one method is new to me. I didn't notice it.

You want to limit the rate per second to protect against overload. That's clear to me.
But you also want to limit the rate per day so that Clients are allowed to send a certain number of orders per day? You don't need that per Client, or?

[RobWin]

I think in order to configure the number of permits (weight) you don't need to create a new RateLimiter instance.
The annotation could look like this,

@RateLimiter(name = "perSecond", permits = 5) or

@RateLimiter(name = "perSecond", permitsCalculator = "depthWeight")

and on another method you could use

@RateLimiter(name = "perSecond") which defaults to 1 permit.

[walec51]

See this method from my initial example:

@Path("")
@Produces(MediaType.APPLICATION_JSON)
public interface Binance {

  @POST
  @Path("api/v3/order")
  @Resilience(
      retry =
          @Retry(
              name = "newOrder",
              baseConfig = ResilienceRegistries.NON_IDEMPOTENTE_CALLS_RETRY_CONFIG_NAME))
  @Resilience(rateLimiter = @RateLimiter(name = ORDERS_PER_SECOND_RATE_LIMITER))
  @Resilience(rateLimiter = @RateLimiter(name = ORDERS_PER_DAY_RATE_LIMITER))
  @Resilience(rateLimiter = @RateLimiter(name = REQUEST_WEIGHT_RATE_LIMITER))
  BinanceNewOrder newOrder(
      @FormParam("symbol") String symbol,
      @FormParam("side") OrderSide side,
      @FormParam("type") OrderType type,
      @FormParam("timeInForce") TimeInForce timeInForce,
      @FormParam("quantity") BigDecimal quantity,
      @FormParam("price") BigDecimal price,
      @FormParam("newClientOrderId") String newClientOrderId,
      @FormParam("stopPrice") BigDecimal stopPrice,
      @FormParam("icebergQty") BigDecimal icebergQty,
      @FormParam("recvWindow") Long recvWindow,
      @FormParam("timestamp") SynchronizedValueFactory<Long> timestamp,
      @HeaderParam(X_MBX_APIKEY) String apiKey,
      @QueryParam(SIGNATURE) ParamsDigest signature)
      throws IOException, BinanceException;
}

This is a real world use case from the Binance cryptocurrency exchanges API. When placing orders it imposes the following limits:

• you cannot place more then X orders per second
• you cannot place more then Y orders per day
• placing order is also subject to their generic rate limiter which applies to all endpoints, you can call Z total weight of calls to their API per minute

[walec51]

In a more broader view: some cryptocurrency exchanges I'm working with impose these type of limits on a per user basis and some on a per IP basis. In the Xchange library I solve this problem by having either static resilience4j registries per exchange or by having an independent registry for each authenticated instance of an exchange client.

[RobWin]

Just to be 100% clear. You are a Client of the Binance API and not the developer of the API Implementation, correct?
You want to rate limit at client-level in order to prevent your application from doing unnecessary calls to the Binance API.
I wonder if you really should do it. Let's assume they are changing their rate limits someday, because they system is more scalable. You would have to adapt your client configuration.

[RobWin]

The current RateLimiter was more designed to limit the rate on backends which are under your control and not elastic scalable and you want to protect them from overload.
Currently, if you scale your client to more instances, you also have to "manually" adapt the RateLimiter configuration. There is no "distributed" rate limiting metric storage. The RateLimiter is not able to adapt itself.

That's why we are implementing an AdaptiveBulkead which can adapt itself. See concept -> #201
Concept is: "Stop Rate Limiting! Capacity Management Done Right"

[walec51]

Yes, I'm the client. I'm working on this library: knowm/XChange#3231

On Binance I can download their limits from their API. On other exchanges I have to mantain my configuration on my part. What I cannot do is to exceed those limit to far because they will block my IP then. And in a multi threaded application even if I will halt all my threads when I get the first error indicating I'm over the limit I still can get a ban because there are many concurrent request in the middle of execution.

[RobWin]

Ok, I understand.

[RobWin]

A new traffic shaping component would be nice. A combination of a ThreadPoolBulkhead and RateLimiter. The Bulkhead has a queue and threadpool and when the rate limit is exceeded, the threadpool stops processing tasks from the queue for a certain time (until the next rate limit window starts).
Basically a Leaky bucket algorithm.

Our current RateLimiter is not an implementation of a leaky bucket algorithm which leaks out at a constant rate. It uses a fixed window algorithm which can have burst effects at time window boundaries. The burst effects at time window boundaries can also lead to a situation where your client is sending to many request to the Binance servers.

[walec51]

I don't need constant rates. Burst effects are fine for me. All I want in my project it to respect the limits imposed by cryptocurrency exchanges. Every API of such that I know has a rate limit policy defined in the same way your RateLimiter works - its a perfect fit.

Applications in a clustered environment that want to use Xchange will have a problem if the limits are not IP bound but user account bound but that is something that the users of our library will have to handle in their applications code. Many exchanges will block their API keys if they see calls from multiple IPs to the same account at once. Making a client session stick with one server / IP is probably the best solution in this use case. Organizing work loads in such a manner is out side the scope of both resilience4j and Xchange.

[RobWin]

@walec51 Are you still working on this topic?

[walec51]

Yes, it took a lot of work to prepare an initial PR with basic support for the annotations I proposed. Please take a look if the direction this is heading is good.

Unfortunately I had to break compatibility for *AspectExt interfaces because they handled a ProceedingJoinPoint directly which was a show stopper for combining multiple decorators.

I've also refactored the code so that common code to handle

@Retry(name = BACKEND, fallbackMethod = "recovery")

and

@Decorator(retry = @Retry(name = BACKEND, fallbackMethod = "recovery"))

is encapsulated in a RetryAspectHelper component. Same thing goes for rate limiter, bulkhead, etc.

Despite doing that I still think that we should pick one way of doing annotations instead of supporting the old way and my new approach. I literary don't want to be the person that will have to explain in our documentation why this respects the annotations ordering:

@Decorator(retryLimiter = @RateLimiter(name = "serviceA"))
@Decorator(retry = @Retry(name = "connectionTimeout"))

and this does NOT:

@RateLimiter(name = "serviceA")
@Retry(name = "connectionTimeout")

I think that not deprecating the old way of doing annotations will give us more confusion then convenience in the long run.

[walec51]

@RobWin could you have a look at my PR, it was a lot of work and I would like to know if there is anything controversial there

[timmolter]

It would be great to have this functionality and have the PR merged!

[badgerwithagun]

+1, LTAGTM. I've been following this for a while.

Multiple rate limiters (and placing fully responsibility on the client to implement them) are a fact of life, and being able to implement these as aspects would really simplify development in cases like the ones @walec51 mentions.

[sureshkmit]

Any update on this issue? This would be a great value addition. Thanks.

[walec51]

my pull request #744

died on a review comment written by @RobWin which would break my working implementation

@RobWin did not respond to my explanation why his proposal was impossible to implement without breaking the order in which decorators would be applied, he just closed the pull request without responding

[RobWin]

Hi @walec51,
please don't take it personal that I didn't respond.
Life was stressful and tough the last years. The Open Source project is not part of my work duty. My daily work takes most of my time, then family and children and then I had to care of my herniated disc in the cervical spine, which effected me over two years until I had to have a surgery. And of course Covid on top of all.
The release of Resilience4j 2.0 took all of my time and there were a lot of other PRs which needed my attention. I'm really sorry if you feel that I wasted your time. That was not my intention.

I really want to look at this PR/feature again and see if we can support multiple decorators/annotation per method. I still like this feature very much.
But I really fear that we can't merge a feature which deprecates the old style to use annotations or effects other framework implementations. We still need to support Micronaut,