Skip to content

[access log] Refactor gRPC access logger to support log sinks with different proto messages#14175

Merged
yanavlasov merged 2 commits intoenvoyproxy:masterfrom
yanavlasov:access-log
Dec 2, 2020
Merged

[access log] Refactor gRPC access logger to support log sinks with different proto messages#14175
yanavlasov merged 2 commits intoenvoyproxy:masterfrom
yanavlasov:access-log

Conversation

@yanavlasov
Copy link
Contributor

@yanavlasov yanavlasov commented Nov 25, 2020

Commit Message:
Refactor gRPC access logger to support log sinks with different proto messages

Additional Description:
This is pre-requisite change for adding OTLP access logger. This change refactors functionality common to all gRPC loggers into a set of template classes parameterized with protos of log entries and gRPC request and response messages.

Refactored functionality:

  • base class for creating gRPC access log connections.
  • cache of gRPC access log connections

As the OTLP access logger is being added these classes may be modified and more of the shared code may be moved.

Risk Level: Low (refactor)
Testing: Unit and Integration Test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: None

Part of #13801

Signed-off-by: Yan Avlasov yavlasov@google.com

… messages

Signed-off-by: Yan Avlasov <yavlasov@google.com>
@yanavlasov yanavlasov requested a review from zuercher as a code owner November 25, 2020 01:42
* Interface for an access logger. The logger provides abstraction on top of gRPC stream, deals with
* reconnects and performs batching.
*/
template <typename HttpLogProto, typename TcpLogProto> class GrpcAccessLogger {
Copy link
Contributor

@itamarkam itamarkam Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean to hijack this review, I was just curious to see how you templated all of this -
Does it make sense to also split the TCP and HTTP logging?
There's nothing specific to HTTP/TCP in this interface and it would allow creating a logger that does only one of the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can not split it as such but we can provide partial specialization for the GrpcAccessLogger that has only one log method, instead of two. There is currently no use case for it now, but it can be added as part of adding OLTP logger if it only supports logging one type of log entry.

template <typename LogProto> class GrpcAccessLogger<LogProto, void> {
public:
  using SharedPtr = std::shared_ptr<GrpcAccessLogger>;

  virtual ~GrpcAccessLogger() = default;

  /**
   * Log access entry.
   * @param entry supplies the access log to send.
   */
  virtual void log(LogProto&& entry) PURE;
};

@zuercher zuercher self-assigned this Nov 25, 2020
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yanavlasov yanavlasov merged commit 20bd8f7 into envoyproxy:master Dec 2, 2020
@yanavlasov yanavlasov deleted the access-log branch February 1, 2021 19:26
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.

3 participants