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

orca: fix the delimiter of the native http parser #37724

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Dec 18, 2024

Commit Message: orca: fix the delimiter of the native http parser
Additional Description:

This should also be cherry-pick to 1.32 to fix this problem.

The previous ORCA parser will use : as the delimiter of key/value pair in the native HTTP report. This is wrong
based on the design document. The correct delimiter should be =. This change fixes the delimiter to =.
See #6614 and related documents for more details.

https://docs.google.com/document/d/1NSnK3346BkBo1JUU3I9I5NYYnaJZQPt8_Z_XCBCI3uA/edit?tab=t.0

Native HTTP header encoding.  Similar to the JSON dictionary format, we will define Endpoint-Load-Metrics as having 
key=value format and use standard HTTP header encoding to either send the metrics as a sequence of HTTP headers or 
as a single header with [comma separated](https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2) key-value 
pairs. E.g.  endpoint-load-metrics: TEXT cpu=0.3,mem=0.8,foo_bytes=123.

Risk Level: n/a.
Testing: n/a.
Docs Changes: n/a.
Release Notes: added.

Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@wbpcode
Copy link
Member Author

wbpcode commented Dec 18, 2024

cc @blake-snyder @efimki

@wbpcode
Copy link
Member Author

wbpcode commented Dec 18, 2024

This is a behavior change. But the orca feature should not be widely used.

I am thinking should we change it directly, or add a runtime flag, or to support both ":" and "=" as delimiters (although may not match the document). cc @alyssawilk

@wbpcode
Copy link
Member Author

wbpcode commented Dec 18, 2024

/retest

@adisuissa
Copy link
Contributor

This change will probably be needed to be updated in many docs.
cc @AndresGuedez to weigh in.

@wbpcode
Copy link
Member Author

wbpcode commented Dec 18, 2024

This change will probably be needed to be updated in many docs. cc @AndresGuedez to weigh in.

I forget to push the change log. Actually, this PR fixed the implementation based on the design document. In the design document, = should be used. But in previous implementation, this : was mis-used. This PR correct that mistake.

@AndresGuedez
Copy link
Contributor

Thanks for catching this, agreed the design calls for '=' as the delimiter. However, this PR is backwards incompatible for existing users, so supporting both ':' and '=' would be preferable IMO.

@wbpcode
Copy link
Member Author

wbpcode commented Dec 18, 2024

Thanks for catching this, agreed the design calls for '=' as the delimiter. However, this PR is backwards incompatible for existing users, so supporting both ':' and '=' would be preferable IMO.

Then we need to ensure both : and = will be treated as special character and won't be a part of metric name. Of course, I am ok to that.

Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@blake-snyder
Copy link
Contributor

Nice catch. Thanks, @wbpcode !

@efimki
Copy link
Contributor

efimki commented Dec 18, 2024

Looks good to me, thank you for fixing this in backwards compatible way!

@wbpcode
Copy link
Member Author

wbpcode commented Dec 19, 2024

/retest

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!
Small nit, but otherwise LGTM.

test/common/orca/orca_parser_test.cc Show resolved Hide resolved
@wbpcode wbpcode enabled auto-merge (squash) December 19, 2024 15:50
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!

test/common/orca/orca_parser_test.cc Show resolved Hide resolved
@wbpcode wbpcode merged commit 70b7478 into envoyproxy:main Dec 19, 2024
25 checks passed
@wbpcode wbpcode deleted the dev-fix-bug-of-orca-parser branch December 19, 2024 16:40
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.

5 participants