📊 Implement Advanced Analytics and Monitoring Dashboard with ZATCA Compliance #36
Conversation
…liance Co-authored-by: Fadil369 <[email protected]>
Co-authored-by: Fadil369 <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return { | ||
| "success": True, | ||
| "data": health_data, | ||
| "message": "System health metrics retrieved successfully", | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we should ensure that internal exception details are not exposed to external users via API responses. Specifically, in MonitoringService._check_database_health (and similar methods), we should avoid including the exception message in the returned dictionary. Instead, return a generic error message, while continuing to log the detailed error server-side for debugging.
Steps:
- In
backend/app/services/monitoring.py, update the exception handler in_check_database_health(and_get_application_metrics, if needed) to return a generic error message in the"error"field, e.g.,"An internal error occurred.", instead ofstr(e). - Do not change the logging statement; continue logging the full error for server-side diagnostics.
- No changes are needed in
backend/app/api/v1/monitoring.py, as the API endpoint will now only return generic error messages.
| @@ -115,7 +115,7 @@ | ||
| logger.error(f"Database health check failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "An internal error occurred.", | ||
| "connection_time_ms": None | ||
| } | ||
|
|
||
| @@ -157,7 +157,7 @@ | ||
| logger.error(f"Application metrics collection failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e) | ||
| "error": "An internal error occurred." | ||
| } | ||
|
|
||
| async def _check_alert_conditions(self, cpu: float, memory: float, disk: float) -> List[Dict[str, Any]]: |
| return { | ||
| "success": True, | ||
| "data": { | ||
| "alerts": active_alerts, | ||
| "total_count": len(active_alerts), | ||
| "critical_count": len([a for a in active_alerts if a.get("severity") == "critical"]), | ||
| "warning_count": len([a for a in active_alerts if a.get("severity") == "warning"]), | ||
| }, | ||
| "message": "Active alerts retrieved successfully", | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we need to ensure that internal exception details (such as the exception message in the "error" field) are not exposed to API consumers. Instead, we should return a generic error message in the API response and log the detailed exception information on the server for debugging purposes.
Specifically, in backend/app/services/monitoring.py, the _check_database_health and _get_application_metrics methods should not include the exception message in the returned dictionary. Instead, they should log the exception and return a generic error message. This change will prevent sensitive information from being propagated to the API layer and exposed to users.
Required changes:
- In
_check_database_health, replace"error": str(e)with a generic message, e.g.,"error": "Internal error"or simply omit the"error"field. - In
_get_application_metrics, do the same for the"error"field. - Ensure that exceptions are still logged server-side for debugging.
No new imports are needed, as logging is already configured.
| @@ -115,7 +115,7 @@ | ||
| logger.error(f"Database health check failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "Internal error", | ||
| "connection_time_ms": None | ||
| } | ||
|
|
||
| @@ -157,7 +157,7 @@ | ||
| logger.error(f"Application metrics collection failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e) | ||
| "error": "Internal error" | ||
| } | ||
|
|
||
| async def _check_alert_conditions(self, cpu: float, memory: float, disk: float) -> List[Dict[str, Any]]: |
| return { | ||
| "success": True, | ||
| "data": realtime_data, | ||
| "message": "Real-time metrics retrieved successfully", | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we need to ensure that sensitive exception details are not exposed to external users via API responses. Specifically, in the MonitoringService._check_database_health and _get_application_metrics methods, instead of including the stringified exception (str(e)) in the returned dictionary, we should log the error server-side and return a generic error message (e.g., "Internal error" or "Database health check failed"). This prevents leaking potentially sensitive information. The changes should be made only in the code regions shown, i.e., the return statements in the exception handlers of these two methods in backend/app/services/monitoring.py.
No changes are needed in the API endpoint code, as the service methods will now return sanitized error messages.
| @@ -115,7 +115,7 @@ | ||
| logger.error(f"Database health check failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "Database health check failed due to an internal error.", | ||
| "connection_time_ms": None | ||
| } | ||
|
|
||
| @@ -157,7 +157,7 @@ | ||
| logger.error(f"Application metrics collection failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e) | ||
| "error": "Application metrics collection failed due to an internal error." | ||
| } | ||
|
|
||
| async def _check_alert_conditions(self, cpu: float, memory: float, disk: float) -> List[Dict[str, Any]]: |
| return { | ||
| "success": True, | ||
| "data": status_data, | ||
| "message": "System status retrieved successfully", | ||
| } |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, we need to ensure that internal error details (such as exception messages) are not exposed to API consumers. Instead, we should log the error server-side and return a generic error message in the API response. Specifically, in MonitoringService._check_database_health and _get_application_metrics, we should replace the "error": str(e) field in the returned dictionary with a generic message (e.g., "error": "Internal error"). The actual exception should still be logged for debugging purposes. Only the generic message should be returned to the API layer, which will then be included in the response.
Files/regions to change:
backend/app/services/monitoring.py: In both_check_database_healthand_get_application_metrics, replace the"error": str(e)field with a generic message.- No changes are needed in
backend/app/api/v1/monitoring.pysince the API endpoint does not directly expose the exception, but rather the data returned from the service.
No new imports are needed, as logging is already used.
| @@ -115,7 +115,7 @@ | ||
| logger.error(f"Database health check failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e), | ||
| "error": "Internal error", | ||
| "connection_time_ms": None | ||
| } | ||
|
|
||
| @@ -157,7 +157,7 @@ | ||
| logger.error(f"Application metrics collection failed: {e}") | ||
| return { | ||
| "status": "error", | ||
| "error": str(e) | ||
| "error": "Internal error" | ||
| } | ||
|
|
||
| async def _check_alert_conditions(self, cpu: float, memory: float, disk: float) -> List[Dict[str, Any]]: |
Fadil369
left a comment
Fadil369
left a comment
There was a problem hiding this comment.
Great Implement Advanced Analytics and Monitoring Dashboard with ZATCA Compliance
|
This pull request introduces a comprehensive analytics and monitoring solution for the BrainSAIT store, focused on real-time system health, business analytics, and financial compliance (including ZATCA requirements). The changes include a new monitoring API, enhanced financial analytics, and integration of these endpoints into the backend. The documentation has also been updated to reflect these new features. Monitoring and Analytics API Enhancements
Financial Analytics Improvements
Documentation Updates
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive analytics and monitoring dashboard providing real-time system monitoring, business intelligence, and ZATCA-compliant financial reporting for the BrainSAIT B2B platform.
- Adds real-time system monitoring with CPU, memory, disk usage tracking and automated alerting
- Implements ZATCA-compliant financial analytics with Saudi 15% VAT calculations and reporting
- Introduces new monitoring APIs and enhanced analytics endpoints with multi-tenant support
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/analytics/FinancialAnalytics.tsx | New comprehensive financial dashboard with ZATCA compliance features and VAT reporting |
| frontend/src/components/analytics/AnalyticsDashboard.tsx | Enhanced with financial & VAT tab integration and data fetching |
| frontend/src/components/admin/MonitoringDashboard.tsx | New real-time system monitoring interface with live metrics and alerting |
| frontend/src/app/dashboard/admin/page.tsx | New admin portal with tabbed interface for monitoring, analytics, and settings |
| backend/requirements.txt | Added psutil dependency for system monitoring capabilities |
| backend/app/services/monitoring.py | New monitoring service with system health checks and performance metrics |
| backend/app/services/analytics.py | Enhanced with financial analytics and ZATCA compliance calculations |
| backend/app/main.py | Added monitoring router integration to API endpoints |
| backend/app/api/v1/monitoring.py | New monitoring API endpoints for health, performance, and alerts |
| backend/app/api/v1/analytics.py | Added financial analytics endpoint for ZATCA-compliant reporting |
| ANALYTICS_MONITORING_FEATURES.md | Documentation for new analytics and monitoring features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| useEffect(() => { | ||
| fetchData(); | ||
| }, [dateRange]); // fetchData is stable since it only depends on dateRange |
There was a problem hiding this comment.
The dependency array for useEffect is incorrect. fetchData depends on dateRange state but fetchData itself is not in the dependencies. This will cause stale closure issues. Either include fetchData in the dependencies or use useCallback to memoize fetchData with proper dependencies.
| }, [dateRange]); // fetchData is stable since it only depends on dateRange | |
| }, [dateRange]); | |
| useEffect(() => { | |
| fetchData(); | |
| }, [fetchData]); |
| // Auto-refresh every 30 seconds | ||
| const interval = setInterval(fetchData, 30000); | ||
| return () => clearInterval(interval); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect has an empty dependency array but fetchData is called inside it and also used in the interval. Since fetchData is defined in the component scope and could potentially change, it should be included in the dependency array or memoized with useCallback.
| WHERE mean_time > 1000 | ||
| ORDER BY mean_time DESC | ||
| LIMIT 5 | ||
| """)).fetchall() if hasattr(self.db, 'execute') else [] |
There was a problem hiding this comment.
The condition if hasattr(self.db, 'execute') is incorrect. All SQLAlchemy Session objects have an execute method. This check will always be True, making the fallback logic unreachable. The intended logic appears to be checking if the pg_stat_statements extension is available.
| """)).fetchall() if hasattr(self.db, 'execute') else [] | |
| # Get slow queries (if pg_stat_statements is available) | |
| try: | |
| slow_queries = self.db.execute(text(""" | |
| SELECT query, calls, total_time, mean_time | |
| FROM pg_stat_statements | |
| WHERE mean_time > 1000 | |
| ORDER BY mean_time DESC | |
| LIMIT 5 | |
| """)).fetchall() | |
| except (ProgrammingError, OperationalError): | |
| slow_queries = [] |
| # Calculate VAT breakdown | ||
| total_revenue = self.db.query(func.sum(Order.total_amount)).filter(*filters).scalar() or Decimal("0.00") | ||
| revenue_before_vat = total_revenue / (1 + vat_rate) | ||
| vat_amount = total_revenue - revenue_before_vat |
There was a problem hiding this comment.
There's a logical error in the VAT calculation. If total_revenue already includes VAT (as suggested by the variable name total_revenue_with_vat), then dividing by (1 + vat_rate) to get revenue_before_vat is correct. However, the subtraction vat_amount = total_revenue - revenue_before_vat would be the VAT amount, not additional VAT to be added. The variable naming and calculation logic need to be consistent.
| vat_amount = total_revenue - revenue_before_vat | |
| total_revenue_with_vat = self.db.query(func.sum(Order.total_amount)).filter(*filters).scalar() or Decimal("0.00") | |
| revenue_before_vat = total_revenue_with_vat / (1 + vat_rate) | |
| vat_amount = total_revenue_with_vat - revenue_before_vat |
This PR implements a comprehensive analytics and monitoring dashboard that provides real-time system insights, business intelligence, and Saudi ZATCA-compliant financial reporting for the BrainSAIT B2B platform.
🚀 Key Features Added
Real-time System Monitoring
Enhanced Business Analytics
ZATCA Compliance Implementation
🛠️ Technical Implementation
Backend Services
New API Endpoints
GET /api/v1/monitoring/health- System health metricsGET /api/v1/monitoring/performance- Application performance dataGET /api/v1/monitoring/alerts- Active system alertsGET /api/v1/analytics/financial- ZATCA-compliant financial reportingFrontend Components
/dashboard/admininterface with four comprehensive sections📊 Live Demo
The implementation includes a fully functional admin dashboard accessible at
/dashboard/adminwith:🔧 Key Technical Details
Performance Optimizations
Security & Compliance
Multi-tenant Architecture
📈 Business Impact
The implementation provides enterprise-grade analytics and monitoring capabilities while maintaining full compliance with Saudi Arabian tax regulations (ZATCA).
Fixes #25.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.