opentelemetry: general fixes for log handling #10693
Conversation
- Add NULL check in otlp_pack_any_value function to handle NULL body parameter - Add NULL check for body access in conditional check to prevent segfault - Handle OPENTELEMETRY__PROTO__COMMON__V1__ANY_VALUE__VALUE__NOT_SET case by packing nil value Signed-off-by: Eduardo Silva <[email protected]>
- Add proper NULL handling for empty map values (unset values) - Fix missing value handling to pack as empty string - Complete logic flow to handle all OpenTelemetry value states Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
WalkthroughThis change updates msgpack CMake configuration to set a stack size macro, improves handling of unset and empty values in OpenTelemetry log processing (including packing nil for unset values), enhances JSON tokenization and msgpack conversion logic, broadens accepted types in OpenTelemetry utilities, and adds new tests for edge cases involving empty arrays and maps. Changes
Sequence Diagram(s)sequenceDiagram
participant OpenTelemetryLog as OpenTelemetry Log Processor
participant MsgpackPacker as Msgpack Packer
OpenTelemetryLog->>MsgpackPacker: Process AnyValue
alt VALUE__NOT_SET
MsgpackPacker->>MsgpackPacker: Pack nil value
else Other value types
MsgpackPacker->>MsgpackPacker: Pack according to value type
end
sequenceDiagram
participant JSONParser as JSON Tokenizer
participant MsgpackConverter as Msgpack Converter
JSONParser->>JSONParser: Parse JSON tokens
alt Token buffer too small
JSONParser->>JSONParser: Resize token buffer
JSONParser->>JSONParser: Reset parser state
JSONParser->>JSONParser: Re-parse from start
end
JSONParser->>MsgpackConverter: Provide tokens
MsgpackConverter->>MsgpackConverter: Validate token boundaries
MsgpackConverter->>MsgpackConverter: Convert valid tokens to msgpack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
edsiper
force-pushed
the
otel-logs-fixes
branch
from
c1eb0e5 to
db28466
Compare
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
edsiper
force-pushed
the
otel-logs-fixes
branch
from
db28466 to
ac0dfc8
Compare
edsiper
requested review from
fujimotos,
niedbalski,
patrick-stephens,
celalettin1286,
leonardo-albertovich and
koleini
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmake/msgpack.cmake (1)
10-11: Fix the inconsistency between comment and implementation.The comment states "define preprocessor MSGPACK_EMBED_STACK_SIZE to 1024" but the actual implementation sets it to 64. Update the comment to match the implementation.
-# define preprocessor MSGPACK_EMBED_STACK_SIZE to 1024 +# define preprocessor MSGPACK_EMBED_STACK_SIZE to 64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmake/msgpack.cmake(1 hunks)plugins/in_opentelemetry/opentelemetry_logs.c(1 hunks)src/flb_pack.c(2 hunks)src/opentelemetry/flb_opentelemetry_utils.c(5 hunks)tests/internal/data/opentelemetry/test_cases.json(1 hunks)tests/internal/pack.c(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (12)
plugins/in_opentelemetry/opentelemetry_logs.c (1)
179-182: LGTM: Proper handling of unset protobuf values.The explicit handling of
VALUE__NOT_SETcase by packing nil is a good improvement that ensures consistent msgpack encoding for unset OpenTelemetry values. This aligns well with the broader improvements in handling empty/missing values throughout the codebase.tests/internal/pack.c (2)
973-1005: LGTM: Comprehensive test for nested JSON with empty arrays.The new test effectively validates the packing of deeply nested JSON structures containing empty arrays, which aligns with the improvements made to JSON tokenization and msgpack conversion logic. The test properly:
- Uses a complex nested structure to exercise edge cases
- Validates successful packing with error checking
- Unpacks the result to verify msgpack correctness
- Includes proper resource cleanup
1152-1152: LGTM: Test properly registered.The test is correctly added to the test list.
src/flb_pack.c (2)
82-83: LGTM: Essential parser state reset after token buffer resize.The addition of
jsmn_init(&state->parser)ensures the parser correctly restarts from the beginning of the input after expanding the tokens array. This prevents parsing inconsistencies that could occur when the buffer is resized mid-parse.
223-223: LGTM: Improved token boundary validation.The simplified and broadened validation check (
t->start < 0 || t->end <= 0) is more comprehensive than the previous implementation, properly catching invalid token boundaries regardless of specific zero combinations.tests/internal/data/opentelemetry/test_cases.json (3)
1229-1254: Well-designed test case for empty map and missing value handling.This test case effectively covers the edge cases for handling empty maps as null values and missing "value" keys as empty strings in OpenTelemetry key-value lists. The expected output correctly demonstrates the intended behavior.
1255-1297: Comprehensive test for deeply nested array-wrapped structures.This test case effectively validates the handling of complex nested structures where maps are wrapped inside arrays within key-value lists. The deep nesting pattern and expected flattened output demonstrate proper parsing logic.
1298-1325: Good coverage for unwrapped plain map structures.This test case validates the handling of plain maps used directly as values in key-value lists, ensuring that mixed data types (strings and integers) are properly preserved without wrapper structures.
src/opentelemetry/flb_opentelemetry_utils.c (4)
138-142: Improved type flexibility for arrayValue handling.The expansion to accept both
MSGPACK_OBJECT_ARRAYandMSGPACK_OBJECT_MAPtypes for "arrayValue" keys enhances the parser's ability to handle diverse OpenTelemetry data structures, as demonstrated in the corresponding test cases.
377-379: Minor formatting improvement for readability.The separation of the conditional logic and return statement enhances code readability without changing functionality.
450-505: Robust implementation for handling empty and missing values.The introduction of boolean flags (
pack_null_value,pack_string_value,pack_value) provides clear control flow for different value states. The logic correctly identifies empty maps as null values and missing "value" keys as empty strings, aligning with the OpenTelemetry specification.
515-539: Well-structured value packing logic with proper precedence.The conditional structure properly handles the three distinct cases (null values, empty strings, and actual values) in the correct order of precedence. The implementation ensures consistent msgpack encoding for all edge cases.
This PR fixes #10672 and #10673
Couple of fixes were needed for cases with messages with long nested array/maps, as well some fixes for packaging for not set values while processing AnyValues.
Added some test cases and fixed the JSON packer that shown some issues packaging messages with many nested levels (recursion)
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.