Skip to content
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

Added utility methods to parse ORCA response headers from backends. #35422

Merged
merged 10 commits into from
Aug 8, 2024

Conversation

blake-snyder
Copy link
Contributor

@blake-snyder blake-snyder commented Jul 24, 2024

Commit Message: Add utility methods to parse ORCA response headers from backends.
Additional Description: Add utility methods to parse ORCA response headers from backends. parseOrcaLoadReportHeaders will be called from the RouterFilter.
Open Request Cost Aggregation (ORCA) Design Proposal
Using ORCA load reports in Envoy
Risk Level: Low
Testing: See included unit tests.
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: JSON formatted header not supported on Envoy Mobile

CC @efimki @AndresGuedez

Signed-off-by: blake-snyder <blakesnyder@google.com>
Copy link

Hi @blake-snyder, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #35422 was opened by blake-snyder.

see: more, trace.

@blake-snyder
Copy link
Contributor Author

/assign @adisuissa

Copy link

blake-snyder is not allowed to assign users.

🐱

Caused by: a #35422 (comment) was created by @blake-snyder.

see: more, trace.

Switch string serialization util method to something compatible with all builds.

Signed-off-by: blake-snyder <blakesnyder@google.com>
@adisuissa adisuissa self-assigned this Jul 25, 2024
@blake-snyder
Copy link
Contributor Author

Failed checks appear to be related to cncf/xds#97

@yanavlasov
Copy link
Contributor

@blake-snyder some error handling code is not covered by tests and coverage requirement is not met. https://storage.googleapis.com/envoy-pr/4dacf1a/coverage/source/common/orca/orca_parser.cc.gcov.html

/wait-any

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks!
Overall LGTM.
Can you please increase coverage (adding more tests)?

source/common/orca/orca_parser.cc Outdated Show resolved Hide resolved
source/common/orca/orca_parser.cc Outdated Show resolved Hide resolved
source/common/orca/orca_parser.cc Outdated Show resolved Hide resolved
@adisuissa
Copy link
Contributor

@wbpcode will you have the bandwidth to be the senior-maintainer reviewing the ORCA & new LB policy PRs?

Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
@wbpcode wbpcode self-assigned this Jul 27, 2024
…eaders. Change static constexpr variables to absl::string_view.

Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
@blake-snyder
Copy link
Contributor Author

/retest

adisuissa
adisuissa previously approved these changes Aug 6, 2024
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM overall with only two nit comments. Thanks so much for your patience, work, and update.

source/common/orca/orca_parser.cc Outdated Show resolved Hide resolved
test/common/orca/orca_parser_test.cc Outdated Show resolved Hide resolved
Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
@adisuissa
Copy link
Contributor

/retest

@adisuissa adisuissa merged commit 603bc61 into envoyproxy:main Aug 8, 2024
51 checks passed
asingh-g pushed a commit to asingh-g/envoy that referenced this pull request Aug 20, 2024
…nds. (envoyproxy#35422)

Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: asingh-g <abhisinghx@google.com>
wbpcode pushed a commit that referenced this pull request Oct 5, 2024
Commit Message: Add support for multiple formats of ORCA headers.
Additional Description: Add support for multiple formats of ORCA
headers. ORCA parsing introduced in
#35422
[Original Design
Proposal](#6614)
[Using ORCA load reports in
Envoy](https://docs.google.com/document/d/1gb_2pcNnEzTgo1EJ6w1Ol7O-EH-O_Ysu5o215N9MTAg/edit#heading=h.bi4e79pb39fe)
Risk Level: Low
Testing: See included unit tests.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: JSON format unsupported on Mobile.

CC @efimki @adisuissa @wbpcode

---------

Signed-off-by: blake-snyder <blakesnyder@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants