I think it would be useful to also have a gauge stat which can capture the "scaling" state. Feel to leave as a TODO for now.

Should reuse existing base types for ranges, see https://github.com/envoyproxy/envoy/blob/master/api/envoy/type/v3/range.proto#L39

Can you enforce min/max value while also use the existing range types?

I think what we actually have here is closer to https://github.com/envoyproxy/envoy/blob/master/api/envoy/type/v3/percent.proto. Maybe add a range there?

This seems inconsistent with the existing ThresholdTrigger definition, which uses a double in the range [0, 1].

htuch: we need a low percentage and a high percentage at which behavior should transition to linear scaling. We need to define a new message type anyway.

This is the first time the reader encounters "scaling state". It should either be explain in detail or a link to the architecture docs added with :ref: added.

Defining saturation/inactive via equality doesn't seem like the most clear possible signal. How about adding a bool isSaturated() const accessor?

if (connection_manager_.overload_stop_accepting_requests_ref_.isSaturated()) {

That doesn't seem like it scales very well; we'd end up with isSaturated(), isInactive(), and isScaling() to cover the three possible ranges. The options I explored were

1. using a variant<inactive{}, scaling{double}, saturated{}>, but that makes call sites messier and wastes space with the tag
2. adding a state() method that returns an enum that is one of {Inactive, Scaling, Saturated}
3. adding a value() method that returns the scale value for the scaling case - but then what does it return for the other cases?

Here's another option: I could get rid of the == and != operators and force callers to rely on <, <=, >, and >=. It makes it clear that there's an ordering for the values and doesn't expose double equality comparison.

I can see us wanting methods that tell us if the resource is saturated or if it is operating in the scaled linear region.

isInactive() seems like an anti-pattern. You want !isSaturated() or scaleFactor() == 0 depending on what kind of resource you're operating on.

Are the comparison operators really needed?

Is there precedent for implementing all of these operators?

Are operators other than == and != used?

No, only == and != are used currently, but we'll want the rest of these for comparing state objects in the scaling regime.

Use of an enum type that we can cast to to int would provide more efficient lookup than string_view.

Do we want this to be available as an extension point? I could see implementers of custom filters and such wanting to register their own timers for scaling in response to load.

Does it matter what kind of timer is being created? The actual timeout value is an input to the enable method.

What if Timer had a method enableTimerRange(const std::chrono::milliseconds& min, const std::chrono::milliseconds& max) that scales timeout based on overload manager state?

Should we consider some changes to how timers are created and enabled?

The thing to realize here is the pattern of creating a timer and immediately enabling it. Should the factory create and enable the timer internally? Doing that would provide us with some freedom regarding representation of timers that have similar deadline deltas. The current Timer class that accepts a delta on enableTimer, we may want a class that takes the timeout delta as a constructor argument and provides methods to register/unregister for that timeout.

I'm not against changing the timer interface, but I think that should be addressed in another PR. That would also allow us to get rid of the redundancies of the enableTimer and enableHRTimer methods - if you construct a timer with a high-resolution value, it is a HR timer, otherwise a regular timer.

You can call enableTimer with different values on the same timer object.

The issue of standard vs HR timers is separate. I think that based on how timers are usually created we only need enableHRTimer. I have a pending change that may obsolete the need for enableHRTimer.

variable naming consistency: active_ is true when state is saturated(). Should the variable be called saturated_?

I was trying to touch this code as little as possible, hence the minimal change. I'd suggest that since the heap shrinker can't be partially active, it doesn't make sense to talk about saturation here, and to leave this as a boolean active_.

Can you enforce min/max value while also use the existing range types?

Comparing to OverloadActionState::inactive() seems like an anti-pattern.

Can you document the meaning of the first argument to this factory method function signature?

Done.