Add query logging #3934
Add query logging #3934
Conversation
📝 WalkthroughWalkthroughAdds SQL timeout monitoring: new LogQueryTimeout listener, pre/post query logging and a shutdown timeout handler in AppServiceProvider, conditional listener registration in EventServiceProvider, supporting docs, and unit tests for the listener. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
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: 5
🧹 Nitpick comments (1)
app/Listeners/LogQueryTimeout.php (1)
45-68: Optional: Extract duplicated log context.The log context arrays on lines 47-56 and 58-67 are nearly identical. Consider extracting to reduce duplication.
🔎 Proposed refactor
// Log if query is dangerously slow + $context = [ + 'execution_time_ms' => $event->time, + 'execution_time_s' => round($event->time / 1000, 2), + 'timeout_limit_s' => $max_execution_time / 1000, + 'percentage' => round(($event->time / $max_execution_time) * 100, 1) . '%', + 'sql' => $event->sql, + 'bindings' => $event->bindings, + 'connection' => $event->connectionName, + 'url' => request()?->fullUrl() ?? 'N/A', + ]; + if ($event->time >= $critical_threshold) { - Log::error('🚨 CRITICAL: Query approaching timeout', [ - 'execution_time_ms' => $event->time, - 'execution_time_s' => round($event->time / 1000, 2), - 'timeout_limit_s' => $max_execution_time / 1000, - 'percentage' => round(($event->time / $max_execution_time) * 100, 1) . '%', - 'sql' => $event->sql, - 'bindings' => $event->bindings, - 'connection' => $event->connectionName, - 'url' => request()?->fullUrl() ?? 'N/A', - ]); + Log::error('🚨 CRITICAL: Query approaching timeout', $context); } elseif ($event->time >= $warning_threshold) { - Log::warning('⚠️ WARNING: Slow query detected', [ - 'execution_time_ms' => $event->time, - 'execution_time_s' => round($event->time / 1000, 2), - 'timeout_limit_s' => $max_execution_time / 1000, - 'percentage' => round(($event->time / $max_execution_time) * 100, 1) . '%', - 'sql' => $event->sql, - 'bindings' => $event->bindings, - 'connection' => $event->connectionName, - 'url' => request()?->fullUrl() ?? 'N/A', - ]); + Log::warning('⚠️ WARNING: Slow query detected', $context); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/Listeners/LogQueryTimeout.phpapp/Providers/AppServiceProvider.phpapp/Providers/EventServiceProvider.phpdocs/specs/2-how-to/sql-timeout-logging.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Listeners/LogQueryTimeout.phpapp/Providers/EventServiceProvider.phpapp/Providers/AppServiceProvider.php
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: Use Markdown format for documentation
At the bottom of documentation files, add an hr line followed by "Last updated: [date of the update]"
Files:
docs/specs/2-how-to/sql-timeout-logging.md
🧠 Learnings (2)
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Listeners/LogQueryTimeout.phpapp/Providers/EventServiceProvider.phpapp/Providers/AppServiceProvider.php
📚 Learning: 2026-01-03T12:36:02.514Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3922
File: app/Listeners/RecomputeAlbumSizeOnPhotoMutation.php:63-69
Timestamp: 2026-01-03T12:36:02.514Z
Learning: In Lychee, treat a PhotoDeleted event with a specific album_id as removing the photo from that album only (not deleting the photo globally). For any listener handling photo mutations, when such an event occurs, recompute the size/statistics for only the affected album, because the photo may remain in other albums due to the many-to-many relationship. Ensure no global deletion logic is triggered and that only the targeted album’s statistics are refreshed.
Applied to files:
app/Listeners/LogQueryTimeout.php
🧬 Code graph analysis (1)
app/Providers/EventServiceProvider.php (1)
app/Listeners/LogQueryTimeout.php (1)
LogQueryTimeout(21-70)
🪛 LanguageTool
docs/specs/2-how-to/sql-timeout-logging.md
[uncategorized] ~11-~11: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...* - Queries using >70% of max execution time 3. Critical queries - Queries using >90% of max e...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.4 - mariadb -- Install
🔇 Additional comments (4)
app/Providers/EventServiceProvider.php (1)
96-99: LGTM!The conditional listener registration follows the existing patterns in the file, uses strict comparison as per coding guidelines, and correctly wires up the
LogQueryTimeoutlistener when SQL logging is enabled.app/Providers/AppServiceProvider.php (2)
205-219: LGTM!The shutdown handler correctly uses
in_array()with strict comparison (third parametertrue) per coding guidelines, and properly handles potential null request context with null-safe operators.
270-285: Debug backtrace usage is reasonable.The implementation correctly uses
DEBUG_BACKTRACE_IGNORE_ARGSto avoid capturing argument values and limits the trace depth to 15 frames, minimizing performance overhead. The origin detection logic appropriately filters out framework internals.app/Listeners/LogQueryTimeout.php (1)
1-14: LGTM!The file correctly includes the license header with a blank line after the opening PHP tag, follows PSR-4 namespace conventions, and imports are properly organized. As per coding guidelines.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Providers/AppServiceProvider.php (1)
306-314: Inconsistent null handling betweenlogSQLStartandlogSQL.Line 262 in
logSQLStartcorrectly usesrequest()?->getRequestUri() ?? '', but line 310 inlogSQLstill usesrequest()->getRequestUri()without null-safe handling. This could cause issues when called from CLI context (e.g., migrations, artisan commands).🔎 Proposed fix
private function logSQL(QueryExecuted $query): void { + $uri = request()?->getRequestUri() ?? ''; // Quick exit if ( - Str::contains(request()->getRequestUri(), 'logs', true) || + Str::contains($uri, 'logs', true) || Str::contains($query->sql, $this->ignore_log_SQL) ) { return; }
♻️ Duplicate comments (1)
docs/specs/2-how-to/sql-timeout-logging.md (1)
139-140: Missing required documentation footer.Per coding guidelines, documentation files should include an hr line followed by the last updated date at the bottom.
🔎 Proposed fix
- Traditional PHP-FPM may not trigger all timeout handlers consistently + +--- + +*Last updated: 2026-01-04*
🧹 Nitpick comments (1)
app/Listeners/LogQueryTimeout.php (1)
14-19: Stale docstring: remove cache reference.The docstring mentions "storing query start information in cache" but the implementation no longer uses cache (per the addressed past review). Update the docstring to accurately describe current behavior.
🔎 Proposed fix
/** * Tracks long-running SQL queries that may timeout. - * - * This listener works by storing query start information in cache - * and checking execution time after completion. */ class LogQueryTimeout
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Listeners/LogQueryTimeout.phpapp/Providers/AppServiceProvider.phpapp/Providers/EventServiceProvider.phpdocs/specs/2-how-to/sql-timeout-logging.mdtests/Unit/Listeners/LogQueryTimeoutTest.php
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Any new PHP file should contain the license header and have a single blank line after the opening PHP tag
Variable names should be in snake_case in PHP
Apply the PSR-4 coding standard in PHP
Use in_array() with true as the third parameter in PHP
Only use booleans in if statements, not integers or strings
Use strict comparison (===) instead of loose comparison (==)
Avoid code duplication in both if and else statements
Do not use empty() in PHP
Use the moneyphp/money library for handling monetary values in PHP
Never use floats or doubles to represent monetary values; use integers representing the smallest currency unit (e.g., cents for USD)
**/*.php: Write or extend executable specifications (unit, behaviour, or scenario tests) ahead of implementation, confirm they fail, and then drive code to green before refactoring. List the expected success, validation, and failure branches and add thin failing tests for each path.
For PHP code, adhere to conventions: license headers in new files, strict comparison (===), no empty(), in_array() with third parameter true, snake_case variables, PSR-4 standard, test base classes (AbstractTestCase for Unit, BaseApiWithDataTest for Feature_v2).
Always run phpunit tests. If a test remains red, disable it with a TODO, note the reason, and capture the follow-up in the relevant plan.
Spotless now uses Palantir Java Format 2.78.0 with a 120-character wrap; configure IDE formatters to match before pushing code changes.
Keep each increment's control flow flat by delegating validation/normalisation into tiny pure helpers that return simple enums or result records, then compose them instead of introducing inline branching that inflates the branch count per change.
When introducing new helpers/utilities or editing files prone to style violations (records, DTOs, generated adapters), run the narrowest applicable lint target (for example phpstan) before the full pipeline. Note the command in the related plan/task.
For PHP changes, ru...
Files:
app/Providers/EventServiceProvider.phptests/Unit/Listeners/LogQueryTimeoutTest.phpapp/Listeners/LogQueryTimeout.phpapp/Providers/AppServiceProvider.php
tests/Unit/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Tests in the tests/Unit directory should extend from AbstractTestCase
Files:
tests/Unit/Listeners/LogQueryTimeoutTest.php
tests/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
No need to mock the database in tests; use the in-memory SQLite database instead
Files:
tests/Unit/Listeners/LogQueryTimeoutTest.php
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: Use Markdown format for documentation
At the bottom of documentation files, add an hr line followed by "Last updated: [date of the update]"
Files:
docs/specs/2-how-to/sql-timeout-logging.md
🧠 Learnings (2)
📚 Learning: 2025-12-28T18:12:55.752Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3901
File: app/Providers/AppServiceProvider.php:0-0
Timestamp: 2025-12-28T18:12:55.752Z
Learning: When using Laravel Octane's tick API, Octane::tick(...) returns an InvokeTickCallable that only has ->seconds(int) and ->immediate() methods. There is no ->every(N) method. Use the correct usage: Octane::tick('name', fn() => ...)->seconds(N) or Octane::tick('name', fn() => ..., N). Apply this guideline to PHP files across the project (not just AppServiceProvider.php).
Applied to files:
app/Providers/EventServiceProvider.phptests/Unit/Listeners/LogQueryTimeoutTest.phpapp/Listeners/LogQueryTimeout.phpapp/Providers/AppServiceProvider.php
📚 Learning: 2026-01-03T12:36:02.514Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3922
File: app/Listeners/RecomputeAlbumSizeOnPhotoMutation.php:63-69
Timestamp: 2026-01-03T12:36:02.514Z
Learning: In Lychee, treat a PhotoDeleted event with a specific album_id as removing the photo from that album only (not deleting the photo globally). For any listener handling photo mutations, when such an event occurs, recompute the size/statistics for only the affected album, because the photo may remain in other albums due to the many-to-many relationship. Ensure no global deletion logic is triggered and that only the targeted album’s statistics are refreshed.
Applied to files:
app/Listeners/LogQueryTimeout.php
🧬 Code graph analysis (2)
app/Providers/EventServiceProvider.php (1)
app/Listeners/LogQueryTimeout.php (1)
LogQueryTimeout(20-60)
tests/Unit/Listeners/LogQueryTimeoutTest.php (1)
app/Listeners/LogQueryTimeout.php (2)
LogQueryTimeout(20-60)handle(29-59)
🪛 LanguageTool
docs/specs/2-how-to/sql-timeout-logging.md
[uncategorized] ~11-~11: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...* - Queries using >70% of max execution time 3. Critical queries - Queries using >90% of max e...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: 2️⃣ PHP dist / 8.4 - mariadb
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.4 - postgresql
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
🔇 Additional comments (6)
app/Listeners/LogQueryTimeout.php (1)
29-59: LGTM!The threshold logic is clear and the logging context is comprehensive. Good use of null-safe operators for request context.
app/Providers/EventServiceProvider.php (1)
96-101: LGTM!The conditional registration follows the existing event listener pattern in this file. Good use of strict comparison (
=== true) per coding guidelines.app/Providers/AppServiceProvider.php (2)
197-220: LGTM!Good implementation of the multi-faceted logging approach:
- Pre-execution hook for query start tracking
- Post-execution hook for completed queries
- Shutdown handler for PHP timeout detection
Correct use of
in_array()withtrueas third parameter per coding guidelines.
260-301: LGTM!The
logSQLStartmethod correctly addresses the previously flagged null reference issue with the null-safe operator at line 262. The backtrace inspection logic for finding the originating app file is reasonable.tests/Unit/Listeners/LogQueryTimeoutTest.php (1)
27-82: LGTM!Tests correctly extend
AbstractTestCaseas required for Unit tests. Good coverage of the three main threshold scenarios using the in-memory SQLite database per coding guidelines.docs/specs/2-how-to/sql-timeout-logging.md (1)
1-14: Well-structured documentation.The overview clearly explains the logging system's capabilities. The four-tier categorization (slow queries, approaching timeout, critical, PHP timeouts) matches the implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/specs/2-how-to/sql-timeout-logging.md (1)
139-140: Add required documentation footer.Per coding guidelines, documentation files must include an hr line followed by the last updated date at the bottom.
🔎 Proposed fix
- Timeout detection works best with FrankenPHP, Swoole, or RoadRunner - Traditional PHP-FPM may not trigger all timeout handlers consistently + +--- + +*Last updated: 2026-01-04*
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/specs/2-how-to/sql-timeout-logging.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.md: Use Markdown format for documentation
At the bottom of documentation files, add an hr line followed by "Last updated: [date of the update]"
Files:
docs/specs/2-how-to/sql-timeout-logging.md
🪛 LanguageTool
docs/specs/2-how-to/sql-timeout-logging.md
[uncategorized] ~11-~11: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...* - Queries using >70% of max execution time 3. Critical queries - Queries using >90% of max e...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.