accesslog: Add buffering and flushing to gRPC access log #7755
Conversation
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
@mattklein123 Do you think it's reasonable to enable buffering by default? I'm thinking about 64KB as a reasonable default, but a bit worried about altering the behavior of existing setups (if any). |
|
I'm OOO for the rest of the week. I will take a look next week. |
|
|
||
| // Soft size limit in bytes for access log entries buffer. Logger will buffer requests until | ||
| // this limit it hit, or every time flush interval is elapsed, whichever comes first. Setting it | ||
| // to zero effectively disables the batching. Defaults to 0. |
There was a problem hiding this comment.
Yeah IMO we should turn this on by default. WDYT about a default of 16KiB and 1s flush interval? I'm thinking that might be the least surprising and then users can tune? I'm trying to find a middle ground of responsiveness and buffering...
| @@ -43,13 +43,32 @@ void GrpcAccessLoggerImpl::LocalStream::onRemoteClose(Grpc::Status::GrpcStatus, | |||
| } | |||
|
|
|||
| GrpcAccessLoggerImpl::GrpcAccessLoggerImpl(Grpc::RawAsyncClientPtr&& client, std::string log_name, | |||
| std::chrono::milliseconds buffer_flush_interval_msec, | |||
| size_t buffer_size_bytes, Event::Dispatcher& dispatcher, | |||
There was a problem hiding this comment.
nit: s/size_t/uint64_t for buffer size (prefer explicit integer sizes in Envoy). Same elsewhere.
docs/root/intro/version_history.rst
Outdated
| @@ -3,6 +3,7 @@ Version history | |||
|
|
|||
| 1.12.0 (pending) | |||
| ================ | |||
| * access log: added :ref:`buffering <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_size_bytes>` and :ref:`periodical flushing <envoy_api_field_config.accesslog.v2.CommonGrpcAccessLogConfig.buffer_flush_interval>` support to gRPC access logger. | |||
There was a problem hiding this comment.
If we decide to default this on, can you add the defaults to the release notes?
…-log-batch Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
| } else { | ||
| // Clear out the stream data due to stream creation failure. | ||
| stream_.reset(); | ||
| } | ||
|
|
||
| // Clear the message regardless of the success. | ||
| approximate_message_size_bytes_ = 0; |
There was a problem hiding this comment.
qq: could we potentially use the proto built-in cached size to avoid keeping track of it ourselves?
There was a problem hiding this comment.
I don't see how. ByteSize() (or ByteSizeLong()) method does not use cached sizes for children (presumably because CachedSize is not invalidated on message modification).
Which means that the only way to use built-in cache is to manually iterate over all log entries and sum result of GetCachedSize() calls, which would be less efficient and less obvious in terms of implementation.
There was a problem hiding this comment.
OK SGTM. Thanks for the explanation.
Description: Add new fields to control buffering in gRPC access log. This buffer is flushed every time when timer is elapsed, or when estimated message size grows beyond the given threshold, whichever comes first.
Risk Level: LOW
Testing: added unit tests
Docs Changes: inline in protobuf files
Release Notes: updated
Fixes #7674