Fix: usage stats changes #9597
Fix: usage stats changes #9597
Conversation
WalkthroughThis pull request refactors the metrics system across the application. It replaces static, function-based metric identifiers with dynamic, resource-type-based ones by updating constants, controllers, and worker methods. Additionally, the stat-collection interval in the environment configuration was reduced from 3600 to 30 seconds. The modifications also include changes to error logging signatures and the introduction of separate counting methods for functions and sites. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIController
participant Constants
participant Worker
participant MetricsDB
Client->>APIController: Send function/site request
APIController->>Constants: Retrieve metric constants
APIController->>Worker: Dispatch metric event with resourceType & resourceInternalId
Worker->>MetricsDB: Log/update metric event
MetricsDB-->>Worker: Acknowledge metric recording
Worker-->>APIController: Confirm metric update
sequenceDiagram
participant StatsWorker
participant StatsResources
participant DB
StatsWorker->>StatsResources: countForSitesAndFunctions(db, region)
StatsResources->>DB: Query deployments and builds
DB-->>StatsResources: Return aggregated data
StatsResources-->>StatsWorker: Provide updated stats
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php (1)
140-141:⚠️ Potential issueFix index in buildsMbSecondsTotal access.
There's an index mismatch when accessing the buildsMbSecondsTotal metric. Line 140 incorrectly uses index 7, but the metric is at index 5 in the metrics array defined on lines 76-81.
- 'buildsMbSecondsTotal' => $usage[$metrics[7]]['total'], - 'buildsMbSeconds' => $usage[$metrics[7]]['data'] + 'buildsMbSecondsTotal' => $usage[$metrics[5]]['total'], + 'buildsMbSeconds' => $usage[$metrics[5]]['data']src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php (1)
129-130:⚠️ Potential issueArray index out of bounds error.
There's a critical issue in the response - the code attempts to access
$metrics[8]forbuildsMbSecondsTotalandbuildsMbSeconds, but the metrics array only has elements at indices 0-6 (line 62-70).- 'buildsMbSecondsTotal' => $usage[$metrics[8]]['total'], - 'buildsMbSeconds' => $usage[$metrics[8]]['data'] + 'buildsMbSecondsTotal' => $usage[$metrics[6]]['total'], + 'buildsMbSeconds' => $usage[$metrics[6]]['data']Alternatively, add the missing metric at index 8 if that's the intended usage.
🧹 Nitpick comments (2)
app/controllers/api/project.php (1)
254-268: Duplicate code blocks could be refactoredThere appears to be duplicate code between these blocks and the earlier similar blocks at lines 165-179 and 181-195. The functions have identical implementations with the same metric construction.
Consider extracting these repeated array_map operations into a reusable function to reduce duplication:
- $executionsMbSecondsBreakdown = array_map(function ($function) use ($dbForProject) { - $id = $function->getId(); - $name = $function->getAttribute('name'); - $metric = str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $function->getInternalId()], METRIC_RESOURCE_TYPE_ID_EXECUTIONS_MB_SECONDS); - $value = $dbForProject->findOne('stats', [ - Query::equal('metric', [$metric]), - Query::equal('period', ['inf']) - ]); - - return [ - 'resourceId' => $id, - 'name' => $name, - 'value' => $value['value'] ?? 0, - ]; - }, $dbForProject->find('functions')); - - $buildsMbSecondsBreakdown = array_map(function ($function) use ($dbForProject) { - $id = $function->getId(); - $name = $function->getAttribute('name'); - $metric = str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $function->getInternalId()], METRIC_RESOURCE_TYPE_ID_BUILDS_MB_SECONDS); - $value = $dbForProject->findOne('stats', [ - Query::equal('metric', [$metric]), - Query::equal('period', ['inf']) - ]); - - return [ - 'resourceId' => $id, - 'name' => $name, - 'value' => $value['value'] ?? 0, - ]; - }, $dbForProject->find('functions')); + function createBreakdown($functions, $metricTemplate, $dbForProject) { + return array_map(function ($function) use ($dbForProject, $metricTemplate) { + $id = $function->getId(); + $name = $function->getAttribute('name'); + $metric = str_replace(['{resourceType}', '{resourceInternalId}'], [RESOURCE_TYPE_FUNCTIONS, $function->getInternalId()], $metricTemplate); + $value = $dbForProject->findOne('stats', [ + Query::equal('metric', [$metric]), + Query::equal('period', ['inf']) + ]); + + return [ + 'resourceId' => $id, + 'name' => $name, + 'value' => $value['value'] ?? 0, + ]; + }, $functions); + } + + $functions = $dbForProject->find('functions'); + $executionsMbSecondsBreakdown = createBreakdown($functions, METRIC_RESOURCE_TYPE_ID_EXECUTIONS_MB_SECONDS, $dbForProject); + $buildsMbSecondsBreakdown = createBreakdown($functions, METRIC_RESOURCE_TYPE_ID_BUILDS_MB_SECONDS, $dbForProject);Also applies to: 270-284
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1192-1229: Consider abstracting repetitive metric construction patternsThe
sendUsagemethod contains several repetitive patterns for metric construction. Consider abstracting these patterns into helper methods to improve code readability and maintenance.protected function sendUsage(Document $resource, Document $deployment, Document $project, StatsUsage $queue): void { + // Helper method to add resource-type metrics + $addResourceMetrics = function(StatsUsage $queue, string $baseMetric, string $resourceTypeMetric, + string $resourceTypeIdMetric, string $resourceType, string $resourceInternalId, int $value) { + return $queue + ->addMetric($baseMetric, $value) + ->addMetric(str_replace(['{resourceType}'], [$resourceType], $resourceTypeMetric), $value) + ->addMetric(str_replace(['{resourceType}', '{resourceInternalId}'], [$resourceType, $resourceInternalId], $resourceTypeIdMetric), $value); + }; switch ($deployment->getAttribute('status')) { case 'ready': - $queue - ->addMetric(METRIC_BUILDS_SUCCESS, 1) // per project - ->addMetric(METRIC_BUILDS_COMPUTE_SUCCESS, (int)$deployment->getAttribute('buildDuration', 0) * 1000) - ->addMetric(str_replace(['{resourceType}'], [$deployment->getAttribute('resourceType')], METRIC_RESOURCE_TYPE_BUILDS_SUCCESS), 1) // per function - ->addMetric(str_replace(['{resourceType}'], [$deployment->getAttribute('resourceType')], METRIC_RESOURCE_TYPE_BUILDS_COMPUTE_SUCCESS), (int)$deployment->getAttribute('buildDuration', 0) * 1000) - ->addMetric(str_replace(['{resourceType}', '{resourceInternalId}'], [$deployment->getAttribute('resourceType'), $resource->getInternalId()], METRIC_RESOURCE_TYPE_ID_BUILDS_SUCCESS), 1) // per function - ->addMetric(str_replace(['{resourceType}', '{resourceInternalId}'], [$deployment->getAttribute('resourceType'), $resource->getInternalId()], METRIC_RESOURCE_TYPE_ID_BUILDS_COMPUTE_SUCCESS), (int)$deployment->getAttribute('buildDuration', 0) * 1000); + $addResourceMetrics($queue, METRIC_BUILDS_SUCCESS, METRIC_RESOURCE_TYPE_BUILDS_SUCCESS, + METRIC_RESOURCE_TYPE_ID_BUILDS_SUCCESS, $deployment->getAttribute('resourceType'), + $resource->getInternalId(), 1); + $addResourceMetrics($queue, METRIC_BUILDS_COMPUTE_SUCCESS, METRIC_RESOURCE_TYPE_BUILDS_COMPUTE_SUCCESS, + METRIC_RESOURCE_TYPE_ID_BUILDS_COMPUTE_SUCCESS, $deployment->getAttribute('resourceType'), + $resource->getInternalId(), (int)$deployment->getAttribute('buildDuration', 0) * 1000); break; // Similar refactoring for the failed case and other metrics } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.env(1 hunks)app/controllers/api/project.php(6 hunks)app/controllers/general.php(1 hunks)app/controllers/shared/api.php(1 hunks)app/init/constants.php(2 hunks)app/worker.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Usage/Get.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Usage/XList.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(3 hunks)src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php(1 hunks)src/Appwrite/Platform/Workers/Functions.php(1 hunks)src/Appwrite/Platform/Workers/StatsResources.php(4 hunks)src/Appwrite/Platform/Workers/StatsUsage.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/controllers/general.php (1)
src/Appwrite/Auth/Key.php (1)
getDisabledMetrics(61-64)
🔇 Additional comments (32)
.env (1)
92-92: Verify the performance impact of increased stats collection frequency.The interval for collecting resource statistics has been significantly reduced from 3600 seconds (hourly) to 30 seconds. This 120x increase in collection frequency could potentially impact system performance.
Please confirm that the system has been tested with this increased collection frequency and that it doesn't introduce performance issues, particularly in production environments with high traffic.
app/worker.php (1)
375-375: LGTM: Made extras parameter optional with a default value.This change improves function flexibility by making the
$extrasparameter optional using a default value ofnull. The internal logic properly handles the case where$extrasis null, so this is a safe improvement.src/Appwrite/Platform/Modules/Functions/Http/Usage/XList.php (1)
64-72:✅ Verification successful
Consistent approach to metric identifiers using dynamic string replacements.
The function has been refactored to use dynamic string replacements for metric identifiers instead of static constants. This approach aligns with the broader metrics system refactoring across the application.
Ensure that all constant definitions referenced here (like
METRIC_RESOURCE_TYPE_DEPLOYMENTS) are properly defined in the system. Run this to verify:
🏁 Script executed:
#!/bin/bash # Check if all metric constants used in string replacements are defined rg -A 1 "METRIC_RESOURCE_TYPE_" app/init/constants.phpLength of output: 2509
Approval: Metric Identifiers Replacement Verified
The dynamic string replacement approach for metric identifiers in this snippet is consistent with our broader metrics refactoring. All constants referenced (e.g.,
METRIC_RESOURCE_TYPE_DEPLOYMENTS,METRIC_RESOURCE_TYPE_DEPLOYMENTS_STORAGE, etc.) are properly defined inapp/init/constants.phpas verified by the grep search. No further changes are needed.src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php (1)
76-81: Enhanced metric identification with resource type and resource ID.The metrics construction has been improved to use both resource type and specific resource ID, making metric tracking more granular and consistent with the new metrics system approach.
src/Appwrite/Platform/Modules/Functions/Http/Usage/Get.php (1)
72-80: Improved approach for metric identifiers.The changes to replace hardcoded metric strings with dynamic, resource-type-based identifiers using constants make the code more maintainable and consistent with the rest of the application. This refactoring allows for better organization and potential extension of metrics in the future.
src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php (1)
64-69: Standardization of metric identifiers is good.The change to use resource-type-based metrics for sites is consistent with the overall refactoring approach and improves maintainability.
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
440-447: Enhanced metric tracking with resource types.The addition of resource-type-based metrics alongside the existing ones provides better granularity and organization of usage statistics. This change is part of the broader standardization effort seen across the application.
app/controllers/shared/api.php (1)
178-181: Good refactoring of metrics for deployments.The code now correctly tracks metrics based on resource type and resource internal ID instead of using function-specific metrics. This change is consistent with the overall architecture improvements seen in other files, making the metrics system more flexible and maintainable.
src/Appwrite/Platform/Workers/StatsUsage.php (3)
192-199: Dynamic resource type metrics implementation looks goodThe code now properly handles both 'functions' and 'sites' collections by using dynamic placeholders
{resourceType}and{resourceInternalId}to generate metrics keys. This enhances flexibility and reusability.
206-209: Good addition of resource-type specific metricsAdding resource-type specific metrics alongside the generic ones allows for more granular analytics while maintaining backward compatibility.
217-220: Consistent metrics 8000 structure across different resource typesThe implementation maintains consistent structure across all metrics (deployments, storage, builds, executions, etc.), making the code more maintainable and predictable.
Also applies to: 228-231, 239-242, 250-253, 261-264, 272-275
app/controllers/general.php (4)
589-593: Good implementation of API key-based metrics controlThe code now properly respects the disabled metrics configuration from the API key, enhancing customization and control over metrics collection.
595-600: Clean implementation of dynamic resource-type metrics variablesCreating separate variables for each metric type at the beginning of the block enhances readability and maintainability, making it clear which metrics are being tracked.
601-627: Comprehensive handling of site-specific metricsThe implementation correctly handles different requirements for sites:
- Disables network metrics for all sites
- Conditionally disables execution metrics for non-SSR sites
- Adds site-specific metrics with proper identifiers
This ensures accurate tracking based on deployment resource type.
630-644: Clear calculation and tracking of compute metricsThe implementation properly calculates compute metrics based on execution duration and resource specifications, then adds them to the stats queue with both generic and resource-specific keys.
app/controllers/api/project.php (4)
152-152: Updated metric key construction for better resource type categorizationThe change now uses a more descriptive metric identifier with both resource type and ID placeholders, which aligns with the broader refactoring across the application.
168-168: Consistent implementation of dynamic resource-based metricsThis follows the same pattern established earlier, replacing static metric identifiers with dynamic ones based on resource type and ID.
184-184: Improved metric construction for builds MB-secondsThe change adapts the builds MB-seconds metric to use the new resource-type-based identifier pattern.
233-234: Consistent metric construction for storage metricsBoth deployment and build storage metrics now use the new format with resource type and ID, which improves consistency across the metrics system.
Also applies to: 239-239
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (4)
849-862: Added disabledMetrics array for site deploymentsThis change enhances the JWT payload by disabling specific metrics during the build process for sites. It includes both generic metrics and resource-type specific metrics, ensuring that unnecessary metrics aren't collected during screenshots/previews.
1198-1202: Implemented dynamic resource-type metrics for build successThe code now uses the deployment's resourceType attribute to dynamically generate metrics for successful builds, making the metric system more flexible and capable of supporting different resource types beyond just functions.
1207-1211: Consistent implementation for build failuresSimilar to the build success metrics, the failure metrics now also use the dynamic resource-type pattern, ensuring consistency across the metrics system.
1219-1226: Comprehensive metric collection for all build typesThe change ensures that all build-related metrics follow the same pattern, capturing data at both the resource-type level and the specific resource instance level. This provides both aggregate and detailed metrics for analysis.
src/Appwrite/Platform/Workers/Functions.php (3)
568-569: Consistent resource-based metric naming.
These lines correctly apply the new resource-based placeholders for execution metrics, ensuring consistency with the broader usage-stat refactor.
571-572: Accurate tracking of compute time.
Usingstr_replaceto dynamically generate metric keys is fine here, and storing execution duration in milliseconds aligns with the existing approach.
574-575: Memory usage metrics appear correctly assigned.
Including the memory-based metric for MB-seconds ensures usage is captured accurately for function executions.src/Appwrite/Platform/Workers/StatsResources.php (4)
202-202: Initiating combined counting for sites and functions.
CallingcountForSitesAndFunctionsnaturally consolidates logic for both resource types. This is a clean integration point in the stats flow.
306-319: Method to consolidate sites and functions counting.
This approach nicely streamlines deployment-related metric calculations while keeping the code brief.
321-369: Function usage stats extraction looks solid.
The incremental approach of summing storage and counting deployments for each function is clear. The repeatedstr_replacecalls are acceptable for readability in this context.
371-415: Parallel logic for site metrics.
Mirroring the function stats logic for sites maintains consistency and simplifies the architecture for usage tracking.app/init/constants.php (2)
201-226: Enhanced resource-based constants.
These new constants unify the naming scheme for many deployment and execution metrics. They provide a flexible way to handle multiple resource types.
241-246: Additional site-based metrics.
Introducing site-specific inbound/outbound/request metrics is a solid step in capturing more granular usage data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (1)
61-66: Consider using a float forbuildsTimeAverage.Casting the average build time to an integer discards fractional precision. If precise build timings are beneficial, define it as a float instead:
-'type' => self::TYPE_INTEGER, +'type' => self::TYPE_FLOAT,src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
7-7: Validate the inheritance design.
UsageSitenow extendsUsageFunction, which might introduce domain ambiguity if sites and functions diverge. Consider extracting shared usage logic into a more generic base class to avoid confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Appwrite/Platform/Modules/Functions/Http/Usage/Get.php(2 hunks)src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php(2 hunks)src/Appwrite/Utopia/Response/Model/UsageFunction.php(2 hunks)src/Appwrite/Utopia/Response/Model/UsageSite.php(1 hunks)src/Appwrite/Utopia/Response/Model/UsageSites.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Modules/Functions/Http/Usage/Get.php
🧰 Additional context used
🧬 Code Definitions (1)
src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (2)
UsageFunction(8-176)__construct(10-155)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: Benchmark
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (6)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (2)
37-42: NewbuildsSuccessmetric looks good.This addition is consistent with your existing usage metrics, and helps capture successful build counts.
43-48: NewbuildsFailedmetric looks good.This complements the success metric and provides additional insight into failed builds.
src/Appwrite/Utopia/Response/Model/UsageSites.php (2)
7-7:✅ Verification successful
Ensure
UsageFunctionsis a valid parent class.Verify that
UsageFunctionsis defined and importable. A missing or incorrect definition forUsageFunctionswill trigger a class not found error at runtime.
🏁 Script executed:
#!/bin/bash # Locate the definition of UsageFunctions in the codebase. fd --hidden --type f --glob "*UsageFunctions.php" --exec cat {} | rg "class UsageFunctions"Length of output: 130
UsageFunctions is properly defined and importable. The script confirms its definition (i.e.,
class UsageFunctions extends Model), so no issues exist with its usage as a parent class inUsageSites.
12-13:❓ Verification inconclusive
Check for references to removed rules.
By removing
functionsTotalandfunctions, ensure you no longer rely on them in any other part of the code.
🏁 Script executed:
#!/bin/bash # Search for references to functionsTotal or functions in the codebase rg "functionsTotal|functions"Length of output: 66061
Action: Verify that removal of
functionsTotalandfunctionsvalidation rules is safe.After running the search, no occurrences of
functionsTotalwere found anywhere in the code. Although the stringfunctionsappears in many locations (mostly within API endpoints and test assertions), these references are used in different contexts (such as REST routes and event channels) and do not appear to be directly linked to the validation rules removed in the UsageSites model.Please perform a manual review of key components (e.g. any model validations or response processing that might have previously depended on these rule names) to confirm that removing these rules does not affect any related functionality.
src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php (1)
76-86:✅ Verification successful
Verify the defined metric constants.
You’re dynamically generating metric names via
str_replace. Confirm that each referenced constant (e.g.,METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS) is correctly declared and spelled.
🏁 Script executed:
#!/bin/bash rg -A 3 "METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS|METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS_STORAGE|METRIC_RESOURCE_TYPE_ID_BUILDS|METRIC_RESOURCE_TYPE_ID_BUILDS_STORAGE|METRIC_RESOURCE_TYPE_ID_BUILDS_COMPUTE|METRIC_RESOURCE_TYPE_ID_EXECUTIONS|METRIC_RESOURCE_TYPE_ID_EXECUTIONS_COMPUTE|METRIC_RESOURCE_TYPE_ID_BUILDS_MB_SECONDS|METRIC_RESOURCE_TYPE_ID_EXECUTIONS_MB_SECONDS|METRIC_RESOURCE_TYPE_ID_BUILDS_SUCCESS|METRIC_RESOURCE_TYPE_ID_BUILDS_FAILED"Length of output: 28066
Metric Constants Verified:
After reviewing the code and confirming the definitions inapp/init/constants.php, all metric constants referenced insrc/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php(includingMETRIC_RESOURCE_TYPE_ID_DEPLOYMENTS,METRIC_RESOURCE_TYPE_ID_DEPLOYMENTS_STORAGE,METRIC_RESOURCE_TYPE_ID_BUILDS,METRIC_RESOURCE_TYPE_ID_BUILDS_STORAGE,METRIC_RESOURCE_TYPE_ID_BUILDS_COMPUTE,METRIC_RESOURCE_TYPE_ID_EXECUTIONS,METRIC_RESOURCE_TYPE_ID_EXECUTIONS_COMPUTE,METRIC_RESOURCE_TYPE_ID_BUILDS_MB_SECONDS,METRIC_RESOURCE_TYPE_ID_EXECUTIONS_MB_SECONDS,METRIC_RESOURCE_TYPE_ID_BUILDS_SUCCESS, andMETRIC_RESOURCE_TYPE_ID_BUILDS_FAILED) are correctly declared and spelled. No action is required.src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
11-11: Inherited constructor call looks fine.Calling
parent::__construct()is appropriate to ensure rules fromUsageFunctionare loaded.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php (1)
62-73: Consider using named constants instead of array indicesThe metrics array is referenced by index in the response construction (e.g.,
$metrics[0],$metrics[1]), which could lead to potential issues if the order changes. Consider using named constants or creating a mapping between metrics and response fields to make the code more maintainable.- $metrics = [ - METRIC_SITES, - str_replace("{resourceType}", RESOURCE_TYPE_SITES, METRIC_RESOURCE_TYPE_DEPLOYMENTS), - ... - ]; + $metricKeys = [ + 'sites' => METRIC_SITES, + 'deployments' => str_replace("{resourceType}", RESOURCE_TYPE_SITES, METRIC_RESOURCE_TYPE_DEPLOYMENTS), + ... + ]; + $metrics = array_values($metricKeys); // Then later in the response - 'sitesTotal' => $usage[$metrics[0]]['total'], + 'sitesTotal' => $usage[$metricKeys['sites']]['total'],Also applies to: 119-140
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php(2 hunks)src/Appwrite/Utopia/Response/Model/UsageSites.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: Benchmark
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (7)
src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php (3)
64-72: LGTM: Refactored metrics to use dynamic resource-type-based identifiersThis change replaces static metrics constants with dynamic ones using
str_replaceto generate metric names based on resource types. This approach makes the code more maintainable and follows the broader restructuring of the metrics system across the application.
126-127: LGTM: Added new execution metrics to responseThe addition of execution-related metrics (
executionsTotal,executionsTimeTotal,executions,executionsTime) correctly corresponds to the new metrics added in the metrics array. These fields properly expose the execution statistics for sites.Also applies to: 134-135
137-139: LGTM: Reorganized and added MB seconds metricsThe reorganization of the MB seconds metrics and addition of execution-specific MB seconds fields is well-structured. The code maintains a consistent pattern between the metrics array and response construction.
src/Appwrite/Utopia/Response/Model/UsageSites.php (4)
7-7: Class inheritance change improves code organization.Changing the parent class from
ModeltoUsageFunctionsindicates a refactoring towards a more hierarchical and DRY metrics system. This aligns with the PR's goal of refactoring the metrics system across the application.
11-14: Good approach to inheritance customization.The implementation correctly calls the parent constructor and then removes function-specific rules that aren't applicable to sites. This selective inheritance pattern allows code reuse while maintaining proper separation of concerns.
15-27: Metrics definition looks correct.The site-specific metrics (
sitesTotalandsites) are properly defined with appropriate types, descriptions, default values, and examples. This maintains consistency with the metrics tracking system while focusing on site-specific statistics.
7-29:❓ Verification inconclusive
Verify consistent metrics behavior across resource types.
While the inheritance change is good for code organization, ensure that this refactoring maintains consistent behavior across all resource types and doesn't introduce any regressions in metrics collection.
🏁 Script executed:
#!/bin/bash # Check for related metric models and patterns echo "Checking related usage metric models..." fd "Usage.*\.php" src/Appwrite/Utopia/Response/Model/ --exec cat {} \; | grep -E "extends|removeRule|addRule" echo -e "\nChecking for metric collection in controllers..." rg "getMetrics|setMetric" app/controllers/ -A 3 -B 3Length of output: 5963
Ensure uniform metrics behavior across resource types.
The refactoring in
UsageSites—removing thefunctionsTotalandfunctionsrules and then addingsitesTotalandsites—follows the established pattern seen in other usage models (e.g., inUsageFunctions,UsageProject, etc.). This change appears consistent with similar model implementations. However, note that our search for controller methods usinggetMetricsorsetMetricdid not return any results. To be safe, please verify through manual testing or further inspection that any downstream logic relying on these metrics (if present) continues to function correctly.
- Action Items:
- Confirm that the new site-related metrics behave as expected in all relevant contexts.
- Manually test that controllers or endpoints that rely on these metrics are not adversely affected.
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (2)
tests/e2e/General/UsageTest.php (2)
1244-1247:⚠️ Potential issueDuplicate issue with execution metrics in global site stats.
Same issue as above - the test is verifying function execution metrics for site usage.
-$this->assertEquals($executions, $response['body']['executions'][array_key_last($response['body']['executions'])]['value']); -$this->validateDates($response['body']['executions']); -$this->assertEquals($executionTime, $response['body']['executionsTime'][array_key_last($response['body']['executionsTime'])]['value']); -$this->validateDates($response['body']['executionsTime']); +// Verify site-specific global metrics +$this->assertEquals(1, $response['body']['sites'][array_key_last($response['body']['sites'])]['value']); +$this->validateDates($response['body']['sites']); +$this->assertEquals(2, $response['body']['deployments'][array_key_last($response['body']['deployments'])]['value']); +$this->validateDates($response['body']['deployments']);
28-78:⚠️ Potential issueFix duplicate trait alias declarations.
The trait resolution block contains several duplicate alias declarations for SitesBase methods which can lead to unexpected behavior or PHP warnings:
listVariablesSite(lines 52-53)updateVariableSite(lines 54-55)deleteVariableSite(lines 55-56)getDeploymentSite(lines 57-58)listDeploymentsSite(lines 59-60)deleteDeploymentSite(lines 61-62)setupDuplicateDeploymentSite(lines 63-64)createDuplicateDeploymentSite(lines 65-66)createTemplateDeploymentSite(lines 67-68)getUsageSite(lines 69-70)getTemplateSite(lines 71-72)getDeploymentDownloadSite(lines 73-74)cancelDeploymentSite(lines 75-76)use SitesBase { FunctionsBase::createDeployment insteadof SitesBase; FunctionsBase::setupDeployment insteadof SitesBase; FunctionsBase::createVariable insteadof SitesBase; FunctionsBase::getVariable insteadof SitesBase; FunctionsBase::listVariables insteadof SitesBase; FunctionsBase::updateVariable insteadof SitesBase; FunctionsBase::deleteVariable insteadof SitesBase; FunctionsBase::getDeployment insteadof SitesBase; FunctionsBase::listDeployments insteadof SitesBase; FunctionsBase::deleteDeployment insteadof SitesBase; FunctionsBase::setupDuplicateDeployment insteadof SitesBase; FunctionsBase::createDuplicateDeployment insteadof SitesBase; FunctionsBase::createTemplateDeployment insteadof SitesBase; FunctionsBase::getUsage insteadof SitesBase; FunctionsBase::getTemplate insteadof SitesBase; FunctionsBase::getDeploymentDownload insteadof SitesBase; FunctionsBase::cancelDeployment insteadof SitesBase; FunctionsBase::listSpecifications insteadof SitesBase; SitesBase::createDeployment as createDeploymentSite; SitesBase::setupDeployment as setupDeploymentSite; SitesBase::createVariable as createVariableSite; SitesBase::getVariable as getVariableSite; SitesBase::listVariables as listVariablesSite; - SitesBase::listVariables as listVariablesSite; SitesBase::updateVariable as updateVariableSite; - SitesBase::updateVariable as updateVariableSite; SitesBase::deleteVariable as deleteVariableSite; - SitesBase::deleteVariable as deleteVariableSite; SitesBase::getDeployment as getDeploymentSite; - SitesBase::getDeployment as getDeploymentSite; SitesBase::listDeployments as listDeploymentsSite; - SitesBase::listDeployments as listDeploymentsSite; SitesBase::deleteDeployment as deleteDeploymentSite; - SitesBase::deleteDeployment as deleteDeploymentSite; SitesBase::setupDuplicateDeployment as setupDuplicateDeploymentSite; - SitesBase::setupDuplicateDeployment as setupDuplicateDeploymentSite; SitesBase::createDuplicateDeployment as createDuplicateDeploymentSite; - SitesBase::createDuplicateDeployment as createDuplicateDeploymentSite; SitesBase::createTemplateDeployment as createTemplateDeploymentSite; - SitesBase::createTemplateDeployment as createTemplateDeploymentSite; SitesBase::getUsage as getUsageSite; - SitesBase::getUsage as getUsageSite; SitesBase::getTemplate as getTemplateSite; - SitesBase::getTemplate as getTemplateSite; SitesBase::getDeploymentDownload as getDeploymentDownloadSite; - SitesBase::getDeploymentDownload as getDeploymentDownloadSite; SitesBase::cancelDeployment as cancelDeploymentSite; - SitesBase::cancelDeployment as cancelDeploymentSite; SitesBase::listSpecifications as listSpecificationsSite; }
🧹 Nitpick comments (3)
tests/e2e/General/UsageTest.php (1)
1185-1186: Variables used but not properly initialized.The
$executionTimeand$executionsvariables are used in the test but may not be passed correctly from thetestPrepareSitesStatsmethod. The null coalescing is a good defensive approach, but these variables seem unrelated to site deployments.-$executionTime = $data['executionTime'] ?? 0; -$executions = $data['executions'] ?? 0; +// These variables aren't populated by testPrepareSitesStats and aren't needed +// for site deployment metrics +$executionTime = 0; +$executions = 0;src/Appwrite/Utopia/Response/Model/UsageFunction.php (1)
61-66: Consider storing the average time as a float
If the average compute time can be fractional or measured in milliseconds, you might consider using a float. Otherwise, clarify the time unit to avoid confusion.src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
7-7: Reevaluate extendingUsageFunction
Inheriting fromUsageFunctionpulls in many function-centric metrics. If those aren’t relevant for sites, consider a dedicated base class or removing unneeded metrics after calling the parent constructor.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php(2 hunks)src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php(2 hunks)src/Appwrite/Utopia/Response/Model/UsageFunction.php(3 hunks)src/Appwrite/Utopia/Response/Model/UsageFunctions.php(2 hunks)src/Appwrite/Utopia/Response/Model/UsageSite.php(1 hunks)src/Appwrite/Utopia/Response/Model/UsageSites.php(1 hunks)tests/e2e/General/UsageTest.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Appwrite/Platform/Modules/Sites/Http/Usage/Get.php
- src/Appwrite/Platform/Modules/Sites/Http/Usage/XList.php
🧰 Additional context used
🧬 Code Definitions (4)
src/Appwrite/Utopia/Response/Model/UsageFunctions.php (1)
src/Appwrite/Utopia/Response.php (1)
Response(131-862)
src/Appwrite/Utopia/Response/Model/UsageSite.php (3)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (2)
UsageFunction(8-190)__construct(10-169)src/Appwrite/Utopia/Response/Model/UsageSites.php (1)
__construct(9-68)src/Appwrite/Utopia/Response.php (1)
Response(131-862)
tests/e2e/General/UsageTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (5)
createDeployment(237-245)setupDeployment(35-67)createVariable(111-119)getDeployment(171-179)listDeployments(201-209)
src/Appwrite/Utopia/Response/Model/UsageSites.php (3)
src/Appwrite/Utopia/Response/Model/UsageFunctions.php (2)
UsageFunctions(8-197)__construct(10-176)src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
__construct(9-53)src/Appwrite/Utopia/Response.php (1)
Response(131-862)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (10)
tests/e2e/General/UsageTest.php (2)
1114-1178: LGTM! Test setup looks good.The test setup method successfully creates a site, deploys two site deployments (one active, one inactive), and verifies their status. The data array passed to the test method properly tracks the num F438 ber of deployments and their success/failure status.
1173-1175: LGTM! Accurate deployment counts.The test data correctly represents the scenario with two successful deployments and zero failed deployments, matching the actual test setup where both deployments reach the 'ready' state.
src/Appwrite/Utopia/Response/Model/UsageFunction.php (2)
37-42: Great new metric to track successful builds
The addition ofbuildsSuccessTotalprovides a clear, aggregated count of successful function builds.
43-48: Nicely labeled metric for failed builds
Keeping track of failed function builds withbuildsFailedTotalfollows the same clear naming scheme asbuildsSuccessTotal.src/Appwrite/Utopia/Response/Model/UsageFunctions.php (2)
100-105: Addition of 'buildsSuccessTotal' looks good
A helpful metric for counting successful function builds across multiple functions.
106-111: Good coverage for failed builds
buildsFailedTotalsimilarly ensures you have a direct metric for errors in build processes.src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
11-11: Confirm inherited rules are still relevant
Callingparent::__construct()loads function usage metrics intoUsageSite. Verify if all inherited rules are necessary or if you need to remove or override any of them.src/Appwrite/Utopia/Response/Model/UsageSites.php (3)
7-11: Inheritance change looks goodThe class now extends
UsageFunctionsinstead ofModeland properly calls the parent constructor. This change creates a more cohesive hierarchy for usage models.
13-14: Appropriate rule removalsCorrectly removing the function-related rules that were inherited from the parent class, as this class should focus on site usage metrics only.
21-27: Type update for sites metricsThe type for
siteshas been properly changed toResponse::MODEL_METRICand includes thearrayproperty to support multiple data points.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (15)
src/Appwrite/Utopia/Response/Model/UsageSite.php (6)
15-15: Incorrect description for 'requestsTotal'The property name suggests request counting, but the description refers to sites.
- 'description' => 'Total aggregated number of sites.', + 'description' => 'Total aggregated number of requests.',
21-21: Incorrect description for 'requests'The property name suggests request metrics, but the description refers to sites.
- 'description' => 'Aggregated number of sites per period.', + 'description' => 'Aggregated number of requests per period.',
28-28: Incorrect description for 'inboundTotal'The property is for inbound traffic, but the description refers to sites count.
- 'description' => 'Total aggregated number of sites.', + 'description' => 'Total aggregated inbound requests or traffic.',
34-34: Incorrect description for 'inbound'The property tracks inbound metrics, but the description refers to sites.
- 'description' => 'Aggregated number of sites per period.', + 'description' => 'Aggregated inbound requests or traffic per period.',
41-41: Incorrect description for 'outboundTotal'The property tracks outbound traffic, but the description refers to sites.
- 'description' => 'Total aggregated number of sites.', + 'description' => 'Total aggregated outbound requests or traffic.',
47-47: Incorrect description for 'outbound'The property tracks outbound metrics, but the description refers to sites.
- 'description' => 'Aggregated number of sites per period.', + 'description' => 'Aggregated outbound requests or traffic per period.',tests/e2e/General/UsageTest.php (3)
28-78: Fix duplicate alias declarations in trait resolutionThere are several duplicate alias declarations in the trait use block which should be removed to prevent unexpected behavior.
use SitesBase { FunctionsBase::createDeployment insteadof SitesBase; FunctionsBase::setupDeployment insteadof SitesBase; FunctionsBase::createVariable insteadof SitesBase; FunctionsBase::getVariable insteadof SitesBase; FunctionsBase::listVariables insteadof SitesBase; FunctionsBase::updateVariable insteadof SitesBase; FunctionsBase::deleteVariable insteadof SitesBase; FunctionsBase::getDeployment insteadof SitesBase; FunctionsBase::listDeployments insteadof SitesBase; FunctionsBase::deleteDeployment insteadof SitesBase; FunctionsBase::setupDuplicateDeployment insteadof SitesBase; FunctionsBase::createDuplicateDeployment insteadof SitesBase; FunctionsBase::createTemplateDeployment insteadof SitesBase; FunctionsBase::getUsage insteadof SitesBase; FunctionsBase::getTemplate insteadof SitesBase; FunctionsBase::getDeploymentDownload insteadof SitesBase; FunctionsBase::cancelDeployment insteadof SitesBase; FunctionsBase::listSpecifications insteadof SitesBase; SitesBase::createDeployment as createDeploymentSite; SitesBase::setupDeployment as setupDeploymentSite; SitesBase::createVariable as createVariableSite; SitesBase::getVariable as getVariableSite; SitesBase::listVariables as listVariablesSite; - SitesBase::listVariables as listVariablesSite; SitesBase::updateVariable as updateVariableSite; - SitesBase::updateVariable as updateVariableSite; SitesBase::deleteVariable as deleteVariableSite; - SitesBase::deleteVariable as deleteVariableSite; SitesBase::getDeployment as getDeploymentSite; - SitesBase::getDeployment as getDeploymentSite; SitesBase::listDeployments as listDeploymentsSite; - SitesBase::listDeployments as listDeploymentsSite; SitesBase::deleteDeployment as deleteDeploymentSite; - SitesBase::deleteDeployment as deleteDeploymentSite; SitesBase::setupDuplicateDeployment as setupDuplicateDeploymentSite; - SitesBase::setupDuplicateDeployment as setupDuplicateDeploymentSite; SitesBase::createDuplicateDeployment as createDuplicateDeploymentSite; - SitesBase::createDuplicateDeployment as createDuplicateDeploymentSite; SitesBase::createTemplateDeployment as createTemplateDeploymentSite; - SitesBase::createTemplateDeployment as createTemplateDeploymentSite; SitesBase::getUsage as getUsageSite; - SitesBase::getUsage as getUsageSite; SitesBase::getTemplate as getTemplateSite; - SitesBase::getTemplate as getTemplateSite; SitesBase::getDeploymentDownload as getDeploymentDownloadSite; - SitesBase::getDeploymentDownload as getDeploymentDownloadSite; SitesBase::cancelDeployment as cancelDeploymentSite; - SitesBase::cancelDeployment as cancelDeploymentSite; SitesBase::listSpecifications as listSpecificationsSite; }
1216-1219: Site executions metrics verification may be unrelated to test dataThe test is verifying execution metrics for site usage, but these values are defaulted to 0 and haven't been explicitly set in the test data. Consider either setting expected values in the test data or verifying site-specific metrics instead.
-$this->assertEquals($executions, $response['body']['executions'][array_key_last($response['body']['executions'])]['value']); -$this->validateDates($response['body']['executions']); -$this->assertEquals($executionTime, $response['body']['executionsTime'][array_key_last($response['body']['executionsTime'])]['value']); -$this->validateDates($response['body']['executionsTime']); +// Verify site-specific metrics instead +$this->assertEquals($data['deployments'], $response['body']['deployments'][array_key_last($response['body']['deployments'])]['value']); +$this->validateDates($response['body']['deployments']); +$this->assertEquals($data['deploymentsSuccess'], $response['body']['buildsSuccess'][array_key_last($response['body']['buildsSuccess'])]['value']); +$this->validateDates($response['body']['buildsSuccess']); +$this->assertEquals($data['deploymentsFailed'], $response['body']['buildsFailed'][array_key_last($response['body']['buildsFailed'])]['value']); +$this->validateDates($response['body']['buildsFailed']);
1244-1247: Same issue with site executions metrics in global statsSimilar to the previous comment, execution metrics verification for sites may be unrelated to the test data.
-$this->assertEquals($executions, $response['body']['executions'][array_key_last($response['body']['executions'])]['value']); -$this->validateDates($response['body']['executions']); -$this->assertEquals($executionTime, $response['body']['executionsTime'][array_key_last($response['body']['executionsTime'])]['value']); -$this->validateDates($response['body']['executionsTime']); +// Verify site-specific metrics instead +$this->assertEquals($data['deployments'], $response['body']['deployments'][array_key_last($response['body']['deployments'])]['value']); +$this->validateDates($response['body']['deployments']); +$this->assertEquals($data['deploymentsSuccess'], $response['body']['buildsSuccess'][array_key_last($response['body']['buildsSuccess'])]['value']); +$this->validateDates($response['body']['buildsSuccess']); +$this->assertEquals($data['deploymentsFailed'], $response['body']['buildsFailed'][array_key_last($response['body']['buildsFailed'])]['value']); +$this->validateDates($response['body']['buildsFailed']);src/Appwrite/Utopia/Response/Model/UsageSites.php (6)
28-33: Fix the description for requestsTotalThe description "Total aggregated number of sites" is incorrect for this metric and should be updated.
->addRule('requestsTotal', [ 'type' => self::TYPE_INTEGER, - 'description' => 'Total aggregated number of sites.', + 'description' => 'Total aggregated number of requests.', 'default' => 0, 'example' => 0, ])
34-40: Fix the description for requestsThe description "Aggregated number of sites per period" is incorrect for this metric and should be updated.
->addRule('requests', [ 'type' => Response::MODEL_METRIC, - 'description' => 'Aggregated number of sites per period.', + 'description' => 'Aggregated number of requests per period.', 'default' => 0, 'example' => 0, 'array' => true ])
41-46: Fix the description for inboundTotalThe description "Total aggregated number of sites" is incorrect for this metric and should be updated.
->addRule('inboundTotal', [ 'type' => self::TYPE_INTEGER, - 'description' => 'Total aggregated number of sites.', + 'description' => 'Total aggregated inbound traffic.', 'default' => 0, 'example' => 0, ])
47-53: Fix the description for inboundThe description "Aggregated number of sites per period" is incorrect for this metric and should be updated.
->addRule('inbound', [ 'type' => Response::MODEL_METRIC, - 'description' => 'Aggregated number of sites per period.', + 'description' => 'Aggregated inbound traffic per period.', 'default' => 0, 'example' => 0, 'array' => true ])
54-59: Fix the description for outboundTotalThe description "Total aggregated number of sites" is incorrect for this metric and should be updated.
->addRule('outboundTotal', [ 'type' => self::TYPE_INTEGER, - 'description' => 'Total aggregated number of sites.', + 'description' => 'Total aggregated outbound traffic.', 'default' => 0, 'example' => 0, ])
60-66: Fix the description for outboundThe description "Aggregated number of sites per period" is incorrect for this metric and should be updated.
->addRule('outbound', [ 'type' => Response::MODEL_METRIC, - 'description' => 'Aggregated number of sites per period.', + 'description' => 'Aggregated outbound traffic per period.', 'default' => 0, 'example' => 0, 'array' => true ])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Appwrite/Platform/Modules/Functions/Http/Usage/Get.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Usage/XList.php(3 hunks)src/Appwrite/Utopia/Response/Model/UsageSite.php(1 hunks)src/Appwrite/Utopia/Response/Model/UsageSites.php(1 hunks)tests/e2e/General/UsageTest.php(5 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/Appwrite/Utopia/Response/Model/UsageSite.php (2)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (2)
UsageFunction(8-190)__construct(10-169)src/Appwrite/Utopia/Response/Model/UsageSites.php (1)
__construct(9-68)
tests/e2e/General/UsageTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (9)
createDeployment(237-245)setupDeployment(35-67)getDeployment(171-179)deleteDeployment(247-255)setupDuplicateDeployment(257-276)createDuplicateDeployment(278-288)getUsage(300-308)cancelDeployment(419-427)getSite(161-169)
src/Appwrite/Utopia/Response/Model/UsageSites.php (3)
src/Appwrite/Utopia/Response/Model/UsageFunctions.php (2)
UsageFunctions(8-197)__construct(10-176)src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
__construct(9-53)src/Appwrite/Utopia/Response.php (1)
Response(131-862)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (9)
src/Appwrite/Platform/Modules/Functions/Http/Usage/Get.php (2)
72-82: Improved metrics identification using resource typesThe switch from static metric identifiers to dynamic, resource-type-based ones improves modularity and consistency across the application. This change aligns with the broader refactoring of the metrics system.
80-82: Good addition of build success/failure trackingAdding separate metrics for successful and failed builds provides valuable insights into the build process health and will help with monitoring and troubleshooting deployment issues.
Also applies to: 134-135, 152-153
src/Appwrite/Platform/Modules/Functions/Http/Usage/XList.php (2)
64-74: Improved metrics identification using resource typesSimilar to the changes in the Get class, this refactoring improves consistency by using a resource-type-based approach for metrics. The str_replace pattern is consistently applied throughout the codebase.
130-133: New metrics properly integrated in response structureThe new metrics for build success and failure counts are correctly integrated into both the totals and time-series data arrays in the response.
Also applies to: 144-145
src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
7-11: Good inheritance restructuringExtending UsageFunction instead of Model reduces code duplication and allows for better reuse of common metrics structure between functions and sites.
tests/e2e/General/UsageTest.php (2)
1114-1178: Good implementation of site statistics preparationThe testPrepareSitesStats method properly sets up test data for site usage statistics by creating deployments and checking their status.
1067-1067: Updated expected response counts to match new metricsThe increased expected count values (from 19 to 24 and from 21 to 25) correctly reflect the addition of new metrics in the responses.
Also applies to: 1092-1092
src/Appwrite/Utopia/Response/Model/UsageSites.php (2)
7-11: Class hierarchy change looks goodChanging the
UsageSitesclass to extendUsageFunctionsand calling the parent constructor is a good refactoring to promote code reuse and maintain inheritance of common metrics functionality.
13-14: Removal of irrelevant metrics is appropriateRemoving functions-related metrics from the sites usage model helps maintain a clean separation of concerns between different resource types.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/e2e/General/UsageTest.php (3)
28-78:⚠️ Potential issueFix duplicate trait alias declarations in SitesBase usage.
There are multiple duplicate alias declarations in your trait usage block that should be removed. These duplications are unnecessary and could potentially lead to unexpected behavior or maintainability issues.
Apply this diff to remove the duplicate aliases:
use SitesBase { FunctionsBase::createDeployment insteadof SitesBase; FunctionsBase::setupDeployment insteadof SitesBase; FunctionsBase::createVariable insteadof SitesBase; FunctionsBase::getVariable insteadof SitesBase; FunctionsBase::listVariables insteadof SitesBase; FunctionsBase::updateVariable insteadof SitesBase; FunctionsBase::deleteVariable insteadof SitesBase; FunctionsBase::getDeployment insteadof SitesBase; FunctionsBase::listDeployments insteadof SitesBase; FunctionsBase::deleteDeployment insteadof SitesBase; FunctionsBase::setupDuplicateDeployment insteadof SitesBase; FunctionsBase::createDuplicateDeployment insteadof SitesBase; FunctionsBase::createTemplateDeployment insteadof SitesBase; FunctionsBase::getUsage insteadof SitesBase; FunctionsBase::getTemplate insteadof SitesBase; FunctionsBase::getDeploymentDownload insteadof SitesBase; FunctionsBase::cancelDeployment insteadof SitesBase; FunctionsBase::listSpecifications insteadof SitesBase; SitesBase::createDeployment as createDeploymentSite; SitesBase::setupDeployment as setupDeploymentSite; SitesBase::createVariable as createVariableSite; SitesBase::getVariable as getVariableSite; SitesBase::listVariables as listVariablesSite; - SitesBase::listVariables as listVariablesSite; SitesBase::updateVariable as updateVariableSite; - SitesBase::updateVariable as updateVariableSite; SitesBase::deleteVariable as deleteVariableSite; - SitesBase::deleteVariable as deleteVariableSite; SitesBase::getDeployment as getDeploymentSite; - SitesBase::getDeployment as getDeploymentSite; SitesBase::listDeployments as listDeploymentsSite; - SitesBase::listDeployments as listDeploymentsSite; SitesBase::deleteDeployment as deleteDeploymentSite; - SitesBase::deleteDeployment as deleteDeploymentSite; SitesBase::setupDuplicateDeployment as setupDuplicateDeploymentSite; - SitesBase::setupDuplicateDeployment as setupDuplicateDeploymentSite; SitesBase::createDuplicateDeployment as createDuplicateDeploymentSite; - SitesBase::createDuplicateDeployment as createDuplicateDeploymentSite; SitesBase::createTemplateDeployment as createTemplateDeploymentSite; - SitesBase::createTemplateDeployment as createTemplateDeploymentSite; SitesBase::getUsage as getUsageSite; - SitesBase::getUsage as getUsageSite; SitesBase::getTemplate as getTemplateSite; - SitesBase::getTemplate as getTemplateSite; SitesBase::getDeploymentDownload as getDeploymentDownloadSite; - SitesBase::getDeploymentDownload as getDeploymentDownloadSite; SitesBase::cancelDeployment as cancelDeploymentSite; - SitesBase::cancelDeployment as cancelDeploymentSite; SitesBase::listSpecifications as listSpecificationsSite; }
1209-1219: 🛠️ Refactor suggestionSite execution metrics should not be verified for site deployments.
The test is asserting execution metrics (
executionsandexecutionsTime) for site usage, but these metrics are related to function executions, not site deployments. The site test should be focusing on site-specific metrics like deployments and builds instead.-$this->assertEquals($executions, $response['body']['executions'][array_key_last($response['body']['executions'])]['value']); -$this->validateDates($response['body']['executions']); -$this->assertEquals($executionTime, $response['body']['executionsTime'][array_key_last($response['body']['executionsTime'])]['value']); -$this->validateDates($response['body']['executionsTime']); +// Verify site-specific metrics instead +$this->assertEquals($data['deployments'], $response['body']['deployments'][array_key_last($response['body']['deployments'])]['value']); +$this->validateDates($response['body']['deployments']); +$this->assertEquals($data['deploymentsSuccess'], $response['body']['buildsSuccess'][array_key_last($response['body']['buildsSuccess'])]['value']); +$this->validateDates($response['body']['buildsSuccess']); +$this->assertEquals($data['deploymentsFailed'], $response['body']['buildsFailed'][array_key_last($response['body']['buildsFailed'])]['value']); +$this->validateDates($response['body']['buildsFailed']);
1227-1233: 🛠️ Refactor suggestionAdd assertions to verify site deployment metrics in global stats.
The test should verify that the deployment counts in the global stats match what was expected from the test setup.
$this->assertEquals(200, $response['headers']['status-code']); $this->assertEquals(31, count($response['body'])); $this->assertEquals($response['body']['range'], '30d'); $this->assertIsArray($response['body']['sites']); $this->assertIsArray($response['body']['deployments']); +$this->assertEquals($data['deployments'], $response['body']['deployments'][array_key_last($response['body']['deployments'])]['value'], "Deployments count doesn't match expected value"); +$this->assertEquals($data['deploymentsSuccess'], $response['body']['buildsSuccess'][array_key_last($response['body']['buildsSuccess'])]['value'], "Successful builds count doesn't match expected value"); +$this->assertEquals($data['deploymentsFailed'], $response['body']['buildsFailed'][array_key_last($response['body']['buildsFailed'])]['value'], "Failed builds count doesn't match expected value"); $this->assertIsArray($response['body']['deploymentsStorage']);
🧹 Nitpick comments (2)
src/Appwrite/Utopia/Response/Model/UsageSites.php (1)
41-66: Good implementation of bandwidth metrics.The bandwidth metrics for both inbound and outbound traffic provide valuable insights for monitoring network usage. The descriptions accurately reflect what each metric tracks.
However, there's a minor wording issue in the descriptions for
inboundandoutboundmetrics. The current text "Aggregated number of inbound/outbound bandwidth" would read better as "Aggregated inbound/outbound bandwidth per period" since bandwidth isn't typically described as a "number of" something.- 'description' => 'Aggregated number of inbound bandwidth per period.', + 'description' => 'Aggregated inbound bandwidth per period.', - 'description' => 'Aggregated number of outbound bandwidth per period.', + 'description' => 'Aggregated outbound bandwidth per period.',tests/e2e/General/UsageTest.php (1)
1185-1186: Variables used but not properly initialized.The
$executionTimeand$executionsvariables are being used but they aren't passed from thetestPrepareSitesStatsmethod. This appears to be intentional as they're set to 0 if not present, but should be properly documented.-$executionTime = $data['executionTime'] ?? 0; -$executions = $data['executions'] ?? 0; +// These values are intentionally set to 0 as they're related to function executions, not site deployments +$executionTime = 0; +$executions = 0;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Appwrite/Utopia/Response/Model/UsageFunction.php(3 hunks)src/Appwrite/Utopia/Response/Model/UsageFunctions.php(2 hunks)src/Appwrite/Utopia/Response/Model/UsageSite.php(1 hunks)src/Appwrite/Utopia/Response/Model/UsageSites.php(1 hunks)tests/e2e/General/UsageTest.php(7 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
src/Appwrite/Utopia/Response/Model/UsageFunctions.php (1)
src/Appwrite/Utopia/Response.php (1)
Response(131-862)
src/Appwrite/Utopia/Response/Model/UsageSite.php (2)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (2)
UsageFunction(8-190)__construct(10-169)src/Appwrite/Utopia/Response/Model/UsageSites.php (1)
__construct(9-68)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (1)
src/Appwrite/Utopia/Response.php (1)
Response(131-862)
tests/e2e/General/UsageTest.php (1)
tests/e2e/Services/Sites/SitesBase.php (5)
createDeployment(237-245)setupDeployment(35-67)createVariable(111-119)getDeployment(171-179)listDeployments(201-209)
src/Appwrite/Utopia/Response/Model/UsageSites.php (2)
src/Appwrite/Utopia/Response/Model/UsageFunctions.php (2)
UsageFunctions(8-197)__construct(10-176)src/Appwrite/Utopia/Response/Model/UsageSite.php (1)
__construct(9-53)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: Benchmark
- GitHub Check: E2E General Test
🔇 Additional comments (15)
src/Appwrite/Utopia/Response/Model/UsageFunction.php (3)
37-48: Good addition of metrics for tracking build success and failure.The new rules for
buildsSuccessTotalandbuildsFailedTotalprovide valuable granularity by separating successful and failed builds, which previously would have been combined in the generalbuildsTotalmetric. This allows for better monitoring of build quality and reliability.
61-66: Good addition of average build time tracking.Adding the
buildsTimeAveragemetric complements the existingbuildsTimeTotalby providing the average computation time, which is more useful for performance analysis than just the aggregate value. This will help identify performance trends over time.
154-167: Good addition of period-based metrics for build success and failure.The addition of
buildsSuccessandbuildsFailedmetrics with typeResponse::MODEL_METRICallows for time-series analysis of build success/failure rates, which is valuable for identifying trends and potential issues over time periods.src/Appwrite/Utopia/Response/Model/UsageFunctions.php (2)
100-111: Good addition of build success/failure tracking to UsageFunctions.The addition of these fields for tracking successful and failed function builds aligns with similar changes in the UsageFunction class, maintaining consistency across the metrics model.
161-174: Good addition of period-based build success/failure metrics.These new metrics
buildsSuccessandbuildsFailedprovide valuable time-series data that complements the total counters, allowing for more detailed analysis of build performance over time.src/Appwrite/Utopia/Response/Model/UsageSite.php (4)
7-7: Good inheritance change to reuse functionality.Extending
UsageFunctioninstead ofModeldirectly is a good refactoring that promotes code reuse. This change allows UsageSite to inherit all the metrics from UsageFunction while adding site-specific metrics.
11-11: Good use of parent constructor.Calling the parent constructor simplifies this class by inheriting all the base metrics without having to duplicate code, promoting DRY principles.
13-25: Good addition of site request metrics.Adding request tracking metrics is valuable for monitoring site usage and performance. The descriptions accurately reflect the purpose of these metrics.
26-51: Good addition of bandwidth tracking.The inbound and outbound bandwidth metrics provide important data for understanding network usage patterns and identifying potential bottlenecks or unusual traffic. The descriptions correctly reflect their purpose.
src/Appwrite/Utopia/Response/Model/UsageSites.php (3)
7-7: Good inheritance refactoring for UsageSites.Changing the parent class to
UsageFunctionsaligns with similar changes inUsageSite, maintaining consistency in the inheritance hierarchy and promoting code reuse.
11-14: Good class initialization with appropriate rule removal.Calling the parent constructor and then removing function-specific rules that don't apply to sites maintains a clean and relevant set of metrics while still leveraging the shared code structure.
28-40: Good implementation of request metrics.The request metrics with clear descriptions help track site traffic patterns over time. The definitions are consistent with those in the UsageSite class, maintaining good alignment between singular and plural usage models.
tests/e2e/General/UsageTest.php (3)
1067-1067: Expected count updated for functions metrics.The count assertion has been updated from a previous value to 24, which likely reflects additional metrics being tracked in the response after the usage stats refactoring.
1092-1092: Expected count updated for global functions metrics.The count assertion has been updated from a previous value to 25, which aligns with the added metrics in the system.
1114-1178: Well-implemented site statistics preparation method.The method creates test sites and deployments to gather usage metrics data, properly testing both active and inactive deployments and verifying their states.
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
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
Summary by CodeRabbit
New Features
Refactor
UsageSiteandUsageSitesclasses to enhance their functionality.Chores