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

formatter context: new method to provide extension data #38232

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

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Jan 28, 2025

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:]

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 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 {
Copy link
Contributor

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 the FilterState::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?

Copy link
Member Author

@wbpcode wbpcode Jan 29, 2025

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.

Copy link
Member Author

@wbpcode wbpcode Jan 29, 2025

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]>
@wbpcode
Copy link
Member Author

wbpcode commented Jan 29, 2025

/retest

3 similar comments
@wbpcode
Copy link
Member Author

wbpcode commented Jan 29, 2025

/retest

@wbpcode
Copy link
Member Author

wbpcode commented Jan 29, 2025

/retest

@wbpcode
Copy link
Member Author

wbpcode commented Jan 29, 2025

/retest

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