-
Notifications
You must be signed in to change notification settings - Fork 966
Introduce DefaultXdsLoadBalancerLifecycleObserver
#6490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ikhoon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍
| final ImmutableList.Builder<Meter> metersBuilder = ImmutableList.builder(); | ||
| metersBuilder.add(Gauge.builder(prefix.name("lb.state.subsets"), () -> numSubsets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Collection seems unnecessary.
| localClusterName = bootstrap.getClusterManager().getLocalClusterName(); | ||
| if (!Strings.isNullOrEmpty(localClusterName) && bootstrap.getNode().hasLocality()) { | ||
| final DefaultXdsLoadBalancerLifecycleObserver observer = | ||
| new DefaultXdsLoadBalancerLifecycleObserver(meterIdPrefix, meterRegistry, localClusterName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that there’s still no extension point for users to configure a custom XdsLoadBalancerLifecycleObserver.
minwoox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍
| private final MeterIdPrefix prefix; | ||
| private final List<Meter> meters; | ||
|
|
||
| private int numSubsets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be volatile or AtomicInteger?
| private int healthyLoad; | ||
| private int degradedLoad; | ||
| private boolean panicState; | ||
| private double zarLocalPercentage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| private int total; | ||
| private int healthy; | ||
| private int degraded; | ||
| private int localityWeight; | ||
| private double zarPercentage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Motivation:
When using xDS based clients, users will often want to know how the xDS resources translate to the load balancer which routes requests upstream.
This changeset attempts introduce a
DefaultXdsLoadBalancerLifecycleObserverwhich collects metrics of each load balancer for each cluster.This observer has the same lifecycle as a
ClusterResourceNode, and will deregister each metric upon close.Note that thanks to
XdsClusterManager, each cluster has a unique name.Modifications:
DefaultXdsLoadBalancerLifecycleObserveris introduced which records metrics for aXdsLoadBalancerMeterRegistry,MeterIdPrefixcan now be specified inXdsBootstrapBuilderMeterRegistry,MeterIdPrefixis also added toSubscriptionContextso that they can be accessed when dynamically querying xDS resourcesEventExecutoris used for the default event loop.Result:
DefaultXdsLoadBalancerLifecycleObservernow records metrics forXdsLoadBalancers by default