formatter: add query_params to log all params #42891
Conversation
|
Hi @frittentheke, 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. |
frittentheke
force-pushed
the
allqueryparams_40925
branch
9 times, most recently
from
b7433af to
b8e7b82
Compare
| } | ||
|
|
||
| const auto query_params = | ||
| Http::Utility::QueryParamsMulti::parseAndDecodeQueryString(request_headers->getPathValue()); |
There was a problem hiding this comment.
I don't think you need to parse query parameters here? If you look into this method it's just searching for the first ? and returns everything after it. That's cheap, but doesn't normalize it in any way.
There was a problem hiding this comment.
@kyessenov So you mean I should just find the ? and return everything until the end of the url / string then?
My idea was to reuse as much of the library functions as possible, to potentially allow decoding or normalizing of parameters later. Kinda like %PATH(X:Y):Z% does allow with X=WQ and X=NQ.
But just having the url params as string is already a great improvement to me and my use case.
I can always normalize or url_decode that string later, outside of Envoy and adding an X parameter with options on what and how to produce the query params can always happen later.
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
A couple of comments...
frittentheke
force-pushed
the
allqueryparams_40925
branch
6 times, most recently
from
7008408 to
51087aa
Compare
| return absl::nullopt; | ||
| } | ||
|
|
||
| bool decode_bool = false; |
There was a problem hiding this comment.
Can you move this logic into the constructor? It is better to parse the config at config time, than at runtime. E.g. save decode_bool_ as a bool field rather than recompute it from decoding_.
There was a problem hiding this comment.
I will align this with how the PathFormatter does things (create method). I hope less variants are a good thing.
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
Please address comments, but mostly LG.
Signed-off-by: Christian Rohmann <[email protected]>
frittentheke
force-pushed
the
allqueryparams_40925
branch
from
51087aa to
7e80ca1
Compare
Fixes: envoyproxy#40925 Signed-off-by: Christian Rohmann <[email protected]>
frittentheke
force-pushed
the
allqueryparams_40925
branch
from
7e80ca1 to
1742c18
Compare
|
CI is still failing |
2 participants
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]
Fixes #40925