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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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.

Signed-off-by: blake-snyder <blakesnyder@google.com>
Signed-off-by: blake-snyder <blakesnyder@google.com>
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.

2 participants