fix: handle missing span_name in logging test assertions #2665
Conversation
- Replace unsafe unwrap() calls with defensive pattern matching Fixes ai-dynamo#2586
|
👋 Hi WaelBKZ! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
the
external-contribution
WalkthroughReplaces unwrap-based assertions on span_name in logging test blocks (parent/child/grandchild) with nested Option checks using if-let to avoid panics when fields are absent or non-string. Assertions on parent_id relationships remain unchanged. No public API or production code modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(no out-of-scope changes identified) Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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 (3)
lib/runtime/src/logging.rs (3)
1012-1017: Simplify nested option checks with a single guardCurrent logic is correct and defensive. You can tighten it up by collapsing the nested if-lets into a single guard; this keeps the intent clear and avoids an extra level of indentation.
- if let Some(span_name) = log_line.get("span_name") { - if let Some(span_name_str) = span_name.as_str() { - if span_name_str == "parent" { - assert!(log_line.get("parent_id").is_none()); - } - } - } + if matches!(log_line.get("span_name").and_then(|v| v.as_str()), Some("parent")) { + assert!(log_line.get("parent_id").is_none()); + }
1023-1031: Remove unwraps on parent_id; assert on Option<&str> to stay panic-freeNice defensive guard for span_name. For consistency with the PR goal (no panics when fields are missing), avoid the remaining unwraps on parent_id. As parent_id is optional in the schema, assert on the Option directly. Behavior is unchanged, but this eliminates a potential panic surface if formatting ever omits/changes the type.
- if let Some(span_name) = log_line.get("span_name") { - if let Some(span_name_str) = span_name.as_str() { - if span_name_str == "child" { - assert_eq!( - log_line.get("parent_id").unwrap().as_str().unwrap(), - &parent_span_id - ); - } - } - } + if matches!(log_line.get("span_name").and_then(|v| v.as_str()), Some("child")) { + assert_eq!( + log_line.get("parent_id").and_then(|v| v.as_str()), + Some(parent_span_id.as_str()) + ); + }
1037-1045: Mirror the child fix for grandchild to avoid unwraps and keep tests robustSame rationale as above: compare Option<&str> instead of unwrapping parent_id. This keeps the test resilient if future changes alter the presence/type of parent_id while preserving the assertion semantics.
- if let Some(span_name) = log_line.get("span_name") { - if let Some(span_name_str) = span_name.as_str() { - if span_name_str == "grandchild" { - assert_eq!( - log_line.get("parent_id").unwrap().as_str().unwrap(), - &child_span_id - ); - } - } - } + if matches!(log_line.get("span_name").and_then(|v| v.as_str()), Some("grandchild")) { + assert_eq!( + log_line.get("parent_id").and_then(|v| v.as_str()), + Some(child_span_id.as_str()) + ); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/runtime/src/logging.rs(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). (4)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: Build and Test - dynamo
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Signed-off-by: Jason Zhou <jasonzho@jasonzho-mlt.client.nvidia.com>
Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
2 participants
Fixes #2586
Overview:
Fixes intermittent
logging::tests::test_json_log_capturefailures when run with other tests. The test was panicking on.unwrap()calls when accessingspan_namefields that don't exist in ERROR logs from background tasks.Details:
Root cause:
logging::tests::test_json_log_capturetests assumed all log lines havespan_namefield, but PR #2406 introduced error logs usingtracing::error!when tasks fail. These error logs contaminate the JSON log output captured by the test but don't containspan_namefields since they're regular log messages, not span events.Why intermittent:
Solution: Replace unsafe
.unwrap()calls with defensiveif letpattern matching that gracefully skips log lines withoutspan_namefields.Testing results: After the fix, ran the test 100 times and it passed 100 times (see test.log)
Where should the reviewer start?
lib/runtime/src/logging.rslines 1011-1047: The three test assertion blocks that were updated with defensive pattern matchingRelated Issues:
Summary by CodeRabbit