SUC-1668 fix:database usage metrics #10483
Conversation
📝 WalkthroughWalkthroughRenames two metric keys in the database usage endpoint within src/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.php: METRIC_DATABASES_OPERATIONS_READS → METRIC_DATABASE_ID_OPERATIONS_READS and METRIC_DATABASES_OPERATIONS_WRITES → METRIC_DATABASE_ID_OPERATIONS_WRITES. The logic continues to substitute {databaseInternalId} using the database sequence. No other metrics, loops, or authorization logic are modified. No public/exported entity signatures are changed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
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: 1
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.php (2)
91-105: Prefer consistent Document accessors to avoid mixed styles.Use getAttribute for totals (like below) to match the loop that already uses getAttribute and avoid relying on ArrayAccess semantics.
- $stats[$metric]['total'] = $result['value'] ?? 0; + $stats[$metric]['total'] = $result instanceof Document + ? ($result->getAttribute('value') ?? 0) + : 0;
73-83: Add a light regression test to lock the fix.A minimal integration test that performs one read and one write against the database, then calls this endpoint and asserts that databaseReadsTotal/databaseWritesTotal increased (>0) will prevent future key regressions.
I can draft a PHPUnit test that seeds a DB, performs 1 insert + 1 fetch, hits GET /v1/databases/:id/usage with range=24h, and asserts non-zero reads/writes—want me to add it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
⏰ 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). (8)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.php (1)
128-144: Response mapping remains consistent.Indexes [3] and [4] still map to reads/writes totals and series; no downstream shape change. Good.
| str_replace('{databaseInternalId}', $database->getSequence(), METRIC_DATABASE_ID_OPERATIONS_READS), | ||
| str_replace('{databaseInternalId}', $database->getSequence(), METRIC_DATABASE_ID_OPERATIONS_WRITES) | ||
| ]; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Correct metric key fix — aligns with per-database metrics.
Switching to METRIC_DATABASE_ID_OPERATIONS_READS/WRITES matches the other ID-scoped metrics in this endpoint and should pull the right time series for the selected database.
Run this to confirm constants exist and there are no lingering references to the old keys:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check for any remaining old metric keys (should be empty):"
rg -nP --glob '!**/vendor/**' 'METRIC_DATABASES_OPERATIONS_(READS|WRITES)' -C2 || true
echo -e "\nVerify new constants are defined and referenced:"
rg -nP --glob '!**/vendor/**' 'METRIC_DATABASE_ID_OPERATIONS_(READS|WRITES)' -C3
echo -e "\nSpot-check other database-ID metrics for consistency with placeholder usage:"
rg -n --glob '!**/vendor/**' '{databaseInternalId}' -C2 | rg -n 'METRIC_DATABASE_ID_' -C2Length of output: 406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
# Produce a newline-separated file list: prefer git-tracked files if inside a repo, otherwise fallback to find
if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
git ls-files > "$tmp" || true
else
find . -type f -not -path './vendor/*' -not -path './.git/*' -print > "$tmp" || true
fi
if [ ! -s "$tmp" ]; then
echo "No files found to search."
exit 0
fi
echo "Searching for old metric keys (METRIC_DATABASES_OPERATIONS_(READS|WRITES))..."
xargs -a "$tmp" grep -nE "METRIC_DATABASES_OPERATIONS_(READS|WRITES)" || true
echo
echo "Searching for new metric keys (METRIC_DATABASE_ID_OPERATIONS_(READS|WRITES))..."
xargs -a "$tmp" grep -nE "METRIC_DATABASE_ID_OPERATIONS_(READS|WRITES)" || true
echo
echo "Searching for placeholder '{databaseInternalId}' occurrences..."
xargs -a "$tmp" grep -nF "{databaseInternalId}" || true
echo
echo "Searching for METRIC_DATABASE_ID_ constant definitions (define(...) or const ...)..."
xargs -a "$tmp" grep -nE "define\\s*\\(\\s*['\"]METRIC_DATABASE_ID_|const\\s+METRIC_DATABASE_ID_" || trueLength of output: 264289
Missing METRIC_DATABASE_ID_OPERATIONS_ constants — define them or revert the change.*
Get.php (src/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.php — lines 80–82) now uses METRIC_DATABASE_ID_OPERATIONS_READS/WRITES but those constants are not defined; app/init/constants.php defines METRIC_DATABASES_OPERATIONS_READS/WRITES (lines 189,191) and other code still references the old keys (app/controllers/api/project.php — lines 70–71, 83–84, 366–371).
- Add the new constants to app/init/constants.php, for example:
const METRIC_DATABASE_ID_OPERATIONS_READS = 'databases.{databaseInternalId}.operations.reads';
const METRIC_DATABASE_ID_OPERATIONS_WRITES = 'databases.{databaseInternalId}.operations.writes'; - Or revert src/.../Get.php (lines 80–82) to use the existing METRIC_DATABASES_OPERATIONS_* keys if per-database series aren’t supported yet.
- Verify no mixed references remain (app/controllers/api/project.php and docs like CONTRIBUTING.md around the {databaseInternalId} placeholders).
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Databases/Http/Databases/Usage/Get.php around
lines 80-82, the code now references METRIC_DATABASE_ID_OPERATIONS_READS/WRITES
which do not exist; either add these two constants to app/init/constants.php or
revert Get.php to use the existing METRIC_DATABASES_OPERATIONS_READS/WRITES
keys. To fix: if you intend per-database metric keys, add constants in
app/init/constants.php with the patterns
'databases.{databaseInternalId}.operations.reads' and
'databases.{databaseInternalId}.operations.writes' and update any other mixed
references; otherwise change the three str_replace calls in Get.php back to
METRIC_DATABASES_OPERATIONS_READS/WRITES so they match existing constants and
ensure no other files (e.g., app/controllers/api/project.php or docs) reference
the new keys.
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
In this PR, we are fixing the incorrect env vars responsible for displaying the incorrect metrics
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