xds: enable orca to lrs propagation by default - #12836
Conversation
Sorry, something went wrong.
danielzhaotongliu
left a comment
There was a problem hiding this comment.
This generally LGTM.
Note that there are (very) minor behaviour changes here that should be no-op in this PR, but should be called out in the PR description for posterity.
Sorry, something went wrong.
| BackendMetricPropagation backendMetricPropagation = null; | ||
|
|
||
| if (isEnabledOrcaLrsPropagation) { | ||
| if (isEnabledOrcaLrsPropagation && !cluster.getLrsReportEndpointMetricsList().isEmpty()) { |
There was a problem hiding this comment.
For my education, is this code being added here as a minor optimization to only propagate the list is not empty?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes and when a CDS resource has no metric specifications configured, we want backendMetricPropagation in CdsUpdate to remain null (representing "not configured") rather than instantiating an empty configuration object.
This was also impacting the behavior of XdsDependencyManager. Existing unit tests in XdsDependencyManagerTest verify parsed CdsUpdate objects using Mockito mock verifications that rely on strict .equals() checks. If we instantiated a non-null empty BackendMetricPropagation object for empty lists, those existing tests would fail because their expected mocks assume null for unconfigured fields.
And yeah it avoids unnecessary object allocation when there is nothing to propagate.
Sorry, something went wrong.
When the GRPC_EXPERIMENTAL_XDS_ORCA_LRS_PROPAGATION flag defaults to true, the following minor behavioral changes in backend metrics reporting will take effect:
Named Metrics Filtering & Prefixing:
Before: All custom/named metrics were reported as-is under their original name keys (e.g., "named1").
Now: Custom/named metrics are filtered based on the CDS-configured metrics specification. If a metric matches the propagation filter, it is now reported with the "named_metrics." prefix (e.g., "named_metrics.named1"). Unmatched named metrics are excluded.
CdsUpdate Structure for Empty Metric Specs (Runtime No-op):
Change: When the CDS cluster resource contains an empty lrs_report_endpoint_metrics list, the parser in XdsClusterResource now leaves backendMetricPropagation as null in the parsed CdsUpdate object rather than instantiating an empty configuration object.
Why: This ensures backward compatibility with Mockito verification tests that assert exact equality on CdsUpdate objects and expect unconfigured fields to be null.