Add execution delay headers to async, event-triggered and sync executions #10191
Conversation
📝 WalkthroughWalkthroughThis change set introduces the tracking and propagation of function execution timing headers throughout the Appwrite platform. A new header, Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (3)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/controllers/shared/api.php (1)
716-721: Consider a more descriptive variable name.The implementation correctly adds the scheduling timestamp header. However, the variable name
$scheduledAtmight be misleading since this represents when the function is queued for execution rather than a user-scheduled execution time.Consider renaming to
$queuedAtor$triggeredAtfor clarity:- $scheduledAt = new \DateTime(); + $queuedAt = new \DateTime(); $queueForFunctions - ->setHeaders(['x-appwrite-scheduled-at' => $scheduledAt->format('Y-m-d\TH:i:s.v\Z')]) + ->setHeaders(['x-appwrite-scheduled-at' => $queuedAt->format('Y-m-d\TH:i:s.v\Z')])The ISO 8601 format with milliseconds and UTC timezone is appropriate for precise timing tracking.
app/controllers/general.php (1)
60-60: Consider implementing the header logic or removing the placeholder comment.This TODO comment suggests headers should be added here, but no implementation is provided. Either implement the necessary header logic or remove the placeholder comment to avoid leaving incomplete TODOs in the codebase.
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (1)
2314-2322: Consider using assertEventually for more robust async execution pollingWhile the fixed 5-second sleep works, consider using
$this->assertEventually()pattern used elsewhere in this file for more reliable async execution polling. However, if the sleep is intentional to ensure meaningful delay values for timing validation, the current approach is acceptable.- sleep(5); - - $execution = $this->getExecution($functionId, $executionId); - $this->assertEquals(200, $execution['headers']['status-code']); - $this->assertEquals('completed', $execution['body']['status']); - $this->assertEquals(200, $execution['body']['responseStatusCode']); - $this->assertGreaterThan(0, $execution['body']['duration']); - $this->assertNotEmpty($execution['body']['logs']); - $this->assertNotEmpty($execution['body']['responseHeaders']); + $this->assertEventually(function () use ($functionId, $executionId) { + $execution = $this->getExecution($functionId, $executionId); + $this->assertEquals(200, $execution['headers']['status-code']); + $this->assertEquals('completed', $execution['body']['status']); + $this->assertEquals(200, $execution['body']['responseStatusCode']); + $this->assertGreaterThan(0, $execution['body']['duration']); + $this->assertNotEmpty($execution['body']['logs']); + $this->assertNotEmpty($execution['body']['responseHeaders']); + return $execution; + }, 10000, 500);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/general.php(2 hunks)app/controllers/shared/api.php(1 hunks)app/init/constants.php(1 hunks)src/Appwrite/Event/Func.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Workers/Functions.php(2 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(1 hunks)tests/resources/functions/basic/index.js(1 hunks)
🧬 Code Graph Analysis (1)
app/controllers/shared/api.php (1)
src/Appwrite/Event/Func.php (1)
setHeaders(167-172)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/controllers/shared/api.php (1)
src/Appwrite/Event/Func.php (1)
setHeaders(167-172)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (10)
src/Appwrite/Event/Func.php (1)
164-164: LGTM! Documentation correction.The PHPDoc parameter type has been correctly updated to match the actual method signature and usage patterns. The method accepts an array of headers, not a string.
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
224-225: LGTM! Consistent timestamp header implementation.The addition of the
x-appwrite-scheduled-atheader is correctly implemented with consistent formatting (Y-m-d\TH:i:s.v\Z) that matches the usage in other parts of the codebase. The variable name$currentTimeis more descriptive than$scheduledAtused elsewhere.This header will enable accurate execution delay tracking when combined with the execution timestamp recorded during function processing.
app/init/constants.php (1)
145-145: LGTM! Essential allowlist update.Adding
x-appwrite-scheduled-atto the function request headers allowlist is necessary for the execution delay tracking feature to work properly. Without this addition, the header would be filtered out during function execution preparation.The header name follows the established
x-appwrite-convention used throughout the codebase.tests/resources/functions/basic/index.js (1)
11-27: LGTM! Consistent header validation logging implementation.The new timing header validations follow the same pattern as the existing JWT validation, which maintains consistency. The three headers being validated (
x-appwrite-execution-delay,x-appwrite-scheduled-at,x-appwrite-executed-at) align perfectly with the PR objective of tracking execution delay for async executions.src/Appwrite/Platform/Workers/Functions.php (1)
455-460: LGTM! Proper header merging for existing executions.The logic correctly merges the new timing headers into existing execution headers when the status is not "processing". This ensures that timing information is preserved when updating existing execution documents.
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (5)
2288-2295: LGTM - Function setup follows established patternsThe function setup is correctly configured with appropriate parameters for testing execution headers functionality.
2297-2304: LGTM - Deployment setup is correctThe deployment setup properly creates and activates the function deployment with appropriate validation.
2305-2312: LGTM - Async execution creation is properly handledThe async execution is created correctly with proper status code validation and execution ID extraction.
2323-2331: LGTM - Header validation logic is comprehensive and correctThe header validation properly:
- Extracts headers into a readable format using
array_column- Validates presence of all required timing headers
- Ensures the execution delay is numeric
- Correctly validates timestamp ordering (executed-at > scheduled-at)
- Verifies function-level validation through log messages
2333-2334: LGTM - Proper test cleanupThe cleanup follows the established pattern used throughout the test file.
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
x-appwrite-scheduled-at,x-appwrite-executed-atandx-appwrite-execution-delayheaders to sync, async and event-triggered executions.Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist