feat: expose gRPC access log buffering on the ALS and OpenTelemetry sinks - #9725
Open
xiongzubiao wants to merge 1 commit into
Open
feat: expose gRPC access log buffering on the ALS and OpenTelemetry sinks#9725xiongzubiao wants to merge 1 commit into
xiongzubiao wants to merge 1 commit into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
xiongzubiao
force-pushed
the
feat/accesslog-grpc-buffer
branch
from
August 11, 2026 19:04
c9abf61 to
16bb949
Compare
Author
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…inks Envoy Gateway built CommonGrpcAccessLogConfig with only LogName, GrpcService and TransportApiVersion set, so buffer_flush_interval and buffer_size_bytes always took Envoy's defaults of 1s and 16384 bytes, and nothing on ProxyAccessLogSetting could change them. Add a `buffer` field carrying `flushInterval` and `sizeBytes` to both gRPC sinks. It sits on the sink rather than on the ProxyAccessLogSetting entry because the setting is meaningless for File sinks, and because one settings entry can carry several sinks whose backends differ in availability. Both sinks feed the same CommonGrpcAccessLogConfig, so both get it. Unset fields are not written, so existing configurations produce byte identical xDS. Envoy carries buffer_size_bytes as a 32-bit unsigned integer. A CEL bound rejecting 4Gi and above at admission is not possible here: quantity() on an x-kubernetes-int-or-string field exceeds the CRD validation cost budget by 63x, and MaxLength does not help because the estimator treats the value as dynamic. The range is checked during translation instead, matching how BufferLimit and the other Quantity byte-size fields already behave. Zero and negative values are rejected there too, since the pattern constrains only the string form of this int-or-string field. Also reword the Gateway status message for access log translation failures from "Invalid access log backendRefs in the referenced EnvoyProxy" to "Invalid access log configuration in the referenced EnvoyProxy". processAccessLog has five error returns and only two concern backendRefs; the message already misreported invalid CEL match expressions, and would now also misreport buffer settings. processTracing and processMetrics each have a single backendRefs error return, so their wording is left alone. e2e coverage is added for both gRPC sinks, following the existing per-sink access log tests. Verified against a kind cluster: the running proxy's config_dump reports the configured values on both the otel_envoy_accesslog and ALS loggers. The task docs describe the two settings as the pair they are. Entries survive a sink outage only while they stay buffered, so flushInterval has to outlast the outage and sizeBytes has to hold the entries arriving during it. At the 1s default a flush is attempted every second, so raising sizeBytes alone does not widen that window. Measured with a fault proxy in front of the collector: with flushInterval 60s and a 20s outage, sizeBytes 4Mi delivered all 20 entries after recovery while 512 bytes delivered 5. Signed-off-by: Zubiao Xiong <xiongzubiao@gmail.com>
zirain
force-pushed
the
feat/accesslog-grpc-buffer
branch
from
August 28, 2026 13:17
16bb949 to
9a893d1
Compare
zirain
reviewed
Aug 28, 2026
| // Buffer defines how access log entries are buffered before being flushed to the | ||
| // access log service. | ||
| // +optional | ||
| Buffer *GRPCAccessLogBufferSettings `json:"buffer,omitempty"` |
Member
There was a problem hiding this comment.
could this be more generic to support more options in the future?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Envoy Gateway builds
CommonGrpcAccessLogConfigfor the OpenTelemetry and ALS access log sinks with onlyLogName,GrpcServiceandTransportApiVersionset, sobuffer_flush_intervalandbuffer_size_bytesalways take Envoy's defaults of 1s and 16384 bytes and nothing onProxyAccessLogSettingcan change them. This adds abufferfield carryingflushIntervalandsizeBytesto both gRPC sinks.Placement. The issue asked whether this belongs per-sink or per-
settingsentry and offered both. This PR puts it per-sink: the setting is meaningless forFilesinks, and onesettingsentry can carry several sinks whose backends differ in availability, so a single knob could not express that.GRPCAccessLogBufferSettingsis declared once and referenced from both sink types.ALS. Also left open in the issue — yes, it applies. Both sinks feed the same
CommonGrpcAccessLogConfig, andapplyAccessLogBufferruns before theswitch als.Type, so ALS HTTP and TCP are both covered.Backward compatibility. Unset fields are never written, so existing configurations produce byte-identical xDS. Verified by regenerating every golden against a stashed tree.
Range checking. Envoy carries
buffer_size_bytesas a 32-bit unsigned integer. A CEL bound rejecting 4Gi and above at admission is not possible here:quantity()on anx-kubernetes-int-or-stringfield exceeds the CRD validation cost budget by 63x, andMaxLengthdoes not help because the estimator treats the value as dynamic. The range is checked during translation instead, matching howBufferLimitand the other Quantity byte-size fields already behave. Zero and negative values are rejected there too, since the pattern constrains only the string form of an int-or-string field.Status message change, please review.
processAccessLoghas five error returns and only two concern backendRefs, so "Invalid access log backendRefs in the referenced EnvoyProxy" already misreported invalid CEL match expressions and would now also misreport buffer settings. It is reworded to "Invalid access log configuration in the referenced EnvoyProxy" and carries its own release note.processTracingandprocessMetricseach have a single backendRefs error return, so their wording is unchanged. This is the one change outside the strict scope of the issue.A practical caveat the issue does not state. Entries survive a sink outage only while they stay buffered, and a flush that fires while the sink is unreachable loses the batch it was carrying. Measured on Envoy 1.40.0-dev with a fault proxy in front of the collector: with
flushInterval: 60sacross a 20s outage,sizeBytes: 4Miretained 20 of 20 entries while512retained 5 of 20. At the defaultflushInterval: 1sa flush is attempted every second, so raisingsizeBytesalone does not widen that window. The task documentation describes the two settings as the pair they are.Which issue(s) this PR fixes:
Fixes #9723
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.API agreed first — the issue proposed exposing these two fields and a maintainer replied "sound reasonable to me, feel free to raise PR to improve it", but the API shape was explicitly left open there (per-sink vs per-
settingsentry). So the concept was agreed and the shape was not. Flagging rather than claiming agreement; happy to split the API into its own PR if that is preferred.