Add performance monitoring (prometheus for metrics and winston for logging) #1394
Conversation
|
DescriptionSummary By MatterAI
🔄 What Changed
🔍 Impact of the ChangeIntroduces comprehensive AI safety evaluation via Qualifire for content moderation, policy enforcement, and output quality. Enables pre/post-request analysis for prompt injections, PII leakage, grounding, and hallucinations. Strengthens observability with improved error logging. Enhances WalledAI integration with proper API key handling. 📁 Total Files Changed
🧪 Test Added/RecommendedAdded
Recommended
🔒Security Vulnerabilities
⏳ Estimated code review effortHIGH (~35 minutes) Tip Quality Recommendations
♫ Tanka Poem
Sequence DiagramsequenceDiagram
participant User
participant Gateway
participant Qualifire
participant WalledAI
User->>Gateway: POST /v1/chat/completions
Gateway->>Gateway: execute beforeRequestHook
alt Prompt Injections Check
Gateway->>Qualifire: postQualifire(input, prompt_injections: true)
Qualifire-->>Gateway: {verdict: boolean, data: results}
end
alt PII Check
Gateway->>Qualifire: postQualifire(input, pii_check: true)
Qualifire-->>Gateway: {verdict: boolean, data: results}
end
alt Dangerous Content Check
Gateway->>Qualifire: postQualifire(input, dangerous_content_check: true)
Qualifire-->>Gateway: {verdict: boolean, data: results}
end
alt WalledAI Protection
Gateway->>WalledAI: POST /v1/walled-protect
WalledAI-->>Gateway: {verdict: boolean, data: results}
end
Gateway->>Upstream: Forward request to AI provider
Upstream-->>Gateway: Return response
Gateway->>Gateway: execute afterRequestHook
alt Grounding Check
Gateway->>Qualifire: postQualifire(input, output, grounding_check: true)
Qualifire-->>Gateway: {verdict: boolean, data: results}
end
alt Hallucinations Check
Gateway->>Qualifire: postQualifire(input, output, hallucinations_check: true)
Qualifire-->>Gateway: {verdict: boolean, data: results}
end
alt Policy Compliance
Gateway->>Qualifire: postQualifire(input, output, assertions: [policies])
Qualifire-->>Gateway: {verdict: boolean, data: results}
end
Gateway-->>User: Return final response
|
![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. |
1 participant
No description provided.