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.

Sign up for GitHub

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
Open

formatter context: new method to provide extension data #38232

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7071902
formatter context: new method to provide extension data
Jan 28, 2025
776618b
add simple test for new code
Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions envoy/formatter/http_formatter_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ using AccessLogType = envoy::data::accesslog::v3::AccessLogType;
*/
class HttpFormatterContext {
public:
/**
* Interface for a context extension which can be used to provide non-HTTP specific data to
* 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 •
edited
Loading

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 •
edited
Loading

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.

public:
virtual ~Extension() = default;
};

/**
* Constructor that uses the provided request/response headers, response trailers, local reply
* body, and access log type. Any of the parameters can be nullptr/empty.
Expand Down Expand Up @@ -135,14 +145,39 @@ class HttpFormatterContext {
*/
static constexpr absl::string_view category() { return "http"; }

/**
* Set the context extension.
* @param extension supplies the context extension.
*/
HttpFormatterContext& setExtension(const Extension& extension) {
extension_ = extension;
return *this;
}

/**
* @return OptRef<const ContextExtension> the context extension.
*/
OptRef<const Extension> extension() const { return extension_; }

/**
* @return OptRef<const ExtensionType> the context extension casted to the specified type.
*/
template <class Type> OptRef<const Type> typedExtension() const {
const Type* typed_extension = dynamic_cast<const Type*>(extension_.ptr());
return makeOptRefFromPtr(typed_extension);
}

private:
const Http::RequestHeaderMap* request_headers_{};
const Http::ResponseHeaderMap* response_headers_{};
const Http::ResponseTrailerMap* response_trailers_{};
absl::string_view local_reply_body_{};
AccessLogType log_type_{AccessLogType::NotSet};
const Tracing::Span* active_span_ = nullptr;
OptRef<const Extension> extension_;
};

using Context = HttpFormatterContext;

} // namespace Formatter
} // namespace Envoy