what will happen if both are set?

This is oneof semantics. (although oneof is not recommend according to our style).

If both are set, the configuration will be rejected.

In the request level, it can be an integer or a format string. How come here it is only an integer?

The format string is used to extract a number based on the request and stream info. See the comments of format string comment

    // Substitution format string to extract the number of hits to add to the rate limit descriptor.
    // The same :ref:`format specifier <config_access_log_format>` as used for
    // :ref:`HTTP access logging <config_access_log>` applies here.
    //
    // .. note::
    //   The format string must contain only single valid substitution field that will be replaced
    //   with a non-negative number.
    //
    // For example, the ``%BYTES_RECEIVED%`` format string will be replaced with the number of bytes
    // received in the request.

In the previous implementation, the class LocalDescriptor has two usages:

1. as the key of rate limit rules that every descriptor is related to a token bucket.
2. as the output of the descriptor populating of local rate limt filter. It will be used to find a matched rate limit rule.

But now, to support per descriptor hits_addend, we need a new class to represent the output of the descriptor populating. Finally, we choose to enhance and re-use the Descriptor class. (The Descriptor class is used as the output of global rate limit filter. Re-using it could also simplify future development when we want to add similar support for global rate limit filter in the future.)

instead of appending to the descriptor, it should be used to populate the hits_addend field in the RLS request
https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/ratelimit/v3/rls.proto#service-ratelimit-v3-ratelimitrequest

Single RateLimit message is used to generate a single descriptor. The RateLimit.hits_addend field here is also a descriptor level configuration. So, the generated value of RateLimit.hits_addend field here should also be populated to the per-descriptor Descriptor.hits_addend rather than request level RateLimitRequest.hits_addend.

shouldnt this be one level up, under https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-ratelimit ? so it can set the hits_addend field of https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/ratelimit/v3/rls.proto#service-ratelimit-v3-ratelimitrequest ?
The ratelimit request holds a list of descriptors

The https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-ratelimit is what I said single RateLimit.

Every route/vhost will have a list of RateLimit configuration. Every Ratelimit will generate a single descriptor (a single descriptor is composited of multiple descriptor entries). All these descriptors finally composite the rate limit request.

But note, every descriptor is evaluated independently and will match to completely different rules. One of the most important target of this PR is to support per-descriptor hits addend, then we can limit different resource like qps or bandwidth in same request. You can check this PR's description for the PR's target. I have no plan to enhance the request level hits addend because it could be replaced by descriptor-level hits addend.

What is the behavior if this is invalid and/or is a format string that doesn't lead to an integer? Please clarify.

I didn't look at this carefully but did you audit everything to make sure there can be no underflow or overflow conditions?

I didn't look at this carefully but did you audit everything to make sure there can be no underflow or overflow conditions?

Good point. I am sure underflow or overflow won't happen because we only consum the token when the expected_tokens larges then to_consume. And they both are uint32_t.

But I think this notices me to add a more strict check to the value range of the to_consume.

Use a uint64_t directly would much simple. And the max to_consume is limited to 1000000000. If even the uint64_t is enough in this case, them just let it explode.

This can heavily log spam if there is a config mistake. This should be a warn every of some type. Optimally we would catch as much as possible at config load time which isn't happening.

This can heavily log spam if there is a config mistake. This should be a warn every of some type. Optimally we would catch as much as possible at config load time which isn't happening.

Currently we have no way to check the provider's expected return type. I can change the level to debug and add more clear descripription on the API.

I think this is going to be very confusing to debug and I would go back to WARN but use one of the power of 2 or timed variants for that, but up to you.

I think this is going to be very confusing to debug and I would go back to WARN but use one of the power of 2 or timed variants for that, but up to you.

Will change this in next PR. There is a series of works.