Introduce fixed_value in the adaptive concurrency filter

[wtzhang23]

Commit Message: introduce fixed_value in acc
Additional Description: This allows users to configure the adaptive concurrency filter to skip learning the minimum RTT and use a predetermined value. This is useful for upstreams that

• Have predictable regressions under load
• Are not sensitive to variable load.
• Have a clear latency target

Risk Level: High
Testing: Unit tests
Docs Changes: N/A
Release Notes: In PR
Platform Specific Features: N/A

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38359 was opened by wtzhang23.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38359 was opened by wtzhang23.

see: more, trace.

[wtzhang23]

Some comments:

• I think for correctness, it may be useful to have this and the dynamic minrtt learner as a typed extension. That way, we don't have to worry about conditional critical sections, which I find risky. But for simplicity, I modified the existing controller.
• Another alternative is to instead of fixing minrtt, define min and max fields that act as clamps. This could be used by customers to signal they either don't care about tuning minrtt under a certain latency, or they don't care about how "truly" performant their service is and want a tight bound on latency. However, this still requires customers to tune the min_concurrency parameter and still requires "exploration phases" learning the minrtt.

[tonya11en]

@wtzhang23 thanks for submitting the PR! We can't remove the required fields without causing incompatibilities, unfortunately. Can you implement this as a new concurrency controller? You should be able to just implement the interface. I suspect it won't be much more difficult, since you've already written the tests for this and the bulk of the complexity in the existing controller is the min_rtt calculation.

Let me know if this proves to be difficult and I can give it some more attention. Thanks!

[wtzhang23]

Sure. @tonya11en would you like this as a typed extension in case more controllers pop up?

[tonya11en]

Sure. @tonya11en would you like this as a typed extension in case more controllers pop up?

I don't think you need to go through all that. You should be able to just make a new config for the controller and add it to:

envoy/api/envoy/extensions/filters/http/adaptive_concurrency/v3/adaptive_concurrency.proto

Lines 99 to 105 in feaa042

	oneof concurrency_controller_config {
	option (validate.required) = true;
	// Gradient concurrency control will be used.
	GradientControllerConfig gradient_controller_config = 1
	[(validate.rules).message = {required: true}];
	}

Then just instantiate the appropriate controller here, based on whichever controller gets configured:
https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/adaptive_concurrency/config.cc#L15

[wtzhang23]

Quickly sketched out the implementation. My C++ is a little rusty so feel free to critique it as harshly as you'd like :) (e.g. whether multiple inheritance is bad here and composition is preferred, or whether it's better to have the config class fields duplicated).

Another problem is this is using protobuf oneofs which from what I remember is advised against.

[wtzhang23]

@tonya11en I was thinking about sharing the same logic between the two for sampling the sample RTT, but:

1. Didn't see much value in it since the shared logic is small
2. Think that spawning a timer in another location (e.g. a shared base class or another class) would be confusing.

Let me know your thoughts.

[tonya11en]

I haven't taken another pass yet, but I'll let you decide on the cleanest approach. Let me know when you think it's ready and I'll review. Seems like there are some conflicts that need to be resolved at the moment.

/wait