Add ExtProc Logging Bits for Immediate Response, Continue and Replace #41602
Conversation
…in FilterState Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
melginaldi
requested review from
mattklein123,
tyxia and
yanavlasov
as code owners
|
/assign @yanjunxiang-google |
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
|
/assign @tyxia |
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
LGTM in general. Thanks for contribution!
I will let @yanjunxiang-google review it as well
|
/wait |
Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
…resp-contreplace Signed-off-by: Melissa Ginaldi <[email protected]>
|
@yanjunxiang-google can you PTAL? |
| envoy::config::core::v3::TrafficDirection traffic_direction); | ||
| envoy::config::core::v3::TrafficDirection traffic_direction, | ||
| bool continue_and_replace = false); | ||
| void setImmediateResponse() { immediate_response_ = true; } |
There was a problem hiding this comment.
Maybe change this into setImmediateResponse(bool immediate_response) {immediate_response_ = immediate_response; } ?
There was a problem hiding this comment.
Personally, I like it it the way it is. Since the default value is false when the variable is created ext_proc.h L 151 (https://github.com/envoyproxy/envoy/pull/41602/files#diff-d95000cdea207e2cf306d30b1706750b57a8228c25f01b0f8b4df0f22bf033c7R151) I think it makes more sense that "set" implies you are changing the value to true.
However, this is not a very strong opinion and I am happy to make the change if you feel strongly it should take in a variable.
There was a problem hiding this comment.
Just thought do you ever need to call setImmediateResponse() to set it to false. Small thing, up to you
|
Thanks for working on this. Just a few nit comments. The data structure definition is following what we discussed offline. Overall LGTM. |
Signed-off-by: Melissa Ginaldi <[email protected]>
Signed-off-by: Melissa Ginaldi <[email protected]>
|
/wait |
|
Could you elaborate on the motivation and use case ?
Also, if it is for debugging, can we use Envoy trace/log |
|
/wait |
|
@melginaldi Actually, can we add an Envoy global immediate_response stats and continue_and_replace stats counters here: . It gives us good idea on these events as well. What's the benefit to put them into a per call stats? |
yanjunxiang-google
left a comment
yanjunxiang-google
left a comment
There was a problem hiding this comment.
please address the left over comments. Dismiss the previous approval.
5 participants
Commit Message: Add bits to the FilterState in ExtProcLoggingInfo for when an immediate response is sent and when continue_and_replace is used.
Additional Description: These bits will be useful for internal debugging. Continue and Replace is only relevant for request and response headers.
Risk Level: Low
Testing: Unit Integration tests
Docs Changes: N/A
Release Notes: Add new bits into ExtProcLoggingInfo for Immediate Response occurrence and ContinueAndReplace.
Platform Specific Features: N/A
/assign @yanjunxiang-google