Add performance monitoring (prometheus for metrics and winston for logging) #1394
Conversation
|
DescriptionSummary By MatterAI
🔄 What Changed
🔍 Impact of the ChangeEnables robust observability with metrics collection (Prometheus) and structured logging (Winston/Loki). Introduces scalable, persistent caching layer supporting multiple backends (Redis, file, memory, Cloudflare KV) with TTL and cleanup. Enhances system reliability with proper error boundary handling and background task management. Improves configuration flexibility with file-based settings. Strengthens rate limiting with Redis-backed atomic operations. 📁 Total Files Changed
🧪 Test Added/RecommendedRecommended
🔒Security Vulnerabilities
⏳ Estimated code review effortHIGH (~45 minutes) Tip Quality Recommendations
♫ Tanka Poem
Sequence DiagramsequenceDiagram
participant App as src/index.ts
participant Cache as CacheService
participant Redis as RedisCacheBackend
participant Settings as initializeSettings.ts
participant Logger as logger.ts
participant Prometheus as Prometheus
Note over App: Application Initialization
App->>Settings: getSettings()
Settings-->>App: settings from conf.json or defaults
App->>Cache: createCacheBackendsRedis(REDIS_CONNECTION_STRING)
Cache->>Redis: new RedisCacheBackend(redisUrl)
Redis-->>Cache: Redis backend instance
Cache-->>App: Initialize token/session/config caches
App->>Logger: createLogger('gateway')
Logger-->>App: Configured logger instance
App->>Prometheus: collectDefaultMetrics()
Prometheus-->>App: Metrics collection started
Note over App: Runtime Execution
App->>App: Add uncaughtException handler
App->>App: Add unhandledRejection handler
loop Every Request
App->>Cache: get/set/delete operations
Cache->>Redis: Execute cache commands
Redis-->>Cache: Return results
Cache-->>App: Cache results
end
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧪 PR Review is completed: Review of the new APM implementation with Prometheus and Winston. Key issues found in environment variable handling, error logging, and logger initialization.
Skipped files
package-lock.json: Skipped file patternwrangler.toml: Skipped file pattern
⬇️ Low Priority Suggestions (4)
src/apm/loki/logger.ts
Location:
src/apm/loki/logger.ts(Lines 31-31)🟡 Error Handling
Issue: The broad
try-catchblock inLokiLoggerinitialization swallows all errors and sets the logger tonull, which can lead to silent failures and make debugging difficult.Fix: Log specific errors during initialization to help with debugging while still providing a fallback.
Impact: Improves debuggability and error visibility
- LokiLogger = null; + console.error('[LOKI LOGGER] Failed to initialize Loki logger:', error); + LokiLogger = null; +
src/apm/prometheus/prometheusClient.ts
Location:
src/apm/prometheus/prometheusClient.ts(Lines 27-27)🟡 Performance
Issue: Calling
Environment({})multiple times (inenvConfig.tsand here) may cause redundant environment variable parsing.Fix: Use the already loaded environment variables from
envVarsinstead of callingEnvironment({})again.Impact: Reduces redundant computation and improves performance
- .PROMETHEUS_LABELS_METADATA_ALLOWED_KEYS?.replaceAll(' ', '') + envVars.PROMETHEUS_LABELS_METADATA_ALLOWED_KEYS?.replaceAll(' ', '') +
src/apm/prometheus/prometheusClient.ts
Location:
src/apm/prometheus/prometheusClient.ts(Lines 322-322)🟡 Error Handling
Issue: Returning an empty string on JSON parse error in
getCustomLabelsis not informative and may hide issues with metadata formatting.Fix: Return an empty object instead to maintain consistent return types and allow for better error handling upstream.
Impact: Improves error handling consistency
- return ''; + return {}; +
src/apm/index.ts
Location:
src/apm/index.ts(Lines 3-3)🟡 Compatibility
Issue: Direct access to
process.envmay not work in all environments (e.g., Cloudflare Workers) and could cause runtime errors.Fix: Add a check for
processandprocess.envexistence before accessing them.Impact: Improves compatibility across different runtime environments
- if (process && process.env.logger === 'loki') { + if (typeof process !== 'undefined' && process.env && process.env.logger === 'loki') { +
| requiredEnvVars.forEach((varName) => { | ||
| if (!env[varName]) { | ||
| console.error(`Missing required environment variable: ${varName}`); | ||
| process.exit(1); |
There was a problem hiding this comment.
🔴 Security & Stability
Issue: Using process.exit(1) in library code can cause the entire application to crash abruptly without proper cleanup. This is especially problematic in serverless environments.
Fix: Throw a specific error that can be caught and handled by the application's error handling mechanism, allowing for graceful degradation.
Impact: Improves application stability and allows for better error handling
| process.exit(1); | |
| throw new Error(`Missing required environment variable: ${varName}`); | |
| requiredEnvVars.forEach((varName) => { | ||
| if (!env[varName]) { | ||
| console.error(`Missing required environment variable: ${varName}`); | ||
| process.exit(1); |
There was a problem hiding this comment.
🔴 Security & Stability
Issue: Using process.exit(1) in library code can cause the entire application to crash abruptly without proper cleanup. This is especially problematic in serverless environments.
Fix: Throw a specific error that can be caught and handled by the application's error handling mechanism, allowing for graceful degradation.
Impact: Improves application stability and allows for better error handling
| process.exit(1); | |
| throw new Error(`Missing required environment variable: ${varName}`); | |
|
✅ Reviewed the changes: Review of APM integration for Prometheus and Winston/Loki |
|
✅ Reviewed the changes: Review of the addition of APM_LOGGER environment variable and related type safety improvements. |
|
✅ Reviewed the changes: Review complete. Found 1 issue related to potential sensitive data exposure in error logging. |
|
✅ Reviewed the changes: Review of new Qualifire plugins and related updates. Identified issues with error logging and type safety. |
|
✅ Reviewed the changes: PR introduces performance monitoring with Prometheus and Winston, along with new cache backends and a custom logger. Key areas for review include error handling, type safety, and consistent logging. |
2 participants
No description provided.