Change the algorithm used for byte-based log rate limiting

[ctlong]

Change the algorithm used for byte-based log rate limiting

[reneighbor]

For Hypothetical scenario 1 where no log lines are allowed - I can imagine customers being confused that they're missing log lines entirely. If 1 log line is larger in bytes than the max bytes per second, can we truncate the line so they know the line exceeds the limit?

[winkingturtle-vmw]

We could make a change in CAPI to not allow log rate limits that are below the max log line size.

This makes sense to me. Having some kind of API level failures where the minimum has to be met.

Too many log rate limit exceeded error messages

I think this comment makes sense as the goal of this feature. I think that instead of emitting every second, if we can figure out when it starts and stops, it makes sense.

Update the generic error message to include more meaningful information.

Can you elaborate what type of meaningful information this could be?

[ctlong]

PSA: planning to discuss this proposal at the working group meeting on Wednesday, May 5th.

[Benjamintf1]

Just for the record: here's a rephrasing of the state we're in

As part of implementing app level rate limiting with quotas, some changes to the algorithm of log rate limiting were made.
One of those dramatically increased the amount of "log rate exceeded" messages a user could see.
What we're looking to do is possibly:

1. reduce the amount of log-rate-exceeded messages, reducing the load on the platform
2. increase the amount of contiguous blocks of logs the user sees, possibly making it easier to interpret the logs that are emitted
3. keep the amount of clarity around which logs are being elluded due to log rate limits.

There's some future thoughts as well on:

1. trying to make sure applications can utilize more of their log-rate-limit when we can allow it, but that's not implemented in the pr we made this time. An exmaple would be truncating oversized logs rather then triggering the timeout immediately without logging.

[ctlong]

cloudfoundry/executor#73 was merged, which implements (2).

[geofffranks]

cloudfoundry/executor#73 has now been released in v2.77.0

[ctlong]

Started a discussion in CF Slack.

[ctlong]

Added a new step to the proposal based off Jochen's suggestion in Slack: Update the generic error message to include more meaningful information.

[Benjamintf1]

We've I think been explicitly avoiding truncating logs to meet the rate limit. Maybe it's something we should have considered more, fair.

TY for bringing it up!!

[Benjamintf1]

I'm not sure I agree with a minimum. People should feel free to set their limit low, or entirely off if need be. I'm not sure we have a good argument as to why that shouldn't be allowed. Perhaps a "hey, you sure about that" on the cli? I don't feel like a "never".

Can you elaborate what type of meaningful information this could be?

An example we've talked about is, if we want to go to a timeout type limit, including that information("You are being timed out for 1 second") in the outage warning log.

[ctlong]

I think that's a good rephrase, thanks Ben! Having a clear idea of (what we the think are) the benefits of the proposal definitely helps with evaluating it.

[Benjamintf1]

I think that instead of emitting every second, if we can figure out when it starts and stops, it makes sense.

That's why we're talking about doing a timeout box. It both allows us to reduce the number of messages saying when the logs are being emitted, and allows us to demarcate clearer when logs are being dropped.

[ctlong]

The outcome of the proposal discussion at the working group was general agreement on the proposal (by the stakeholders present at the meeting).

[winkingturtle-vmw]

I am going to close this issue since all related PRs have been merged. Please re-open if that's not the case.