out_influxdb: allow stripping of tag prefix #9427
Open
Conversation
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
ueli-g
force-pushed
the
influxdb-strip-tag-prefix
branch
from
d6e4d82 to
cb44828
Compare
|
Example configuration file for this change - how to write to different buckets in the same DB without adding measurement name prefixes: |
|
Debug log output |
|
valgrind memcheck output |
ueli-g
force-pushed
the
influxdb-strip-tag-prefix
branch
from
d4aa84c to
c45887d
Compare
ueli-g
requested review from
edsiper,
fujimotos,
koleini and
leonardo-albertovich
as code owners
ueli-g
changed the title
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
this PR is pending for review |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Removes a configured prefix from measurement names Signed-off-by: Ueli Graf <[email protected]>
Signed-off-by: Ueli Graf <[email protected]>
ueli-g
force-pushed
the
influxdb-strip-tag-prefix
branch
from
c45887d to
79bc485
Compare
WalkthroughAdds per-instance tag prefix stripping to the InfluxDB output plugin: a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CFG as Config Loader
participant OI as InfluxDB Instance
participant FM as Formatter
participant EM as Emitter
CFG->>OI: cb_influxdb_init (read `strip_prefix`)
Note right of OI: store `prefix` and `prefix_len`
loop For each record
FM->>FM: receive record tag
alt tag startsWith(prefix) and tag.length > prefix_len
FM->>FM: compute offset, produce tag_without_prefix
Note right of FM #dff0d8: (new/changed path)
else
FM->>FM: use original tag
end
FM->>EM: influxdb_bulk_append_header(tag_used, len)
EM-->>EM: emit line protocol
end
OI-->>OI: cb_influxdb_exit
Note right of OI #f8d7da: free(prefix) if allocated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)plugins/out_influxdb/influxdb.c (2)
🔇 Additional comments (4)
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: 1
🧹 Nitpick comments (4)
plugins/out_influxdb/influxdb.h (1)
59-62: Fields look fine; keep length type consistent with existing code.No blockers. If you later use size_t with memcmp/strlen, cast appropriately to avoid sign/width issues when calling libc.
plugins/out_influxdb/influxdb.c (3)
136-141: Guard against invalid lengths when appending header.With the above fix, prefix_offset is bounded. If you want an extra safety net:
- ret = influxdb_bulk_append_header(bulk_head, - tag + prefix_offset, - tag_len - prefix_offset, + ret = influxdb_bulk_append_header(bulk_head, + tag + prefix_offset, + (int)(tag_len - prefix_offset), seq, ctx->seq_name, ctx->seq_len);Explicit cast avoids accidental promotion if the callee expects int.
382-390: Check strdup() failure for strip_prefix.Tiny robustness gap: ctx->prefix is used unconditionally; guard OOM.
tmp = flb_output_get_property("strip_prefix", ins); if (!tmp) { - ctx->prefix = flb_strdup(""); + ctx->prefix = flb_strdup(""); } else { - ctx->prefix = flb_strdup(tmp); + ctx->prefix = flb_strdup(tmp); } - ctx->prefix_len = strlen(ctx->prefix); + if (!ctx->prefix) { + flb_errno(); + return -1; + } + ctx->prefix_len = (int) strlen(ctx->prefix);
723-728: Config map entry reads well; consider clarifying docs.Doc string could mention: “Only stripped when tag strictly longer than the prefix.” Matches implementation and avoids empty measurement names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_influxdb/influxdb.c(5 hunks)plugins/out_influxdb/influxdb.h(1 hunks)
🔇 Additional comments (1)
plugins/out_influxdb/influxdb.c (1)
617-620: Good lifecycle hygiene.prefix is freed alongside other dynamic members.
Comment on lines
+77
to
79
| int prefix_match = 0; | ||
| int prefix_offset = 0; | ||
| msgpack_object map; |
There was a problem hiding this comment.
Fix OOB read on tag and per-record state leak (prefix_offset).
- tag is a raw buffer with explicit length; strncmp(tag, …, ctx->prefix_len) can read past tag when prefix_len > tag_len.
- prefix_offset is not reset inside the event loop; a prior match can incorrectly affect subsequent records.
Apply:
@@
- char tmp[128];
- int prefix_match = 0;
- int prefix_offset = 0;
+ char tmp[128];
+ int prefix_offset = 0;
@@
- prefix_match = strncmp(tag, ctx->prefix, ctx->prefix_len);
- if (prefix_match == 0) {
- if (tag_len > ctx->prefix_len) {
- prefix_offset = ctx->prefix_len;
- }
- }
+ /* reset per-record */
+ prefix_offset = 0;
+ /* safe compare: only when prefix is non-empty and tag is longer */
+ if (ctx->prefix_len > 0 && tag_len > ctx->prefix_len) {
+ if (memcmp(tag, ctx->prefix, (size_t) ctx->prefix_len) == 0) {
+ prefix_offset = ctx->prefix_len;
+ }
+ }And ensure memcmp is declared:
#include <stdio.h>
+#include <string.h>This prevents undefined reads and guarantees no negative length is passed to influxdb_bulk_append_header.
Also applies to: 129-139
🤖 Prompt for AI Agents
In plugins/out_influxdb/influxdb.c around lines 77-79 (and similarly 129-139),
fix the OOB read and state leak by treating tag as a length-delimited buffer:
obtain tag_len from tag.via.raw.size and only compare up to that length (do not
call strncmp with ctx->prefix_len if ctx->prefix_len > tag_len); use memcmp with
the minimum of the two lengths or explicitly require tag_len >= ctx->prefix_len
before matching to avoid undefined reads, and reset prefix_offset to 0 at the
start of each event/record iteration so previous matches do not carry over; also
ensure memcmp is declared by including the proper header (string.h) so the
comparator is defined, and guard calls to influxdb_bulk_append_header so no
negative length is ever passed.
|
@ueli-g can you resolve the conflicts shown above please? |
Signed-off-by: Ueli Graf <[email protected]>
|
@eschabell thanks for the nudge. I did address the valid bug that Could you have another look please and let me know if there is anything else I should address? I deliberately did not switch to |
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.
This removes a defined prefix from measurement names which might otherwise be shared between many measurements in the same data bucket.
When writing to a range of different buckets, routing to the corresponding out_influxdb instances happens on tag matches. This change allows to match on tag prefixes, but strip them from the measurement name. This avoids having identical prefixes for all measurement names in the same data bucket.
To achieve this, read from char tag[] with an offset when writing the measurement name, provided the prefix matches the tag completely and the overlap is at most tag_length - 1 characters.
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
fluent/fluent-bit-docs#1468
Backporting
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
Configuration