Skip to content

SUC-1668 fix:database usage metrics #10483

Merged
stnguyen90 merged 1 commit into1.8.xfrom
SUC-1668-fix-database-usage-stats
Sep 15, 2025
Merged

SUC-1668 fix:database usage metrics #10483
stnguyen90 merged 1 commit into1.8.xfrom
SUC-1668-fix-database-usage-stats

Conversation

@Divyansha23
Copy link
Copy Markdown
Contributor

@Divyansha23 Divyansha23 commented Sep 12, 2025

What does this PR do?

In this PR, we are fixing the incorrect env vars responsible for displaying the incorrect metrics

  • METRIC_DATABASES_OPERATIONS_WRITES --> METRIC_DATABASE_ID_OPERATIONS_WRITES
  • METRIC_DATABASES_OPERATIONS_READS --> METRIC_DATABASE_ID_OPERATIONS_READS

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

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 12, 2025

📝 Walkthrough

Walkthrough

Renames 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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch SUC-1668-fix-database-usage-stats

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "SUC-1668 fix:database usage metrics" directly references the tracked issue and succinctly summarizes the primary change—fixing database usage metrics by renaming metric keys—so it accurately reflects the changeset described in the raw summary and PR objectives.
Description Check ✅ Passed The PR description explicitly states the intention to correct two environment variable names for database usage metrics and lists the exact renames, which directly matches the code changes shown in the summary (renaming METRIC_DATABASES_OPERATIONS_READS/WRITES to METRIC_DATABASE_ID_OPERATIONS_READS/WRITES), so the description is related to the changeset.

@Divyansha23 Divyansha23 changed the title fixed db stats env var SUC-1668 fix:database usage metrics Sep 12, 2025
@github-actions
Copy link
Copy Markdown

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 918912c and ef3d38c.

📒 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.

Comment on lines +80 to 82
str_replace('{databaseInternalId}', $database->getSequence(), METRIC_DATABASE_ID_OPERATIONS_READS),
str_replace('{databaseInternalId}', $database->getSequence(), METRIC_DATABASE_ID_OPERATIONS_WRITES)
];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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_' -C2

Length 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_" || true

Length 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.

@github-actions
Copy link
Copy Markdown

✨ Benchmark results

  • Requests per second: 1,217
  • Requests with 200 status code: 219,073
  • P99 latency: 0.161276113

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,217 973
200 219,073 175,103
P99 0.161276113 0.203972083

@stnguyen90 stnguyen90 self-requested a review September 12, 2025 15:03
@stnguyen90 stnguyen90 merged commit 64bb1ef into 1.8.x Sep 15, 2025
41 checks passed
@stnguyen90 stnguyen90 deleted the SUC-1668-fix-database-usage-stats branch September 15, 2025 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants