Skip to content

Add Prometheus metrics support and update environment configuration#179

Open
YashShah-Josh wants to merge 7 commits intolingo/mainfrom
feature/prometheus
Open

Add Prometheus metrics support and update environment configuration#179
YashShah-Josh wants to merge 7 commits intolingo/mainfrom
feature/prometheus

Conversation

@YashShah-Josh
Copy link
Contributor

  • Introduced Prometheus metrics tracking for request counts and durations in main.py.
  • Updated .env.example to include SSL mode and Prometheus configuration variables.
  • Modified Dockerfile to create a directory for Prometheus metrics with appropriate permissions.

- Introduced Prometheus metrics tracking for request counts and durations in main.py.
- Updated .env.example to include SSL mode and Prometheus configuration variables.
- Modified Dockerfile to create a directory for Prometheus metrics with appropriate permissions.
- Instrument Next.js API routes with request logging and per-endpoint metrics, exposing a Prometheus-compatible `/api/metrics` endpoint and a `/metrics` dashboard page for inspecting backend and frontend metrics.
- Wrap the Node `pg` pool used by drizzle to measure every database query, exporting query counts and latency histograms alongside API metrics, and align DB SSL configuration via `DB_SSL_MODE` in env examples and migration script.
- Enhance the FastAPI service with structured logging, health and test-log endpoints, and unified Prometheus metrics (including banking SQLAlchemy queries), and extend `docker-compose` plus `prometheus.yml` so Prometheus/Grafana can scrape both backend and frontend metrics.

Made-with: Cursor
@github-actions
Copy link

🤖 AI Code Review (GLM via Ollama)

{
  "summary": "This PR introduces configurable database SSL modes via environment variables, adds a new metrics API endpoint for Prometheus export and management, and heavily instruments the profile summary endpoint with logging and metrics collection.",
  "critical_issues": [
    "The POST endpoint in /api/metrics allows clearing metrics without any authentication or authorization checks, allowing any user to disrupt observability data.",
    "The diff for app/src/app/api/profile/summary/route.ts is truncated, making it impossible to verify the correctness of the database query or error handling logic."
  ],
  "security_issues": [
    "In app/scripts/migrate.js, the 'require' SSL mode sets rejectUnauthorized: false, which leaves the database connection vulnerable to Man-in-the-Middle (MitM) attacks by ignoring certificate validation.",
    "The SSL logic for the 'prefer' mode in app/scripts/migrate.js is flawed; it resolves to passing false to the ssl config, effectively disabling SSL instead of preferring it."
  ],
  "performance_issues": [
    "Extensive synchronous console.log usage in app/src/app/api/profile/summary/route.ts blocks the event loop and will significantly degrade API throughput under load.",
    "Generating Request IDs using Math.random() is not collision-resistant, though the impact is low for logging purposes."
  ],
  "readability_suggestions": [
    "The conditional logic sslConfig === 'prefer' ? false : sslConfig in app/scripts/migrate.js is confusing and brittle; refactor to a clear switch statement.",
    "Replace manual console.log formatting with a structured logging library (e.g., pino or winston) to reduce boilerplate and improve log parsing."
  ],
  "test_coverage_assessment": "Missing",
  "test_recommendations": [
    "Add unit tests for the SSL configuration logic in migrate.js to verify that 'prefer', 'require', and 'disable' produce the correct connection objects.",
    "Add integration tests for the new /api/metrics endpoint to validate Prometheus output format and the security of the POST action.",
    "Add tests to verify that metrics are correctly recorded in app/src/app/api/profile/summary/route.ts for both success and error scenarios."
  ]
}

📊 AI Quality Score: 6/100

@github-actions
Copy link

🤖 AI Code Review (GLM via Ollama)

{
  "summary": "This PR introduces a metrics collection system (frontend dashboard and API endpoint), adds extensive logging to the profile summary API, and refactors database SSL configuration logic to be environment-dependent.",
  "critical_issues": [
    "The POST endpoint in `app/src/app/api/metrics/route.ts` allows clearing metrics via the 'clear' action without any authentication or authorization checks. This allows any anonymous user to destroy application observability data."
  ],
  "security_issues": [
    "The `profile/summary` route logs potentially sensitive PII (Personally Identifiable Information) to the console, specifically `User ID` and the full `Response Data` which contains email addresses. Logging PII to stdout is a security risk and privacy violation.",
    "The metrics dashboard stores the API key in `localStorage` (`app/src/app/metrics/page.tsx`). While common for client-side tools, this exposes the key to XSS attacks.",
    "The database migration script uses `rejectUnauthorized: false` for SSL. While often necessary for AWS RDS self-signed certs, this configuration disables certificate validation and makes the connection vulnerable to Man-in-the-Middle (MitM) attacks."
  ],
  "performance_issues": [
    "The `profile/summary` route uses synchronous `console.log` extensively (6+ times per request). Synchronous logging blocks the Node.js event loop, which will significantly degrade API throughput and increase latency under load."
  ],
  "readability_suggestions": [
    "The logic for `DB_SSL_MODE` in `migrate.js` is confusing. The 'prefer' mode sets `sslConfig` to the string 'prefer', but the Pool configuration ternary operator (`sslConfig === 'prefer' ? false : sslConfig`) forces it to `false`. This makes 'prefer' and 'disable' functionally identical, contradicting the comment.",
    "The excessive logging in `route.ts` obscures the business logic. Consider using a dedicated logging library (e.g., Winston or Pino) with configurable levels (debug vs production) rather than hard-coded `console.log` statements."
  ],
  "test_coverage_assessment": "Missing",
  "test_recommendations": [
    "Add unit tests for the SSL configuration logic in `app/scripts/migrate.js` to ensure the correct `ssl` object is generated for 'require', 'disable', and 'prefer' modes.",
    "Add integration tests for the new `app/src/app/api/metrics/route.ts` to verify that metrics are exported correctly and that the 'clear' action is properly secured.",
    "Add tests for `app/src/app/api/profile/summary/route.ts` to ensure the `metricsRecorder` is capturing the correct status codes and durations."
  ]
}

📊 AI Quality Score: 21/100

@github-actions
Copy link

🤖 AI Code Review (GLM via Ollama)

{
  "summary": "This PR introduces configurable database SSL modes, adds a metrics collection API, enhances the profile summary API with extensive logging and metrics recording, and adds a new frontend dashboard for viewing Prometheus metrics.",
  "critical_issues": [
    "In `app/scripts/migrate.js`, the SSL logic for the 'prefer' mode is contradictory. It sets `sslConfig` to 'prefer' but then passes `ssl: false` to the Pool constructor if mode is 'prefer', effectively disabling SSL instead of allowing the connection string to dictate behavior.",
    "In `app/src/app/api/profile/summary/route.ts`, there is a variable shadowing bug. `const userId` is declared inside the `try` block, shadowing the outer `let userId`. Consequently, if an error occurs after the inner declaration, the `catch` block references the outer `userId` (undefined), causing metrics to be recorded without a user context."
  ],
  "security_issues": [
    "The new `POST /api/metrics` endpoint in `app/src/app/api/metrics/route.ts` allows clearing metrics via an 'action': 'clear' payload without any authentication or authorization checks, allowing any user to disrupt observability."
  ],
  "performance_issues": [
    "The `app/src/app/api/profile/summary/route.ts` implementation uses extensive synchronous `console.log` statements within the request path. This blocks the event loop and adds unnecessary latency, especially under load.",
    "The metrics parsing logic in `app/src/app/metrics/page.tsx` performs heavy string manipulation and regex on the client side. If the metrics payload is large, this could cause UI jank."
  ],
  "readability_suggestions": [
    "Refactor `app/src/app/api/profile/summary/route.ts` to extract the logging and metrics recording logic into a middleware or higher-order function to separate concerns and clean up the business logic.",
    "The file `app/src/app/metrics/page.tsx` is over 500 lines. Consider extracting the metrics parsing logic and specific UI components (e.g., MetricsConfig, MetricsSummary) into separate modules."
  ],
  "test_coverage_assessment": "Missing",
  "test_recommendations": [
    "Add unit tests for `app/scripts/migrate.js` to verify that the `ssl` object passed to `new Pool` is correct for 'require', 'disable', and 'prefer' modes.",
    "Add integration tests for `app/src/app/api/metrics/route.ts` to ensure the clear action is gated behind authentication (once fixed) and the format is valid.",
    "Add tests for `app/src/app/api/profile/summary/route.ts` to ensure metrics are recorded correctly on both success and failure paths, specifically verifying the user ID is passed correctly in the error handler.",
    "Add tests for the frontend metrics parser in `app/src/app/metrics/page.tsx` to ensure it handles malformed or empty metric strings gracefully."
  ]
}

📊 AI Quality Score: 16/100

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.

1 participant