test(buffer): verify deeply nested JSON events do not corrupt disk buffer (LOG-9386) - #3377
test(buffer): verify deeply nested JSON events do not corrupt disk buffer (LOG-9386)#3377vparfonov wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds functional coverage for deeply nested JSON with AtLeastOnce HTTP output buffering, verifying shallow messages remain deliverable and collector logs record expected handling without protobuf or crash failures. ChangesNested JSON HTTP output test
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoTest deeply nested JSON handling with disk-buffered HTTP output
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/outputs/http/forward_to_http_test.go`:
- Around line 215-224: Update the deep-event assertions near
framework.ReadCollectorLogs to verify the collector container restart count is
zero after sending the event, rather than relying only on current-instance log
substrings. If the restart count is nonzero, inspect the terminated container’s
previous logs so crashes are detected and diagnosed; retain the existing
dropped-event and nesting-warning checks.
- Around line 211-213: Strengthen the assertions in the HTTP output test after
ReadRawApplicationLogsFrom by verifying the delivered raw output contains or
parses the expected “shallow” payload. Keep the existing error and non-empty
checks, but ensure the test distinguishes the shallow event from an over-nested
event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0e64908-87be-4cad-b67d-a467f3cf348f
📒 Files selected for processing (1)
test/functional/outputs/http/forward_to_http_test.go
| raw, err := framework.ReadRawApplicationLogsFrom(string(obs.OutputTypeHTTP)) | ||
| Expect(err).To(BeNil(), "Expected no errors reading the logs") | ||
| Expect(raw).ToNot(BeEmpty(), "Expected the shallow message to be delivered") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the delivered record is the shallow event.
A non-empty HTTP response can be the over-nested event. Verify the output contains/parses the "shallow" payload so this test actually covers continued delivery.
Proposed assertion
Expect(err).To(BeNil(), "Expected no errors reading the logs")
Expect(raw).ToNot(BeEmpty(), "Expected the shallow message to be delivered")
+ Expect(strings.Join(raw, "\n")).To(ContainSubstring("shallow"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raw, err := framework.ReadRawApplicationLogsFrom(string(obs.OutputTypeHTTP)) | |
| Expect(err).To(BeNil(), "Expected no errors reading the logs") | |
| Expect(raw).ToNot(BeEmpty(), "Expected the shallow message to be delivered") | |
| raw, err := framework.ReadRawApplicationLogsFrom(string(obs.OutputTypeHTTP)) | |
| Expect(err).To(BeNil(), "Expected no errors reading the logs") | |
| Expect(raw).ToNot(BeEmpty(), "Expected the shallow message to be delivered") | |
| Expect(strings.Join(raw, "\n")).To(ContainSubstring("shallow")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/outputs/http/forward_to_http_test.go` around lines 211 - 213,
Strengthen the assertions in the HTTP output test after
ReadRawApplicationLogsFrom by verifying the delivered raw output contains or
parses the expected “shallow” payload. Keep the existing error and non-empty
checks, but ensure the test distinguishes the shallow event from an over-nested
event.
| collectorLogs, err := framework.ReadCollectorLogs() | ||
| Expect(err).To(BeNil()) | ||
| Expect(collectorLogs).ToNot(ContainSubstring("InvalidProtobufPayload"), | ||
| "Vector should not crash with InvalidProtobufPayload on deeply nested events") | ||
| Expect(collectorLogs).ToNot(ContainSubstring("failed to decoded record"), | ||
| "Vector should not fail to decode buffered records") | ||
| Expect(collectorLogs).To(ContainSubstring("Events dropped"), | ||
| "Expected over-nested event to be reported as dropped") | ||
| Expect(collectorLogs).To(ContainSubstring("Event nesting cost exceeds maximum"), | ||
| "Expected nesting cost warning for the over-nested event") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Detect collector restarts, not just current log text.
ReadCollectorLogs() runs oc logs for the current container instance. Vector can crash and restart, then deliver the shallow message, without these substring checks seeing the terminated instance’s logs. Assert the collector container restart count remains zero (and inspect previous logs when nonzero) after sending the deep event.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/outputs/http/forward_to_http_test.go` around lines 215 - 224,
Update the deep-event assertions near framework.ReadCollectorLogs to verify the
collector container restart count is zero after sending the event, rather than
relying only on current-instance log substrings. If the restart count is
nonzero, inspect the terminated container’s previous logs so crashes are
detected and diagnosed; retain the existing dropped-event and nesting-warning
checks.
Code Review by Qodo
Context used✅ Compliance rules (platform):
9 rules 1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| Expect(err).To(BeNil(), "Expected no errors reading the logs") | ||
| Expect(raw).ToNot(BeEmpty(), "Expected the shallow message to be delivered") | ||
|
|
||
| collectorLogs, err := framework.ReadCollectorLogs() |
There was a problem hiding this comment.
1. Collector restart goes undetected 🐞 Bug ☼ Reliability
The test checks only the current collector instance's logs, so a crash followed by a container restart can lose the failure logs and still pass after the shallow event is delivered. This does not reliably verify the test's core no-crash requirement.
Agent Prompt
## Issue description
The regression test can pass after the collector container crashes and restarts because it reads only the current container instance's logs and never checks restart history.
## Issue Context
`ReadCollectorLogs` invokes ordinary `oc logs`, which excludes a previous container instance. The functional pod also does not set a non-restarting policy, so the test must explicitly verify that the collector's restart count remains unchanged.
## Fix Focus Areas
- test/functional/outputs/http/forward_to_http_test.go[211-224]
- test/framework/functional/read.go[208-210]
- test/framework/functional/framework.go[290-303]
- internal/runtime/core.go[27-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vparfonov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…ffer (LOG-9386) Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
|
@vparfonov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Description
Verify that deeply nested JSON events (>32 protobuf nesting levels) don't crash Vector when disk buffering if enabled (AtLeastOnce delivery mode) and JSON parse filter.
/cc @Clee2691
/assign @jcantrill
Links
Summary by CodeRabbit