out_chronicle: Support label and namespace mapping#11836
Open
cosmo0920 wants to merge 3 commits into
Open
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:
📝 WalkthroughWalkthroughThe Google Chronicle output plugin gains config-driven namespace and labels support: accessors resolve namespace and label values per-record, metadata is resolved once per batch (batch-split on changes), and optional ChangesChronicle namespace and labels metadata feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/runtime/out_chronicle.c (1)
153-170: ⚡ Quick winAssert label key/value pairs together to avoid false positives.
The current checks validate keys and values independently, so a mismatched pairing could still pass. Prefer asserting the expected pair objects (or parsing JSON and validating structured fields).
Suggested patch
- p = strstr(out_json, "\"key\":\"env\""); - if (!TEST_CHECK(p != NULL)) { - TEST_MSG("Expected static label key not found. Got: %s", out_json); - } - - p = strstr(out_json, "\"value\":\"production\""); - if (!TEST_CHECK(p != NULL)) { - TEST_MSG("Expected static label value not found. Got: %s", out_json); - } - - p = strstr(out_json, "\"key\":\"cluster_name\""); - if (!TEST_CHECK(p != NULL)) { - TEST_MSG("Expected dynamic label key not found. Got: %s", out_json); - } - - p = strstr(out_json, "\"value\":\"blue\""); - if (!TEST_CHECK(p != NULL)) { - TEST_MSG("Expected dynamic label value not found. Got: %s", out_json); - } + p = strstr(out_json, "\"key\":\"env\",\"value\":\"production\""); + if (!TEST_CHECK(p != NULL)) { + TEST_MSG("Expected static label pair not found. Got: %s", out_json); + } + + p = strstr(out_json, "\"key\":\"cluster_name\",\"value\":\"blue\""); + if (!TEST_CHECK(p != NULL)) { + TEST_MSG("Expected dynamic label pair not found. Got: %s", out_json); + }🤖 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 `@tests/runtime/out_chronicle.c` around lines 153 - 170, The tests currently look for keys and values separately using strstr(out_json, ...) which can yield false positives if pairs are mismatched; update the checks in tests/runtime/out_chronicle.c to assert the key/value pairs together (e.g. search for the combined substring representing the pair such as "\"key\":\"env\",\"value\":\"production\"" and similarly for cluster_name/blue) or parse out_json into a JSON object and validate that for the same label object the key equals "env" and value equals "production" (use the existing TEST_CHECK/TEST_MSG pattern and the same variables p and out_json or the parsed structure to report failures).
🤖 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 `@plugins/out_chronicle/chronicle_conf.c`:
- Around line 71-74: The validation wrongly allows empty label values and
mis-detects record accessors by scanning for any '$' anywhere; update the check
around key, key_len, value, value_len to treat value_len == 0 as an error as
well (same failure path and log), and change the RA detection logic that
currently inspects value for any '$' to only treat it as a record accessor when
the first character of value is '$' (i.e., use value[0] == '$' or equivalent) so
static values containing '$' are accepted; update references in chronicle_conf.c
where key/key_len/value/value_len and the RA detection are used.
In `@plugins/out_chronicle/chronicle.c`:
- Around line 883-897: The code currently uses metadata_resolved as a one-time
gate so chronicle_metadata_resolve is called only for the first record in a
batch, causing later records to reuse the same namespace/resolved_labels; remove
that gating and call chronicle_metadata_resolve for each record (e.g., before
sending each log_event) using the same parameters (tag, tag_len,
*log_event.body, &namespace, &resolved_labels, &label_count), ensure you
destroy/free any previous namespace and resolved_labels
(flb_sds_destroy(namespace) and
chronicle_resolved_labels_destroy(&resolved_labels)) before reassigning to avoid
leaks, and handle the non-zero return path the same way it does now (cleanup and
return error) so each record gets its correct per-record namespace/labels.
In `@tests/runtime/out_chronicle.c`:
- Around line 138-143: The new formatter callbacks use strstr on
res_data/out_json without checking for NULL; add a null guard that checks if
res_data (or out_json) is NULL before calling strstr and handle the error path
cleanly (e.g., set_output_invoked to indicate failure, fail the test or return
early) so the test does not segfault; update both occurrences around the strstr
calls (the variables res_data/out_json and the pointer p) to bail out with a
clear test failure when NULL is encountered.
---
Nitpick comments:
In `@tests/runtime/out_chronicle.c`:
- Around line 153-170: The tests currently look for keys and values separately
using strstr(out_json, ...) which can yield false positives if pairs are
mismatched; update the checks in tests/runtime/out_chronicle.c to assert the
key/value pairs together (e.g. search for the combined substring representing
the pair such as "\"key\":\"env\",\"value\":\"production\"" and similarly for
cluster_name/blue) or parse out_json into a JSON object and validate that for
the same label object the key equals "env" and value equals "production" (use
the existing TEST_CHECK/TEST_MSG pattern and the same variables p and out_json
or the parsed structure to report failures).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b42c43d8-6d76-4b12-84bd-f9607f85f4a4
📒 Files selected for processing (4)
plugins/out_chronicle/chronicle.cplugins/out_chronicle/chronicle.hplugins/out_chronicle/chronicle_conf.ctests/runtime/out_chronicle.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df047220c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
cosmo0920
force-pushed
the
cosmo0920-label-and-namespace-mapping-chronicle
branch
from
519af6b to
38d78bb
Compare
cosmo0920
changed the title
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 `@plugins/out_chronicle/chronicle.c`:
- Around line 975-987: The failure path after chronicle_metadata_resolve(...)
currently returns -1 without freeing any previously queued buffered entries;
update the error branch in chronicle_format (the block handling ret != 0) to
free the queued entry_list before returning by calling the same cleanup used on
successful/normal teardown (i.e., invoke the function that destroys/clears the
buffered entries — the one used elsewhere for entry_list cleanup) in addition to
the existing flb_sds_destroy( log_text / record_namespace / namespace ) and
chronicle_resolved_labels_destroy(&record_labels / &resolved_labels) calls so no
entry_list memory is leaked on metadata-resolution failure.
In `@tests/runtime/out_chronicle.c`:
- Around line 201-235: The test callback
cb_check_format_split_on_metadata_change currently forces the invocation count
to 1 via set_output_invoked(1), which hides whether a second split payload was
emitted; change the callback to increment the invocation counter (e.g., call
set_output_invoked(get_output_invoked()+1) or use an increment function) and add
assertions that when invoked a second time the res_data contains the second
payload markers ("tenant-b", "green", "record two"); update the test harness
assertions (where cb_check_format_split_on_metadata_change is used) to expect at
least two invocations instead of only one so the test fails if the second split
is dropped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f33ca8d8-3bee-451c-9297-c46e28027d74
📒 Files selected for processing (2)
plugins/out_chronicle/chronicle.ctests/runtime/out_chronicle.c
cosmo0920
changed the title
cosmo0920
force-pushed
the
cosmo0920-label-and-namespace-mapping-chronicle
branch
from
38d78bb to
475a48f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/out_chronicle/chronicle.c`:
- Around line 902-903: chronicle_format() must initialize its output pointers to
deterministic defaults so callers (like cb_chronicle_flush()) don't attempt to
free garbage when chronicle_format() returns early; at the top of
chronicle_format(), set *out_data = NULL, *out_size = 0, and *out_offset = 0 (or
equivalent deterministic values) so payload_buf in cb_chronicle_flush() will be
NULL if an error occurs and flb_sds_destroy(payload_buf) is safe; ensure any
early-return error paths do not overwrite these defaults.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5db04ec5-1291-459d-808d-cff5d8ff67a2
📒 Files selected for processing (4)
plugins/out_chronicle/chronicle.cplugins/out_chronicle/chronicle.hplugins/out_chronicle/chronicle_conf.ctests/runtime/out_chronicle.c
7 tasks
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
…mappings Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-label-and-namespace-mapping-chronicle
branch
from
475a48f to
deaf050
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugins/out_chronicle/chronicle.c`:
- Around line 837-851: flb_ra_translate_check may return an empty string which
should be treated like NULL; update the post-call check for the resolved
namespace (the result stored in *namespace after calling flb_ra_translate_check
with ctx->namespace_ra, tag, tag_len, map) to treat both NULL and empty-string
results as unresolved and trigger the existing fallback: if the translated value
is NULL or its first character is '\0' then fall back to
flb_sds_create(ctx->namespace) when ctx->namespace is set, otherwise call
flb_plg_warn(ctx->ins, "namespace_key '%s' did not resolve for this batch",
ctx->namespace_key). Ensure you reference ctx->namespace_ra,
flb_ra_translate_check, *namespace, and ctx->namespace in the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 553b6feb-9ecc-4bad-b88b-bdef1f5cac3d
📒 Files selected for processing (4)
plugins/out_chronicle/chronicle.cplugins/out_chronicle/chronicle.hplugins/out_chronicle/chronicle_conf.ctests/runtime/out_chronicle.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
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.
To follow the requested structure of unstructuredlogentries format on https://docs.cloud.google.com/chronicle/docs/reference/ingestion-api#unstructuredlogentries,
we need to add a metadata like namespace and labels which should be defined by users.
Closes #11833.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
fluent/fluent-bit-docs#2582
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests