accesslog: Restructure gRPC access log implementation#7730
accesslog: Restructure gRPC access log implementation#7730mattklein123 merged 6 commits intoenvoyproxy:masterfrom
Conversation
* Move messaage construction and stream management to per-thread GrpcAccessLogger object * Don't silently ignore config changes and/or loggers with different configs Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, this is great. Flushing out a bunch of comments and then will take another pass.
/wait
| } | ||
| const Grpc::AsyncClientFactoryPtr factory = | ||
| async_client_manager_.factoryForGrpcService(config.grpc_service(), scope_, false); | ||
| const auto logger = std::static_pointer_cast<GrpcAccessLogger>( |
There was a problem hiding this comment.
nit: you shouldn't need a pointer case here.
There was a problem hiding this comment.
Used explicit type instead with implicit conversion.
source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc
Outdated
Show resolved
Hide resolved
| virtual ~GrpcAccessLogStreamer() = default; | ||
| virtual ~GrpcAccessLogger() = default; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Please bring back interface doc comments. Also, what's your plan for eventually supporting TCP logs? Presumably just have a separate log message that takes a TCP log?
There was a problem hiding this comment.
The plan is to have a separate log method which accepts TCP log.
Most of the log function implementation will be moved to flush in next pull request, so http-specific logic is quite small.
source/extensions/access_loggers/http_grpc/grpc_access_log_impl.h
Outdated
Show resolved
Hide resolved
source/extensions/access_loggers/http_grpc/grpc_access_log_impl.h
Outdated
Show resolved
Hide resolved
source/extensions/access_loggers/http_grpc/grpc_access_log_impl.h
Outdated
Show resolved
Hide resolved
source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc
Outdated
Show resolved
Hide resolved
| }); | ||
| } | ||
|
|
||
| GrpcAccessLoggerSharedPtr GrpcAccessLoggerCacheImpl::getOrCreateLogger( |
There was a problem hiding this comment.
Technically, you will never delete loggers that are no longer used. In practice, I don't think this is a real issue, but it's something that I would document in a TODO somewhere.
…-log-refactor Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
@mattklein123 I believe comments are addressed, can you have another look, please? |
|
@euroelessar PTAL at clang-tidy CI |
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
Oh, fair, thanks, updated. |
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
/azp run envoy-macos |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description:
Risk Level: MEDIUM (logger rewrite)
Testing: updated unit tests, existing integration tests
Docs Changes: n/a
Release Notes: n/a
Updates #7674