out_stackdriver: fix batch drop on invalid labels - #11539
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a centralized per-record validator ChangesStackdriver plugin refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
fcf50e3 to
7928a60
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_stackdriver/stackdriver.c (1)
1927-1929:⚠️ Potential issue | 🟡 MinorLog a summary reason before returning on all-invalid batches.
If prescan rejects every record, the function returns before the logged pass, so there is no message explaining that all entries were skipped for invalid
insertIdor labels. Emitting one summary warning here would make this failure mode diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_stackdriver/stackdriver.c` around lines 1927 - 1929, When prescan leaves no valid records (array_size == 0) add a summary warning log immediately before the early return to explain why the batch is dropped (e.g., "all entries skipped: invalid insertId or labels"); use the same logging facility already used in this file (the local logger used elsewhere in stackdriver.c) and reference the prescan result/array_size check so the message is emitted whenever array_size == 0 instead of returning silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_stackdriver/stackdriver.c`:
- Around line 1917-1923: The loop that decrements array_size when
should_skip_record() returns true adjusts the emitted count locally but
cb_stackdriver_flush() still reports event_chunk->total_events; update the code
to propagate the actual emitted/processed count to cb_stackdriver_flush() (e.g.,
compute emitted_count by starting from event_chunk->total_events and
decrementing for each skipped record inside the
flb_log_event_decoder_next()/should_skip_record() loop or accumulate
emitted_count directly) and change metric reporting in cb_stackdriver_flush() to
use this emitted_count (or a new parameter name you add) instead of
event_chunk->total_events so dropped records are reflected in flush and
dropped-record metrics.
---
Outside diff comments:
In `@plugins/out_stackdriver/stackdriver.c`:
- Around line 1927-1929: When prescan leaves no valid records (array_size == 0)
add a summary warning log immediately before the early return to explain why the
batch is dropped (e.g., "all entries skipped: invalid insertId or labels"); use
the same logging facility already used in this file (the local logger used
elsewhere in stackdriver.c) and reference the prescan result/array_size check so
the message is emitted whenever array_size == 0 instead of returning silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d8e76ba-c3cc-4894-b540-33bba85548b2
⛔ Files ignored due to path filters (4)
tests/runtime/data/stackdriver/stackdriver_batch_all_labels_not_a_map.logis excluded by!**/*.logtests/runtime/data/stackdriver/stackdriver_batch_first_record_labels_not_a_map.logis excluded by!**/*.logtests/runtime/data/stackdriver/stackdriver_batch_labels_not_a_map.logis excluded by!**/*.logtests/runtime/data/stackdriver/stackdriver_batch_mixed_errors.logis excluded by!**/*.log
📒 Files selected for processing (3)
plugins/out_stackdriver/stackdriver.ctests/runtime/data/stackdriver/stackdriver_test_labels.htests/runtime/out_stackdriver.c
7928a60 to
faa836c
Compare
c6aadf8 to
154c0fe
Compare
When a single record contains a logging.googleapis.com/labels field that is not a map, Stackdriver can drop the entire batch. This causes data loss for valid records in the same batch. Extract a shared should_skip_record() helper that validates insertId and labels fields. Use it in the prescan loop and at the top of the main packing loop so invalid records are skipped before field extraction. Report the number of formatted records back to the flush path so Stackdriver processed, retry, and dropped-record metrics use the records actually sent. Treat an all-invalid batch as locally dropped instead of a retryable formatter failure. This change leaves existing batch-level errors unchanged, including decoder initialization, k8s local_resource_id processing, and JSON serialization failures. Signed-off-by: Yu Yi <yiyu@yiyu.me>
Add Stackdriver runtime formatter coverage for invalid labels in both single-record and batch flows. Cover default labels, custom labels_key, records with fields that used to require cleanup, mixed invalid insertId and labels, first-record skips, and all-invalid batches. Signed-off-by: Yu Yi <yiyu@yiyu.me>
154c0fe to
dfbf56c
Compare
|
This PR is ready for review. It has been rebased onto current master (including the recent stackdriver memory-leak fixes and grouped-log counter parity changes) and the full Note on the 3 failing checks — all are unrelated to this change:
This PR only touches |
| msgpack_object *payload_labels_ptr; | ||
|
|
||
| /* Check insertId */ | ||
| in_status = validate_insert_id(&insert_id_obj, obj); |
There was a problem hiding this comment.
A Performance Optimization:
This iteration over the msgpack content (against the log entry) already calls validate_insert_id and get_payload_labels to traverse the list twice. Can you update the function to return the result of validate_insert_id and get_payload_labels as well?
With the change, we don't need to call the function in stackdriver_format again.
There was a problem hiding this comment.
Good call — done in f6d50ee. should_skip_record() now returns the validated insertId (status + object) and the payload-labels pointer through out-params, and stackdriver_format() reuses them in the packing loop instead of calling validate_insert_id() and get_payload_labels() a second time per record. The prescan pass calls it with NULL out-params. Full flb-rt-out_stackdriver suite passes locally.
should_skip_record() already validates insertId and payload labels by traversing the record. Return those results (insertId status and object, payload labels pointer) so stackdriver_format() reuses them instead of calling validate_insert_id() and get_payload_labels() a second time per record in the packing loop. Addresses review feedback on the double traversal. Signed-off-by: Yu Yi <yiyu@google.com>
2 participants
Description
When a single record contains a
logging.googleapis.com/labelsfield that is not a map, theout_stackdriverplugin currently drops the entire batch. This causes data loss for all other valid records in that batch.Root Cause
In
stackdriver_format(), the labels type check had two problems:destroy_http_request(), causing a memory leak).Fix
Extracted a shared
should_skip_record()helper that validates bothinsertIdandlabelsin one place:This helper is now used:
log_errors=FLB_FALSE) to correctly computearray_size(entries count).log_errors=FLB_TRUE), before any field extraction, so invalid records are skipped cleanly with no cleanup needed.Metrics
The number of formatted records is reported back to
cb_stackdriver_flush()so processed, retry, and dropped-record metrics reflect the records actually sent:should_skip_record()are added to the dropped-records counter once the flush completes without retry.FLB_OK+ dropped-records counter) instead of a retryable formatter failure, which previously caused infinite retries of an unformattable chunk.Scope
The following batch-level drop scenarios remain unchanged as they affect shared batch-level state by design:
flb_log_event_decoder_init()failureflb_log_event_decoder_next()failurelocal_resource_idextraction/processing failures (k8s_container,k8s_node,k8s_pod)process_local_resource_id()failuresTesting
New Tests (7 total)
labels_not_a_maplabels_not_a_map_custom_keylabels_keywith string value → no outputlabels_not_a_map_with_extracted_fieldsbatch_labels_not_a_mapbatch_first_record_labels_not_a_mapbatch_all_records_labels_not_a_mapbatch_mixed_errorsBatch tests with content assertions (
batch_labels_not_a_map,batch_first_record_labels_not_a_map,batch_mixed_errors) use tail/file-backed fixtures to guarantee single-batch semantics. Thebatch_all_records_labels_not_a_maptest uses a fixture file as well but only asserts no output.Regression Tests
Full
flb-rt-out_stackdriversuite passes locally after rebasing onto current master (including the recent stackdriver memory-leak fixes, SDS append checks, and grouped-log counter parity changes).Summary by CodeRabbit
Bug Fixes
Tests