feat: add PSR-3 Trap logger with STDERR fallback#212
feat: add PSR-3 Trap logger with STDERR fallback#212petrdobr wants to merge 2 commits intobuggregator:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a standalone client-side PSR-3 logger implementation for Trap, exposed via TrapHandle::logger() (and therefore callable as trap()::logger()), with TCP delivery using a Monolog-style JSON payload and a STDERR fallback.
Changes:
- Introduced
Buggregator\Trap\Log\TrapLogger(PSR-3AbstractLogger) with sync + fiber-aware “async” transport and STDERR fallback formatting. - Added
TrapHandle::logger()singleton accessor configured byTRAP_MONOLOG_HOST/TRAP_MONOLOG_PORT. - Added unit tests for logger access, fallback formatting, and invalid level handling; added
psr/logdependency.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Log/TrapLogger.php |
New PSR-3 logger that attempts TCP delivery (sync/async) and falls back to STDERR text lines. |
src/Client/TrapHandle.php |
Adds logger() accessor and env-based configuration; refactors env getter to static. |
tests/Unit/Client/TrapLoggerTest.php |
Adds tests for logger singleton, fallback formatting, and invalid level behavior. |
composer.json |
Adds psr/log requirement for PSR-3 interfaces/base logger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $stream = @\stream_socket_client( | ||
| $address, | ||
| $errorCode, | ||
| $errorMessage, | ||
| $this->connectTimeout, | ||
| ); | ||
|
|
||
| if ($stream === false) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| $payload .= "\n"; | ||
| $offset = 0; | ||
| $length = \strlen($payload); | ||
|
|
||
| while ($offset < $length) { | ||
| $written = @\fwrite($stream, \substr($payload, $offset)); | ||
|
|
||
| if (!\is_int($written) || $written <= 0) { | ||
| return false; | ||
| } | ||
|
|
||
| $offset += $written; | ||
| } | ||
|
|
There was a problem hiding this comment.
sendSync() only applies a connect timeout, but the subsequent fwrite() calls can still block for a long time if the remote peer is slow or stops reading. Consider setting a stream write timeout (e.g., via stream_set_timeout) or using non-blocking writes with a bounded wait to prevent unexpected stalls in synchronous code paths.
tests/Unit/Client/TrapLoggerTest.php
Outdated
| public function testLoggerCanWriteInfo(): void | ||
| { | ||
| $logger = new TrapLogger(host: '127.0.0.1', port: 1); | ||
|
|
||
| $logger->info('Hello {name}', ['name' => 'Trap']); | ||
|
|
||
| self::assertTrue(true); | ||
| } |
There was a problem hiding this comment.
This test calls TrapLogger::info() with an unreachable TCP port (1), which will trigger the fallback path and write to STDERR. With PHPUnit configured as beStrictAboutOutputDuringTests="true", this risks making the suite fail or at least polluting test output. Consider using a local stream_socket_server() in the test to accept the connection (so no fallback occurs), or restructuring the test to avoid calling log() directly.
tests/Unit/Client/TrapLoggerTest.php
Outdated
| $logger = new TrapLogger(host: '127.0.0.1', port: 1); | ||
|
|
||
| $logger->info('Hello {name}', ['name' => 'Trap']); | ||
|
|
||
| self::assertTrue(true); | ||
| } | ||
|
|
||
| public function testLoggerCanWriteError(): void | ||
| { | ||
| $logger = new TrapLogger(host: '127.0.0.1', port: 1); | ||
|
|
||
| $logger->error('Something went wrong: {error}', ['error' => 'boom']); | ||
|
|
There was a problem hiding this comment.
Same issue as the info test: calling error() against port 1 will fall back to STDERR output, which is risky under beStrictAboutOutputDuringTests. Make this deterministic by spinning up a local TCP server socket for the duration of the test (and optionally asserting the JSON line was received), or otherwise avoid executing the fallback path in unit tests.
| $logger = new TrapLogger(host: '127.0.0.1', port: 1); | |
| $logger->info('Hello {name}', ['name' => 'Trap']); | |
| self::assertTrue(true); | |
| } | |
| public function testLoggerCanWriteError(): void | |
| { | |
| $logger = new TrapLogger(host: '127.0.0.1', port: 1); | |
| $logger->error('Something went wrong: {error}', ['error' => 'boom']); | |
| $server = \stream_socket_server('tcp://127.0.0.1:0', $errno, $errstr); | |
| self::assertNotFalse($server, 'Failed to create local TCP server: ' . $errstr); | |
| $address = \stream_socket_get_name($server, false); | |
| [$host, $port] = \explode(':', $address); | |
| $port = (int) $port; | |
| $logger = new TrapLogger(host: $host, port: $port); | |
| $logger->info('Hello {name}', ['name' => 'Trap']); | |
| $client = @\stream_socket_accept($server, 1); | |
| if ($client !== false) { | |
| // Read a single JSON log line to ensure the message was sent. | |
| \stream_get_line($client, 8192, "\n"); | |
| \fclose($client); | |
| } | |
| \fclose($server); | |
| self::assertTrue(true); | |
| } | |
| public function testLoggerCanWriteError(): void | |
| { | |
| $server = \stream_socket_server('tcp://127.0.0.1:0', $errno, $errstr); | |
| self::assertNotFalse($server, 'Failed to create local TCP server: ' . $errstr); | |
| $address = \stream_socket_get_name($server, false); | |
| [$host, $port] = \explode(':', $address); | |
| $port = (int) $port; | |
| $logger = new TrapLogger(host: $host, port: $port); | |
| $logger->error('Something went wrong: {error}', ['error' => 'boom']); | |
| $client = @\stream_socket_accept($server, 1); | |
| if ($client !== false) { | |
| // Read a single JSON log line to ensure the message was sent. | |
| \stream_get_line($client, 8192, "\n"); | |
| \fclose($client); | |
| } | |
| \fclose($server); |
| $host = self::getEnvValue('TRAP_MONOLOG_HOST', '127.0.0.1'); | ||
| $port = (int) self::getEnvValue('TRAP_MONOLOG_PORT', '9913'); | ||
|
|
||
| self::$logger = new TrapLogger( | ||
| host: $host, | ||
| port: $port, | ||
| ); |
There was a problem hiding this comment.
TRAP_MONOLOG_PORT is cast to int without validation. If the env var is non-numeric (or empty), this becomes 0 and the logger will attempt to connect to an invalid port instead of falling back to the default. Consider validating the value (numeric and within 1..65535) and using 9913 when invalid.
src/Log/TrapLogger.php
Outdated
| $write = [$stream]; | ||
| $read = []; | ||
| $except = []; | ||
|
|
||
| if (@\stream_select($read, $write, $except, 0, 0) !== 1) { |
There was a problem hiding this comment.
The async/fiber path is effectively "immediate write or fail": stream_select(..., 0, 0) !== 1 returns false unless the socket is writable right away. With STREAM_CLIENT_ASYNC_CONNECT, this will commonly return 0 while the connection is still in progress, causing almost every async log to fall back to STDERR. Consider allowing a small (still non-blocking / bounded) wait (e.g., a few milliseconds) or a limited retry loop before giving up.
| $write = [$stream]; | |
| $read = []; | |
| $except = []; | |
| if (@\stream_select($read, $write, $except, 0, 0) !== 1) { | |
| $isWritable = false; | |
| // Allow a small, bounded wait for async connect to complete. | |
| // This keeps the operation effectively non-blocking while avoiding | |
| // spurious failures when the socket is not immediately writable. | |
| for ($attempt = 0; $attempt < 3; $attempt++) { | |
| $write = [$stream]; | |
| $read = []; | |
| $except = []; | |
| $result = @\stream_select($read, $write, $except, 0, 5000); | |
| if ($result === 1) { | |
| $isWritable = true; | |
| break; | |
| } | |
| if ($result === false) { | |
| return false; | |
| } | |
| } | |
| if (! $isWritable) { |
What was changed
trap()::logger()TrapLoggerbased onpsr/logSTDERRoutput when Trap server delivery failsTRAP_MONOLOG_HOSTandTRAP_MONOLOG_PORTWhy?
The issue asks for a standalone PSR-3 logger implementation for the client side of the project, accessible via
trap()::logger().This makes Trap usable in places where a PSR-3 logger is expected without requiring Monolog as the application logger. The logger tries to send records to the Trap server first, and falls back to
STDERRin text format when delivery is not possible.The async/fiber path is implemented as best-effort non-blocking behavior to avoid interfering with fiber-based execution.
Checklist
Notes
The fiber/async path is implemented as best-effort non-blocking behavior.
I’m open to adjusting it if a different async strategy is preferred.