lrs: add a load-reporter for the SotW GrpcMux#43688
lrs: add a load-reporter for the SotW GrpcMux#43688yanavlasov merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…_v2_part1 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…_v2_part1 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
| std::move(primary_client), std::move(failover_client), main_thread_dispatcher_, random_, | ||
| *stats_.rootScope(), dyn_resources.ads_config(), local_info_, | ||
| std::move(custom_config_validators), std::move(backoff_strategy), xds_config_tracker, {}); | ||
| std::function<std::unique_ptr<Upstream::LoadStatsReporter>()> lrs_factory = |
There was a problem hiding this comment.
It looks like you can move the creation of the function object to the very top, since it is the same for all if/else branches.
There was a problem hiding this comment.
Unfortunately this depends on the primary_client that is dependent on the type of factory that is created.
A bigger refactor may be possible, and I'll try to see if we can have it in another PR.
| * Returns a load-stats-reporter that was created for the gRPC-Mux. | ||
| * Returns nullptr if a load-stats-reporter wasn't created for the gRPC-Mux. | ||
| */ | ||
| virtual Upstream::LoadStatsReporter* loadStatsReporter() const PURE; |
There was a problem hiding this comment.
It is hard to tell from the PR if the two methods are needed. Are there cases where we need to get load stats reporter without creating it first?
There was a problem hiding this comment.
I agree it is challenging to see how this is used now, but to keep the change small I've opted to introduce these now.
The reason to make it is to ensure that the load-stats-reporter isn't created when not needed (create by accident). I can merge the two methods now if you think it is a better approach. I have no strong preference.
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks for the review!
| std::move(primary_client), std::move(failover_client), main_thread_dispatcher_, random_, | ||
| *stats_.rootScope(), dyn_resources.ads_config(), local_info_, | ||
| std::move(custom_config_validators), std::move(backoff_strategy), xds_config_tracker, {}); | ||
| std::function<std::unique_ptr<Upstream::LoadStatsReporter>()> lrs_factory = |
There was a problem hiding this comment.
Unfortunately this depends on the primary_client that is dependent on the type of factory that is created.
A bigger refactor may be possible, and I'll try to see if we can have it in another PR.
| * Returns a load-stats-reporter that was created for the gRPC-Mux. | ||
| * Returns nullptr if a load-stats-reporter wasn't created for the gRPC-Mux. | ||
| */ | ||
| virtual Upstream::LoadStatsReporter* loadStatsReporter() const PURE; |
There was a problem hiding this comment.
I agree it is challenging to see how this is used now, but to keep the change small I've opted to introduce these now.
The reason to make it is to ensure that the load-stats-reporter isn't created when not needed (create by accident). I can merge the two methods now if you think it is a better approach. I have no strong preference.
2 participants
Commit Message: lrs: add a load-reporter for the SotW GrpcMux
Additional Description:
This allows creating a per-GrpcMux load-stats-reporter. Note that the only implementation is for SotW non-Unified GrpcMux to make this PR small. The following PR will add similar logic for unified/non-unified SotW+Delta-xDS.
The feature is protected behind the
envoy.reloadable_features.enable_lrs_server_self_adsruntime flag that is false by default.Part of the work towards #43326.
Risk Level: low - behind a runtime flag, should not impact current Envoys.
Testing: Added unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A