test(otel): add unit tests for empty events/links flattening (#1198)#1584
test(otel): add unit tests for empty events/links flattening (#1198)#1584LeC-D wants to merge 1 commit intoparseablehq:mainfrom
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
WalkthroughAdded a suite of unit tests to src/otel/traces.rs validating flattening behavior for events, links, and span records, including empty-input edge cases and known-field / encoding consistency checks. No production code or API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
I have read the CLA Document and I hereby sign the CLA |
|
Thanks for the PR @LeC-D - looks like the commit email id is not linked to github account, so the CLA Assistant is failing. Would you add your commit email to github, so the CLA is signed properly. |
…lehq#1198) Document the expected behaviour of flatten_events and flatten_links when called with empty slices, and the fall-back logic inside flatten_span_record that the PR parseablehq#1196 fix relies on: - test_flatten_events_empty_slice: flatten_events(&[]) returns [] - test_flatten_links_empty_slice: flatten_links(&[]) returns [] - test_flatten_span_record_events_only_no_links: span with events but no links produces one record per event, each enriched with span fields - test_flatten_span_record_links_only_no_events: span with links but no events produces one record per link, each enriched with span fields
ffc3e69 to
7a1b8fd
Compare
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/otel/traces.rs (2)
1059-1114: Well-structured test for events-only span flattening.The test correctly validates that a span with events but no links produces one record per event, each enriched with span-level fields.
Consider also asserting that link-specific fields are absent from event records for completeness (similar to how
test_flatten_span_record_without_events_and_linksverifies absence of both):💡 Optional enhancement
// Each event record must also carry the span-level fields. for record in &result { assert_eq!( record.get("span_name").unwrap(), &Value::String("events-only-span".to_string()), "Event records must be enriched with span fields" ); + assert!( + !record.contains_key("link_trace_id"), + "Event-only records should not contain link fields" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/otel/traces.rs` around lines 1059 - 1114, Update the test test_flatten_span_record_events_only_no_links to also assert that link-specific fields are absent on each event record: after the existing span-level checks, iterate the result and assert that keys such as "link_trace_id" and "link_span_id" (and any other link-related keys your flatten_span_record produces) are either None or not present (e.g., record.get("link_trace_id").is_none()). This keeps the test aligned with flatten_span_record's behavior and mirrors the completeness checks in test_flatten_span_record_without_events_and_links.
1116-1158: Good coverage for links-only span flattening.The test correctly validates that a span with links but no events produces one record per link, enriched with span-level fields.
For symmetry with the events-only test enhancement, consider also verifying the absence of event-specific fields:
💡 Optional enhancement
assert_eq!( result[0].get("span_name").unwrap(), &Value::String("links-only-span".to_string()), "Link records must be enriched with span fields" ); + assert!( + !result[0].contains_key("event_name"), + "Link-only records should not contain event fields" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/otel/traces.rs` around lines 1116 - 1158, Update the test_flatten_span_record_links_only_no_events test to also assert that event-specific fields are not present on link records: after calling flatten_span_record(&span) and the existing link assertions, add checks that result[0] does not contain keys such as "event_name", "event_time_unix_nano", "event_attributes" (or any other event-related keys used by flatten_span_record) to ensure link-only records aren't populated with event fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/otel/traces.rs`:
- Around line 1059-1114: Update the test
test_flatten_span_record_events_only_no_links to also assert that link-specific
fields are absent on each event record: after the existing span-level checks,
iterate the result and assert that keys such as "link_trace_id" and
"link_span_id" (and any other link-related keys your flatten_span_record
produces) are either None or not present (e.g.,
record.get("link_trace_id").is_none()). This keeps the test aligned with
flatten_span_record's behavior and mirrors the completeness checks in
test_flatten_span_record_without_events_and_links.
- Around line 1116-1158: Update the
test_flatten_span_record_links_only_no_events test to also assert that
event-specific fields are not present on link records: after calling
flatten_span_record(&span) and the existing link assertions, add checks that
result[0] does not contain keys such as "event_name", "event_time_unix_nano",
"event_attributes" (or any other event-related keys used by flatten_span_record)
to ensure link-only records aren't populated with event fields.
Summary
Closes #1198
What
Adds four targeted unit tests to
src/otel/traces.rsthat document the expected behaviour of the specialised flattening logic introduced in #1196 (emptyEvents/Linksin OTel spans):test_flatten_events_empty_sliceflatten_events(&[])returns an emptyVectest_flatten_links_empty_sliceflatten_links(&[])returns an emptyVectest_flatten_span_record_events_only_no_linkstest_flatten_span_record_links_only_no_eventsThe
if span_records_json.is_empty()branch inflatten_span_recordis the critical path for the #1196 fix; these tests pin its contract so that any future refactor that breaks the fallback will be caught immediately.Summary by CodeRabbit