pack: Use lowercase string for binary format string#11748
pack: Use lowercase string for binary format string#11748
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a conditional msgpack→JSON rewrite for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_pack.c`:
- Around line 1346-1352: The loop that writes hex digits using msgpack_pack_char
is semantically wrong; instead allocate a temporary buffer of length
obj->via.bin.size * 2, fill it with the two hex characters per input byte using
the existing table and input (use the same index/byte logic currently in the
loop), then call msgpack_pack_str_body(pck, buffer, obj->via.bin.size * 2) and
free the buffer (or use a stack array if size is bounded). Replace the
msgpack_pack_char calls with this single msgpack_pack_str_body call while
keeping the preceding msgpack_pack_str(pck, obj->via.bin.size * 2) intact so the
hex string body is packed correctly.
🪄 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: 2cdfd4d3-da32-4e56-ab84-3e672ccda852
📒 Files selected for processing (3)
src/flb_pack.ctests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.pytests/internal/pack.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
…n JSON Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
5af5840 to
a7dca93
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_pack.c`:
- Around line 1333-1366: flb_pack_msgpack_binary_to_hex currently treats a
zero-length BIN as an allocation error and can return -1 (corrupting the map);
modify flb_pack_msgpack_binary_to_hex so that when obj->via.bin.size == 0 it
does not call flb_malloc(0) but instead directly packs an empty string
(msgpack_pack_str with length 0 and no body) and returns 0; additionally, update
the caller flb_pack_msgpack_append_json_metadata to check the return value of
flb_pack_msgpack_binary_to_hex and, on error, pack a safe fallback value (e.g.,
empty string) so the map header count remains consistent and the msgpack stream
cannot become malformed.
🪄 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: 9116438b-b8e7-49f7-ae99-53a23d4c0135
📒 Files selected for processing (3)
src/flb_pack.ctests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.pytests/internal/pack.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
For human readable encoding, we need to encode trace_id and span_id for sending as JSON payloads.
Without this hex encoding, we have to handle binary tofu-ed character to search relative traces and logs.
So, we need to use hex format encoding before sending as JSON.
For just peek the contents of otel's payloads, we don't need do that.
Closes #11630.
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 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