feat: spans #28
feat: spans #28
Conversation
WalkthroughThe pull request replaces console-based logging with distributed tracing instrumentation using Span across the DNS Server implementation. The composer.json dependency is updated from utopia-php/console to utopia-php/span. The Server.php class is refactored to instrument packet processing with span-based metrics tracking decode, resolve, and encode operation durations. Error handling is restructured to delegate to registered handlers with span error recording. The test server setup is updated to initialize span storage and exporters with coroutine-based context. No changes are made to public method signatures or class interfaces. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
composer.json (1)
23-28: VCS repository is temporary.The direct VCS repository reference suggests utopia-php/span is not yet published to Packagist. Ensure this is transitioned to a stable package release before merging to production.
src/DNS/Server.php (1)
239-249: Potential nested encoding failure.If the fallback
$response->encode()on Line 249 also throws an exception, it will propagate unhandled. While unlikely, consider whether a final catch or simpler error response (e.g., pre-encoded SERVFAIL bytes) would provide better resilience.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
composer.jsonsrc/DNS/Server.phptests/resources/server.php
⏰ 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). (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (7)
tests/resources/server.php (2)
63-66: LGTM!The worker startup span instrumentation is appropriate and follows a complete lifecycle (init → set attributes → finish).
10-19: No issues found—Span API usage is correct.All API calls in the code (
Span::setStorage(),Span::addExporter(),Span::init(),$span->set(), and$span->finish()) are documented and properly supported in the utopia-php/span library. TheStorage\CoroutineandExporter\Stdoutclasses are valid built-in implementations. The integration is sound.src/DNS/Server.php (5)
167-168: LGTM!Span initialization with client IP tracking is well-placed at the start of packet processing. This provides good observability for DNS queries.
170-171: LGTM!Pre-initializing
$questionand$responseto null ensures safe access in the finally block for telemetry recording.
196-211: LGTM!Question validation with proper error response (FORMERR) and span instrumentation for observability is well-implemented.
213-233: LGTM!Resolve phase error handling properly returns a SERVFAIL response while recording span errors and metrics. This ensures clients receive appropriate DNS error responses.
258-271: LGTM!The finally block ensures telemetry is always recorded and the span is properly finished, with appropriate null-safety checks for conditional metrics.
| "require": { | ||
| "php": ">=8.3", | ||
| "utopia-php/console": "0.0.*", | ||
| "utopia-php/span": "dev-main", |
There was a problem hiding this comment.
Version constraint conflicts with minimum stability.
Using "dev-main" for utopia-php/span conflicts with "minimum-stability": "stable" (Line 7). This may cause Composer to fail or require manual intervention during installation.
Recommended fixes:
- Use a tagged version with semantic versioning (e.g.,
"^1.0") once available, OR - Add
@devflag:"utopia-php/span": "dev-main@dev", OR - Change minimum-stability to
"dev"with"prefer-stable": true
🔎 Proposed fix using @dev flag
- "utopia-php/span": "dev-main",
+ "utopia-php/span": "dev-main@dev",🤖 Prompt for AI Agents
In composer.json around line 31, the dependency "utopia-php/span": "dev-main"
conflicts with "minimum-stability": "stable" (line 7); update the constraint to
a stable tag if available (e.g., "^1.0"), or append the @dev stability flag
("dev-main@dev") to allow this dev branch, or change composer settings to
"minimum-stability": "dev" and add "prefer-stable": true; pick one of these
fixes and apply it consistently to resolve installation failures.
| } catch (Throwable $e) { | ||
| $span->setError($e); | ||
| $this->handleError($e); | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full method context around lines 187-191 in Server.php
cat -n src/DNS/Server.php | sed -n '165,200p'Repository: utopia-php/dns
Length of output: 1580
🏁 Script executed:
#!/bin/bash
# Search for resolve/encode error handling to understand the pattern
rg -n "SERVFAIL|setError" src/DNS/Server.php | head -20Repository: utopia-php/dns
Length of output: 278
🏁 Script executed:
#!/bin/bash
# Check how resolve errors are handled
rg -nB5 -A5 "resolve.*catch" src/DNS/Server.php | head -40Repository: utopia-php/dns
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Get context for lines 210-250 to see resolve/encode error handling
cat -n src/DNS/Server.php | sed -n '210,250p'Repository: utopia-php/dns
Length of output: 1806
🏁 Script executed:
#!/bin/bash
# Also check what happens after line 190 - is there a caller that handles empty string?
cat -n src/DNS/Server.php | sed -n '145,165p'Repository: utopia-php/dns
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Check how onPacket is used - see who calls it and what they do with the return value
rg -n "onPacket" src/DNS/Server.phpRepository: utopia-php/dns
Length of output: 344
🏁 Script executed:
#!/bin/bash
# See if there's special handling for empty string returns anywhere in the flow
rg -n "onPacket\|return ''" src/DNS/Server.php -B2 -A2Repository: utopia-php/dns
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the Message::response method to see if it can construct a response without header
fd "Message.php" -t f | xargs cat -n | grep -A20 "function response\|public static function response"Repository: utopia-php/dns
Length of output: 941
🏁 Script executed:
#!/bin/bash
# Check the PartialDecodingException to see if it extends or relates to other exceptions
fd "PartialDecodingException.php" -t f | head -1 | xargs cat -nRepository: utopia-php/dns
Length of output: 819
🏁 Script executed:
#!/bin/bash
# Look for other exceptions that might be thrown by Message::decode
rg -n "class.*Exception" src/ --type=php | head -20Repository: utopia-php/dns
Length of output: 377
🏁 Script executed:
#!/bin/bash
# Check Message::decode to understand what exceptions it can throw
fd "Message.php" -t f | head -1 | xargs cat -n | grep -A50 "public static function decode"Repository: utopia-php/dns
Length of output: 2157
🏁 Script executed:
#!/bin/bash
# Check the DecodingException class
fd "DecodingException.php" -t f --path "*/Exception/Message/*" | xargs cat -nRepository: utopia-php/dns
Length of output: 291
🏁 Script executed:
#!/bin/bash
# See how onPacket return value is used in the adapter
fd "Server.php" -t f | xargs cat -n | sed -n '270,285p'Repository: utopia-php/dns
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Check the adapter to see what happens with empty string returns
rg -n "onPacket|adapter" src/DNS/Server.php | tail -20Repository: utopia-php/dns
Length of output: 647
🏁 Script executed:
#!/bin/bash
# Find the Adapter interface/class to understand what it expects
fd "Adapter.php" -t f | head -5 | xargs cat -n | head -80Repository: utopia-php/dns
Length of output: 1077
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for this behavior
fd ".*Test.php|*test.php" -t f | xargs rg -l "onPacket\|decode.*error" | head -5Repository: utopia-php/dns
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Look for usage of DecodingException vs PartialDecodingException
rg -n "DecodingException" src/ --type=php -C2Repository: utopia-php/dns
Length of output: 16279
Returning an empty string for fatal decode errors is inconsistent with error handling elsewhere and should be reconsidered.
When decoding fails before the message header can be extracted (e.g., packet too short), the code returns an empty string, meaning no DNS response is sent to the client. This differs from resolve/encode errors, which return a SERVFAIL response. While a SERVFAIL response cannot be constructed without a header, returning an empty string leaves the client without any response, potentially causing timeouts.
Consider whether a minimal response (even just a header) or an alternative error handling strategy would be more appropriate for fatal decode errors.
| * @param Throwable $error | ||
| * @return void | ||
| */ | ||
| protected function handleError(Throwable $error): void |
There was a problem hiding this comment.
do we still need this method?
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.