Skip to content

api: add fields for propagation of ORCA to LRS#35210

Merged
adisuissa merged 2 commits into
envoyproxy:mainfrom
markdroth:orca_lrs_propagation
Jul 17, 2024
Merged

api: add fields for propagation of ORCA to LRS#35210
adisuissa merged 2 commits into
envoyproxy:mainfrom
markdroth:orca_lrs_propagation

Conversation

@markdroth

@markdroth markdroth commented Jul 16, 2024

Copy link
Copy Markdown
Contributor

Commit Message: api: add fields for propagation of ORCA to LRS
Additional Description: Add fields in CDS to control which ORCA fields get propagated to LRS. Also add fields in LRS for top-level ORCA metrics, to avoid having to encode the field name in LRS for these common cases.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A

CC @efimki @AndresGuedez @ejona86 @dfawley @vishalpowar

@markdroth
api: add fields for propagation of ORCA to LRS
d491032 
Signed-off-by: Mark D. Roth <roth@google.com>
@repokitteh-read-only

repokitteh-read-only Bot commented Jul 16, 2024

Copy link
Copy Markdown

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

🐱

Caused by: #35210 was opened by markdroth.

see: more, trace.

@tonya11en

tonya11en commented Jul 16, 2024

Copy link
Copy Markdown
Member

format wants you to apply:

Changes made to following files:

  ./envoy/config/endpoint/v3/load_report.proto

Format check failed, try apply following patch to fix:
diff --git a/api/envoy/config/endpoint/v3/load_report.proto b/api/envoy/config/endpoint/v3/load_report.proto
index f91c9cb..5ad177a 100644
--- a/api/envoy/config/endpoint/v3/load_report.proto
+++ b/api/envoy/config/endpoint/v3/load_report.proto
@@ -78,8 +78,11 @@ message UpstreamLocalityStats {
   // Stats for multi-dimensional load balancing.
   // These typically come from endpoint metrics reported via ORCA.
   UnnamedEndpointLoadMetricStats cpu_utilization = 12;
+
   UnnamedEndpointLoadMetricStats mem_utilization = 13;
+
   UnnamedEndpointLoadMetricStats application_utilization = 14;
+
   repeated EndpointLoadMetricStats load_metric_stats = 5;
 
   // Endpoint granularity stats information for this locality. This information

@markdroth
fix formatting
d54bede 
Signed-off-by: Mark D. Roth <roth@google.com>
@markdroth

markdroth commented Jul 17, 2024

Copy link
Copy Markdown
Contributor Author

/retest

adisuissa
adisuissa approved these changes Jul 17, 2024
View reviewed changes

@adisuissa adisuissa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

/lgtm api

adisuissa
adisuissa approved these changes Jul 17, 2024
View reviewed changes

@adisuissa adisuissa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

/lgtm api

@adisuissa adisuissa merged commit 18269c5 into envoyproxy:main Jul 17, 2024
@markdroth markdroth deleted the orca_lrs_propagation branch July 18, 2024 15:52
dfawley
dfawley reviewed Jul 18, 2024
View reviewed changes
Comment thread api/envoy/config/cluster/v3/cluster.proto
// The metric names in LRS will follow the same semantics as this field. In other words, if this field
// contains ``named_metrics.foo``, then the LRS load report will include the data with that same string
// as the key.
repeated string lrs_report_endpoint_metric = 57;

@dfawley dfawley Jul 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this isn't a plural, but all the other repeated fields I'm seeing adjacent to this PR are plural. Not sure if this kind of consistency is important to Envoy maintainers, just calling it out.

@wbpcode wbpcode Jul 19, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @markdroth is this field used in gRPC or not? If it haven't be released in gRPC, I think we can re-evaluate/update it.

@markdroth markdroth Jul 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't started using it yet. I'm fine with changing the name to be plural.

@markdroth markdroth Jul 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sent #35284 to make this change.

@markdroth markdroth mentioned this pull request Jul 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wbpcode wbpcode wbpcode left review comments

@adisuissa adisuissa adisuissa approved these changes

@mattklein123 mattklein123 mattklein123 approved these changes

+1 more reviewer

@dfawley dfawley dfawley left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@markdroth @tonya11en @adisuissa @mattklein123 @wbpcode @dfawley
⚡