opentelemetry: allow empty strings for TraceID and SpanID (logs)#10956
opentelemetry: allow empty strings for TraceID and SpanID (logs)#10956
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
WalkthroughUpdates OpenTelemetry log parsing to treat empty traceId/spanId as absent, bypassing validation and emission. Maintains strict hex and length checks when IDs are non-empty. Adds a test case verifying logs with empty IDs are processed without trace/span metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Ingest as Input Log
participant Parser as OTEL Log Parser
participant Validator as ID Validator
participant Emitter as Encoder/Emitter
Ingest->>Parser: Log record with traceId/spanId
Parser->>Validator: Check traceId/spanId
alt Empty ID
note over Validator: Size == 0 ➜ treat as absent
Validator-->>Parser: No ID metadata
else Non-empty ID
Validator->>Validator: Length check (traceId=32, spanId=16)
Validator->>Validator: Hex check (0-9,a-f)
Validator-->>Parser: Valid or error
end
Parser->>Emitter: Emit log (include IDs only if valid & present)
Emitter-->>Ingest: Output log record
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
tests/internal/data/opentelemetry/test_cases.json (1)
1310-1329: Consider adding test cases for partial empty ID scenarios.While this test case covers both IDs being empty, consider adding test cases for:
- Empty traceId with valid spanId
- Valid traceId with empty spanId
These scenarios would ensure the implementation correctly handles mixed empty/non-empty ID combinations.
Would you like me to generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/opentelemetry/flb_opentelemetry_logs.c(4 hunks)tests/internal/data/opentelemetry/test_cases.json(1 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). (29)
- 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: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=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 (-DFLB_SIMD=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 (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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 (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=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=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (3)
src/opentelemetry/flb_opentelemetry_logs.c (2)
162-192: LGTM! Empty trace_id is now correctly treated as absent.The implementation properly handles empty trace_id strings by treating them as absent (setting to NULL) and skipping validation, while maintaining strict 32-character hex validation for non-empty values. The logic is clear and aligns with the PR objectives.
200-230: LGTM! Empty span_id is now correctly treated as absent.The implementation mirrors the trace_id logic, properly handling empty span_id strings by treating them as absent and skipping validation, while maintaining strict 16-character hex validation for non-empty values. The parallel structure with trace_id handling ensures consistency.
tests/internal/data/opentelemetry/test_cases.json (1)
1310-1329: LGTM! Test case correctly verifies empty trace/span ID behavior.The new test case properly validates that logs with empty traceId and spanId are processed without including trace/span metadata in the output. The expected output correctly shows the log is processed with standard group metadata and log body, while omitting trace_id and span_id from the log_metadata.otlp object.
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