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

Add support for multiple formats of ORCA headers. #35894

Merged
merged 4 commits into from
Oct 5, 2024

Conversation

blake-snyder
Copy link
Contributor

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
Using ORCA load reports in Envoy
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

@wbpcode wbpcode self-assigned this Aug 29, 2024
@wbpcode
Copy link
Member

wbpcode commented Aug 29, 2024

I will insist on my previous point: use as less as possible header to do this work. The endpoint-load-metrics-bin is supported to keep compatibility with gRPC.

But for new format, we can use same header to carry content with different format. And single prefix in the header value could be used to indicate it's format. like:

endpoint-load-metrics: TEXT xxxxxxxxx
endpoint-load-metrics: JSON xxxxxxxxx

/wait

@blake-snyder
Copy link
Contributor Author

Ran some benchmarking tests to investigate performance questions raised in original PR an got the following:

-----------------------------------------------------------------------------------------
Benchmark                                               Time             CPU   Iterations
-----------------------------------------------------------------------------------------
BM_ParseOrcaHeaders_NativeHTTP/1                     6.46 us         6.00 us            1
BM_ParseOrcaHeaders_NativeHTTP/8                     7.27 us         7.26 us            1
BM_ParseOrcaHeaders_NativeHTTP/64                    16.3 us         16.3 us            1
BM_ParseOrcaHeaders_NativeHTTP/98                    18.2 us         18.2 us            1
BM_ParseOrcaHeaders_Json/1                           4.73 us         4.71 us            1
BM_ParseOrcaHeaders_Json/8                           6.32 us         6.29 us            1
BM_ParseOrcaHeaders_Json/64                          14.8 us         14.8 us            1
BM_ParseOrcaHeaders_Json/98                          18.0 us         18.0 us            1
BM_ParseOrcaHeaders_Bin/1                            3.79 us         3.80 us            1
BM_ParseOrcaHeaders_Bin/8                            5.25 us         5.22 us            1
BM_ParseOrcaHeaders_Bin/64                           13.7 us         13.7 us            1
BM_ParseOrcaHeaders_Bin/98                           16.1 us         16.1 us            1
BM_ParseOrcaHeaders_Missing/1                        2.30 us         2.31 us            1
BM_ParseOrcaHeaders_Missing/8                        3.52 us         3.52 us            1
BM_ParseOrcaHeaders_Missing/64                       12.2 us         12.2 us            1
BM_ParseOrcaHeaders_Missing/98                       15.8 us         15.8 us            1
BM_HeaderMapGetOpMiss/1                              2.32 us         2.32 us            1
BM_HeaderMapGetOpMiss/8                              3.10 us         3.11 us            1
BM_HeaderMapGetOpMiss/64                             11.7 us         11.7 us            1
BM_HeaderMapGetOpMiss/98                             14.5 us         14.5 us            1

@wbpcode
Copy link
Member

wbpcode commented Sep 20, 2024

cc @blake-snyder Hi, per our offline discussion, I think the final conclusion is keep only one header and only to support on format first, but will add a short prefix flag ('TEXT ' or 'T ', a single space is used to separate the prefix and value) in the header value.

It's easy to add prefix flag in the future in the code, but it's not easy to keep backward compatibility in the future if no this flag now. This is the reason why we discuss this for a long time.

And CI is complaining, you may need to check it.

/wait

@wbpcode
Copy link
Member

wbpcode commented Sep 20, 2024

And I have to say sorry, this is not a big job, but finally it taken your very long time.

But I actually don't want to compromise on this point. :( Because I know if I compromised this time, then in the future, in case new format is necessary, the new header must be introduced.

@blake-snyder
Copy link
Contributor Author

/retest

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 4, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #35894 was synchronize by blake-snyder.

see: more, trace.

Signed-off-by: blake-snyder <[email protected]>
@wbpcode
Copy link
Member

wbpcode commented Oct 5, 2024

Thanks so much for this great contribution and all your patience.

@wbpcode wbpcode merged commit 33679e4 into envoyproxy:main Oct 5, 2024
21 checks passed
Stevenjin8 pushed a commit to Stevenjin8/envoy that referenced this pull request Oct 10, 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
envoyproxy#35422
[Original Design
Proposal](envoyproxy#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 <[email protected]>
Signed-off-by: Steven Jin Xuan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants