xds: LRS custom metrics changes (a85)#9005
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9005 +/- ##
==========================================
+ Coverage 83.04% 83.06% +0.01%
==========================================
Files 411 413 +2
Lines 32892 33135 +243
==========================================
+ Hits 27316 27524 +208
- Misses 4181 4201 +20
- Partials 1395 1410 +15
🚀 New features to boost your workflow:
|
9173e18 to
8822c92
Compare
8822c92 to
7c192be
Compare
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the changes for LRS custom metrics as described in gRPC proposal A85. The changes are comprehensive, touching upon configuration parsing, balancer logic, picker implementation, and the LRS client itself to support propagating new ORCA metric types. The addition of an environment variable to gate this experimental feature is appropriate. The new functionality is also well-covered by unit and end-to-end tests.
The implementation looks solid. I have one minor suggestion regarding spec compliance when parsing lrs_report_endpoint_metrics.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: eshitachandwani <59800922+eshitachandwani@users.noreply.github.com>
eshitachandwani
approved these changes
Mar 27, 2026
eshitachandwani
left a comment
Member
There was a problem hiding this comment.
Approved with minor comments.
easwars
reviewed
Mar 31, 2026
dd2c42a to
5b5b6da
Compare
easwars
reviewed
Mar 31, 2026
easwars
left a comment
Contributor
There was a problem hiding this comment.
LGTM, modulo some outstanding comments.
The most important one of them was moving the newly added test TestValidateCluster_LRSReportEndpointMetrics to the other unmarshal_cds_test.go which is in the same package as the code (and therefore not having to use an variable that is defined only to be overridden by a test in another package).
easwars
approved these changes
Apr 2, 2026
| // update with the one currently used by picker, and is protected by b.mu. It | ||
| // returns a boolean indicating if a new picker needs to be generated. | ||
| func (b *clusterImplBalancer) handleDropAndRequestCountLocked(clusterConfig xdsresource.ClusterConfig) bool { | ||
| // handleClusterConfigLocked compares internal state with the new cluster |
Contributor
There was a problem hiding this comment.
I just realized that the old docstring was outdated too, in the sense that it does more than just compare the old and the new. Its updates the internal state as well. Could you please update the docstring to reflect reality. Thanks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses A85. Implements changes for LRS custom metrics.
RELEASE NOTES: