feat: span actions #29
Conversation
WalkthroughThe Swoole DNS adapter constructor now accepts two additional integer parameters, Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (1)
src/DNS/Adapter/Swoole.php (1)
21-26: Consider adding parameter validation and documentation.The constructor now accepts
$numWorkersand$maxCoroutinesparameters, but there's no validation to ensure they're positive integers. While Swoole may validate these internally, adding bounds checking would provide clearer error messages and prevent potential runtime issues.Additionally, consider adding PHPDoc to document the purpose and acceptable ranges for these parameters.
🔎 Suggested enhancement with validation
+ /** + * @param string $host DNS server host address + * @param int $port DNS server port + * @param int $numWorkers Number of worker processes (must be >= 1) + * @param int $maxCoroutines Maximum number of coroutines per worker (must be >= 1) + */ public function __construct(string $host = '0.0.0.0', int $port = 53, int $numWorkers = 1, int $maxCoroutines = 3000) { + if ($numWorkers < 1) { + throw new \InvalidArgumentException('Number of workers must be at least 1'); + } + if ($maxCoroutines < 1) { + throw new \InvalidArgumentException('Maximum coroutines must be at least 1'); + } + $this->host = $host; $this->port = $port; $this->numWorkers = $numWorkers; $this->maxCoroutines = $maxCoroutines;
📜 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)
src/DNS/Adapter/Swoole.phpsrc/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 (3)
tests/resources/server.php (1)
63-63: LGTM! Enhanced telemetry with descriptive span name.The addition of
'dns.worker.start'as the span action name improves observability and aligns with the telemetry naming convention used elsewhere in this PR.src/DNS/Server.php (1)
167-167: LGTM! Enhanced telemetry with descriptive span name.The addition of
'dns.packet'as the span action name improves observability for DNS packet processing and maintains consistency with the span naming pattern introduced in this PR.src/DNS/Adapter/Swoole.php (1)
28-31: LGTM! Configuration applied correctly.The Swoole server configuration is properly initialized with the worker and coroutine settings. The timing is appropriate—configuration is applied in the constructor before the server starts.
loks0n
force-pushed
the
feat-span-actions
branch
from
12a2363 to
46c84a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/DNS/Adapter/Swoole.php (1)
25-26: Consider adding validation for worker configuration.While Swoole may handle invalid values internally, adding validation for
numWorkersandmaxCoroutineswould provide clearer error messages and prevent unexpected behavior from values like 0 or negative numbers.🔎 Optional validation example
$this->host = $host; $this->port = $port; + if ($numWorkers < 1) { + throw new \InvalidArgumentException('numWorkers must be at least 1'); + } + if ($maxCoroutines < 1) { + throw new \InvalidArgumentException('maxCoroutines must be at least 1'); + } $this->numWorkers = $numWorkers; $this->maxCoroutines = $maxCoroutines;
📜 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 (1)
src/DNS/Adapter/Swoole.php
🔇 Additional comments (2)
src/DNS/Adapter/Swoole.php (2)
18-19: LGTM!The property declarations are correctly typed and follow the existing code style.
28-31: Bothworker_numandmax_coroutineare valid standard configuration keys in Swoole 5.1.8. The configuration is correct and properly applied before server start.
| protected int $maxCoroutines; | ||
|
|
||
| public function __construct(string $host = '0.0.0.0', int $port = 53) | ||
| public function __construct(string $host = '0.0.0.0', int $port = 53, int $numWorkers = 1, int $maxCoroutines = 3000) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add PHPDoc for new constructor parameters.
The constructor signature now accepts two new parameters, but they lack documentation. Since this is a public API, adding PHPDoc comments would help users understand the purpose and valid ranges of numWorkers and maxCoroutines.
🔎 Suggested PHPDoc addition
+ /**
+ * @param string $host The host address to bind to
+ * @param int $port The port to listen on
+ * @param int $numWorkers The number of worker processes (default: 1)
+ * @param int $maxCoroutines The maximum number of coroutines (default: 3000)
+ */
public function __construct(string $host = '0.0.0.0', int $port = 53, int $numWorkers = 1, int $maxCoroutines = 3000)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct(string $host = '0.0.0.0', int $port = 53, int $numWorkers = 1, int $maxCoroutines = 3000) | |
| /** | |
| * @param string $host The host address to bind to | |
| * @param int $port The port to listen on | |
| * @param int $numWorkers The number of worker processes (default: 1) | |
| * @param int $maxCoroutines The maximum number of coroutines (default: 3000) | |
| */ | |
| public function __construct(string $host = '0.0.0.0', int $port = 53, int $numWorkers = 1, int $maxCoroutines = 3000) |
🤖 Prompt for AI Agents
In src/DNS/Adapter/Swoole.php around line 21, the constructor's new parameters
$numWorkers and $maxCoroutines lack PHPDoc; add a docblock above the constructor
documenting each parameter (string $host, int $port, int $numWorkers, int
$maxCoroutines) with a brief description and valid ranges/defaults (e.g.,
$numWorkers: number of Swoole worker processes, positive int, default 1;
$maxCoroutines: maximum coroutines per worker, positive int, typical limit and
default 3000), and include @param tags for each parameter and an @throws or @see
tag if relevant to runtime limits.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.