-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
formatter context: new method to provide extension data #38232
base: main
Are you sure you want to change the base?
formatter context: new method to provide extension data #38232
Conversation
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting this PR.
It makes it easier to discuss the intent and achieve an agreed design.
* formatters. This could be used for non-HTTP protocols to provide protocol specific data to | ||
* formatters. | ||
*/ | ||
class Extension { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation of discussion from this comment.
Will it be possible to add specific methods to the interface. Having an open-ended interface isn't good in this case IMHO, and will make both the implementers and users life somewhat challenging.
This
Extension
is more like theFilterState::Object
and it may have complete different implementations for different protocols. The custom substitution formatter commands may convert it to specific data with dynamic_cast.
I think this is what I'm missing. Why is there a need for an opaque data-structure?
Generally speaking one should prefer abstraction (interfaces) over opaque data-structures, as they define the contract of what is allowed/expected.
What I'm trying to get away from is having void* structures as we used to have in C, and improve a bit if possible.
You have more domain knowledge here, so apologies in advance if my suggestion is naive/non-possible.
It would be best if a strategy pattern can be used here, but it really depends on the details.
Can you point out some of the use-cases so we can try to see if there's a shared interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking one should prefer abstraction (interfaces) over opaque data-structures, as they define the contract of what is allowed/expected.
Yeah. I know an explict interface is better. But at some scenarios, we still require an any
. We have lots of similar cases in Envoy code base, for example, the FilterState::Ojbect
, the Singleton::Instance
.
We cannot assume the structure/behavior of the data that will be stored in FilterState
or Singleton::Manager
, so, we can only use a opaque interface for FilterState::Ojbect
and Singleton::Instance
.
The formatter context extension is the same case: we cannot assume the structure/behavior, so, opaque is used and setter/getter will determine the specific type/structure/behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point out some of the use-cases so we can try to see if there's a shared interface?
Now, I have two scenarios will use the formatter context extension.
First, it is used to store the request/response metadata in the generic proxy. Here is its definition:
struct FormatterContextExtension : public Formatter::Context::Extension {
FormatterContextExtension(const RequestHeaderFrame* request, const ResponseHeaderFrame* response)
: request_(request), response_(response) {}
FormatterContextExtension() = default;
const RequestHeaderFrame* request_{};
const ResponseHeaderFrame* response_{};
};
By this extension, the generic-proxy-specific formatter commands could convert the Formatter::Context::Extension
to the GenericProxy::FormatterContextExtension
and access the meadata of request/response of generic proxy.
Second, it is used to store the parsed JSON of HTTP request in a custom HTTP filter. Then, this filter will extend a custom formatter command %BODY(xxx)%
to access the request body content. Here is its definition:
struct FormatterContextExtension : public Formatter::Context::Extension {
nlohmann::json json_body_;
};
You can see, they have no any shared point. Only the caller could know the type and usage of the data.
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
/retest |
3 similar comments
/retest |
/retest |
/retest |
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:]