feat(exporterhelper): add queue payload codec hook (SAW-6556 backport)#5
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe changes introduce a new queue payload codec mechanism for batch exporters. A public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Mar 2, 2026
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
1 issue found across 3 files
Confidence score: 3/5
- Order-dependent behavior in
exporter/exporterhelper/internal/base_exporter.gomeansWithQueueBatchPayloadCodeccan be silently ignored if passed afterWithQueue/WithQueueBatch, which could change runtime behavior unexpectedly. - This is a concrete user-facing configuration pitfall (severity 6/10), so there’s some merge risk despite being localized.
- Pay close attention to
exporter/exporterhelper/internal/base_exporter.go- option ordering can cause the payload codec to be skipped.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="exporter/exporterhelper/internal/base_exporter.go">
<violation number="1" location="exporter/exporterhelper/internal/base_exporter.go:258">
P2: `WithQueueBatchPayloadCodec` is order-dependent and can be silently ignored when provided after `WithQueue`/`WithQueueBatch`.</violation>
</file>
Linked issue analysis
Linked issue: SAW-6556: Compressed Sending Queue + file_storage for Collector exporters
| Status | Acceptance criteria | Notes |
|---|---|---|
| ❌ | Add optional queue payload compression for loadbalancingexporter only. | No LB exporter integration present in diffs |
| Validate compatibility with persistent sending queue (sending_queue.storage: file_storage/...) for burst absorption with bounded RAM. | Hook wraps encoding for persistent queue but no validation tests | |
| ❌ | Provide benchmark evidence of tradeoffs for this exporter path. | No benchmarks or results included |
| ❌ | Implement optional payload compression path in loadbalancingexporter queue handling: sending_queue.payload_compression: none|snappy|zstd (default none). | No config or LB exporter code added |
| Ensure compatibility with existing queue semantics: ordering, retry/backoff, shutdown/drain, restart/recovery for persistent queue. | Codec is integrated into encoding but semantics tests are missing | |
| ✅ | Keep existing sending_queue.storage semantics (file_storage/), do not introduce memory|file_storage enum. | No storage enum introduced; persistent-encoding check retained |
| ❌ | Add telemetry for: enqueue/dequeue bytes (raw vs stored), compression ratio, compression/decompression CPU time. | No telemetry additions in diffs |
| ✅ | Feature off by default; no behavior change when disabled. | Codec applied only when provided; nil leaves behavior unchanged |
| ❌ | Applies only to loadbalancingexporter. | Hook added generically to exporterhelper, not LB-only |
| Correctness tests pass for: enqueue/dequeue integrity. | Unit test for Marshal/Unmarshal exists but not full enqueue/dequeue tests | |
| ❌ | Correctness tests pass for: retry behavior with compressed entries. | No retry behavior tests for compressed entries |
| ❌ | Correctness tests pass for: persistent queue restart/recovery. | No restart/recovery tests present |
| ❌ | Benchmarks at minimum: 14k EPS (stable). | No benchmark artifacts or results |
| ❌ | Benchmarks: 16k EPS (stress). | No benchmark artifacts or results |
| ❌ | Benchmark variant: memory queue + none. | No benchmark variants provided |
| ❌ | Benchmark variant: memory queue + snappy (and/or zstd). | No benchmark variants provided |
| ❌ | Benchmark variant: file_storage queue + none. | No benchmark variants provided |
| ❌ | Benchmark variant: file_storage queue + compression. | No benchmark variants provided |
| ❌ | Report: achieved EPS, p95/p99, error/fallback rates. | No report or metrics included |
| ❌ | Report: collector CPU and queue fill slope. | No report or metrics included |
| ❌ | Report: RSS and disk usage. | No report or metrics included |
| ❌ | Report: codec break-even guidance. | No analysis or guidance provided |
| ❌ | Implementation in LB exporter path. | No modifications to loadbalancingexporter found |
| ❌ | Tests + benchmark artifacts. | Only unit tests for encoding exist; no benchmarks |
| ❌ | Tuning/failure-mode documentation for operators. | No documentation added in diffs |
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![sawmills-architect-review[bot]](https://avatars.githubusercontent.com/in/2910947?s=60&v=4){kind=small}
There was a problem hiding this comment.
[ARCH-REVIEW] REQUEST_CHANGES — 1 concern, 1 nit.
Clean backport conceptually: the payloadCodecEncoding wrapper is well-structured, the interface is minimal and correct, and the roundtrip test covers the happy path + invalid prefix case. One silent footgun in the option wiring order needs to be fixed before merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
exporter/exporterhelper/internal/base_exporter.go (1)
214-233:⚠️ Potential issue | 🟠 MajorCodec hook is order-dependent and can be silently skipped.
WithQueueBatchPayloadCodeconly sets a field, while wrapping happens insideWithQueueBatch. If callers passWithQueueBatch(...)beforeWithQueueBatchPayloadCodec(...), the codec is never applied.💡 Proposed fix (apply wrapping after all options are parsed)
func NewBaseExporter(set exporter.Settings, signal pipeline.Signal, pusher sender.SendFunc[request.Request], options ...Option) (*BaseExporter, error) { be := &BaseExporter{ Set: set, timeoutCfg: NewDefaultTimeoutConfig(), } for _, op := range options { if err := op(be); err != nil { return nil, err } } + + if be.queuePayloadCodec != nil && be.queueBatchSettings.Encoding != nil { + be.queueBatchSettings.Encoding = payloadCodecEncoding{ + encoding: be.queueBatchSettings.Encoding, + codec: be.queuePayloadCodec, + } + } // Consumer Sender is always initialized. be.firstSender = sender.NewSender(pusher) @@ func WithQueueBatch(cfg queuebatch.Config, set queuebatch.Settings[request.Request]) Option { return func(o *BaseExporter) error { if !cfg.Enabled { o.ExportFailureMessage += " Try enabling sending_queue to survive temporary failures." return nil } - if o.queuePayloadCodec != nil && set.Encoding != nil { - set.Encoding = payloadCodecEncoding{ - encoding: set.Encoding, - codec: o.queuePayloadCodec, - } - } if cfg.StorageID != nil && set.Encoding == nil { return errors.New("`Settings.Encoding` must not be nil when persistent queue is enabled") } o.queueBatchSettings = set o.queueCfg = cfg return nil } }Also applies to: 256-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@exporter/exporterhelper/internal/base_exporter.go` around lines 214 - 233, The issue is that wrapping the queue settings with the payload codec is order-dependent: WithQueueBatch currently wraps only when the codec was set first, so callers that apply WithQueueBatch before WithQueueBatchPayloadCodec silently skip the codec; fix by making the codec application idempotent in both places: keep WithQueueBatch assigning o.queueBatchSettings and o.queueCfg (but do not expect codec order), and update WithQueueBatchPayloadCodec to, after setting o.queuePayloadCodec, check if o.queueBatchSettings.Encoding != nil and then wrap o.queueBatchSettings.Encoding with payloadCodecEncoding (using the same wrapper logic currently in WithQueueBatch), so whichever option runs second will apply the codec to the already-stored settings; reference functions/fields: WithQueueBatch, WithQueueBatchPayloadCodec, BaseExporter.queuePayloadCodec, BaseExporter.queueBatchSettings, payloadCodecEncoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@exporter/exporterhelper/internal/base_exporter.go`:
- Around line 214-233: The issue is that wrapping the queue settings with the
payload codec is order-dependent: WithQueueBatch currently wraps only when the
codec was set first, so callers that apply WithQueueBatch before
WithQueueBatchPayloadCodec silently skip the codec; fix by making the codec
application idempotent in both places: keep WithQueueBatch assigning
o.queueBatchSettings and o.queueCfg (but do not expect codec order), and update
WithQueueBatchPayloadCodec to, after setting o.queuePayloadCodec, check if
o.queueBatchSettings.Encoding != nil and then wrap o.queueBatchSettings.Encoding
with payloadCodecEncoding (using the same wrapper logic currently in
WithQueueBatch), so whichever option runs second will apply the codec to the
already-stored settings; reference functions/fields: WithQueueBatch,
WithQueueBatchPayloadCodec, BaseExporter.queuePayloadCodec,
BaseExporter.queueBatchSettings, payloadCodecEncoding.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
exporter/exporterhelper/internal/base_exporter.goexporter/exporterhelper/internal/base_exporter_test.goexporter/exporterhelper/queue_batch.go
There was a problem hiding this comment.
[ARCH-REVIEW] Re-review: No blockers. Prior concern resolved, one nit.
The option-ordering footgun (🟡 from prior review) is fully addressed — applyQueuePayloadCodec() now runs in NewBaseExporter after all options are applied, and the double-wrap guard is clean. Both ordering variants are tested. All threads resolved. One 🟢 nit on duplicate interface definitions flagged inline.
Author
|
Addressed latest thread:
|
Author
|
Addressed latest thread:
|
Author
|
CI sweep update:
|
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
1 participant
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.
Summary
QueueBatchPayloadCodecinterface andWithQueueBatchPayloadCodecoptionValidation
Context
Backport branch for SAW-6556 to avoid full collector/contrib uplift in this ticket.
Summary by cubic
Adds a queue payload codec hook in exporterhelper to enable optional payload compression for sending queues. Supports SAW-6556 (loadbalancingexporter-only) without changing default behavior.
New Features
Refactors
Written for commit 1db2895. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests