carverauto/serviceradar

Fork 0

fixing analytics dashboard #2323

Merged

mfreeman451 merged 3 commits from refs/pull/2323/head into main

2025-10-15 16:08:16 +00:00

mfreeman451 commented

2025-10-15 15:17:19 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1774
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1774
Original created: 2025-10-15T15:17:19Z
Original updated: 2025-10-15T16:11:17Z
Original head: carverauto/serviceradar:1766-bugui-analytics-dashboard-slow-load
Original base: main
Original merged: 2025-10-15T16:08:16Z by @mfreeman451

PR Type

Enhancement, Bug fix

Description

Migrated analytics dashboard from REST API to SRQL queries for improved performance
Refactored service statistics computation from client-side to server-side aggregation
Consolidated sysmon and rperf data fetching to use SRQL-based services
Added rperf_metrics entity mapping to SRQL engine

Diagram Walkthrough

flowchart LR
  A["Client Components"] -- "SRQL queries" --> B["analyticsService"]
  A -- "SRQL queries" --> C["sysmonService"]
  A -- "SRQL queries" --> D["rperfService"]
  B --> E["SRQL Engine"]
  C --> E
  D --> E
  E --> F["timeseries_metrics"]
  E --> G["cpu_metrics"]
  E --> H["memory_metrics"]
  E --> I["disk_metrics"]

File Walkthrough

Relevant files

Enhancement

11 files

useAnalyticsData.ts `Moved service statistics computation to server-side`	+11/-33
analyticsService.ts `Refactored to compute service stats server-side with SRQL`	+202/-38
rperfService.ts `Migrated from REST API to SRQL queries`	+175/-65
sysmonService.ts `Replaced REST endpoints with SRQL-based data aggregation`	+307/-74
entity_mapping.ml Added `rperf_metrics` entity mapping to SRQL	+1/-0
Dashboard.tsx `Simplified dashboard to use pre-computed service counts`	+2/-26
HighUtilizationWidget.tsx `Updated to consume sysmon summary data structure`	+130/-131
SysmonOverviewWidget.tsx `Refactored to use sysmon context with SRQL data`	+87/-247
MultiSysmonMetrics.tsx `Migrated to sysmon service with SRQL-based summaries`	+91/-47
AnalyticsContext.tsx `Updated interface to include pre-computed service statistics`	+3/-1
SysmonContext.tsx Changed data type to use `SysmonAgentSummary`	+4/-11

Tests

1 files

test_query_engine.ml
Added tests for sysmon stats and rperf metrics +23/-0

Imported from GitHub pull request. Original GitHub pull request: #1774 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1774 Original created: 2025-10-15T15:17:19Z Original updated: 2025-10-15T16:11:17Z Original head: carverauto/serviceradar:1766-bugui-analytics-dashboard-slow-load Original base: main Original merged: 2025-10-15T16:08:16Z by @mfreeman451 --- ### **PR Type** Enhancement, Bug fix ___ ### **Description** - Migrated analytics dashboard from REST API to SRQL queries for improved performance - Refactored service statistics computation from client-side to server-side aggregation - Consolidated sysmon and rperf data fetching to use SRQL-based services - Added `rperf_metrics` entity mapping to SRQL engine ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Client Components"] -- "SRQL queries" --> B["analyticsService"] A -- "SRQL queries" --> C["sysmonService"] A -- "SRQL queries" --> D["rperfService"] B --> E["SRQL Engine"] C --> E D --> E E --> F["timeseries_metrics"] E --> G["cpu_metrics"] E --> H["memory_metrics"] E --> I["disk_metrics"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>11 files</summary><table> <tr> <td><strong>useAnalyticsData.ts</strong><dd><code>Moved service statistics computation to server-side</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-1392dcacb26b2fc76526ad0279e4fa54826acece95554fa9330779e8275f1ae5">+11/-33</a>  </td> </tr> <tr> <td><strong>analyticsService.ts</strong><dd><code>Refactored to compute service stats server-side with SRQL</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-1140b7ee45921b9100c38a5330d66377a73258a776733b422d4689dc4ce282c5">+202/-38</a></td> </tr> <tr> <td><strong>rperfService.ts</strong><dd><code>Migrated from REST API to SRQL queries</code>                                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-91b271ba4ebedfb079e4531649bf42c19fc6262e293d7b0dd2df697be0b781c1">+175/-65</a></td> </tr> <tr> <td><strong>sysmonService.ts</strong><dd><code>Replaced REST endpoints with SRQL-based data aggregation</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-7994e212b8dcc74e40e26c5c45399aeaa91f2127b135d03cf317e78a363634bb">+307/-74</a></td> </tr> <tr> <td><strong>entity_mapping.ml</strong><dd><code>Added `rperf_metrics` entity mapping to SRQL</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-b72b5ae7933263f4e466c25037cc269868634df4a2b2a27dcbcd04fe742b0003">+1/-0</a>      </td> </tr> <tr> <td><strong>Dashboard.tsx</strong><dd><code>Simplified dashboard to use pre-computed service counts</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-9b358d7a65a8625828453f8c8636f3b1077948967d32327f7357b97b257e5e31">+2/-26</a>    </td> </tr> <tr> <td><strong>HighUtilizationWidget.tsx</strong><dd><code>Updated to consume sysmon summary data structure</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-716a93a815f8f58832dcd4a0e7b1b7cbc53722bf383fad88cc7732b684c2baa7">+130/-131</a></td> </tr> <tr> <td><strong>SysmonOverviewWidget.tsx</strong><dd><code>Refactored to use sysmon context with SRQL data</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-0112a0388728f4b7c40ca6acd5abd4578dc06e502b8557d447cd7250feff43bb">+87/-247</a></td> </tr> <tr> <td><strong>MultiSysmonMetrics.tsx</strong><dd><code>Migrated to sysmon service with SRQL-based summaries</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-ec9e5c2a2b20db6877753d4c65bd8c1b3a93ab89b2580643cfd32a51e12b5675">+91/-47</a>  </td> </tr> <tr> <td><strong>AnalyticsContext.tsx</strong><dd><code>Updated interface to include pre-computed service statistics</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-8bd8a5899b5763f65b9f01fe71cfb7e19526a6f463cbd724f279328a04853517">+3/-1</a>      </td> </tr> <tr> <td><strong>SysmonContext.tsx</strong><dd><code>Changed data type to use `SysmonAgentSummary`</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-f0c6c75f2742965cb4a28448fd7ae01477f5eceb1c2f80786406a21511596cdb">+4/-11</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>test_query_engine.ml</strong><dd><code>Added tests for sysmon stats and rperf metrics</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-0c1dcb695d94269d0e3f899c01455e21cb50242b4665e3a0ef5f2b198c5cbe2f">+23/-0</a>    </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-15 15:18:05 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1774#issuecomment-3406980488
Original created: 2025-10-15T15:18:05Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟡

🎫 #1766

🟢	Fix slow loading of the analytics dashboard likely caused by 404s to deprecated sysmon REST endpoints after SRQL switch.
	Ensure dashboard widgets hydrate without being blocked by missing endpoints.
	Use SRQL queries that aggregate across all sysmon metrics globally, not tied to specific pollers/agents.
	Replace client-side computation with server-side/SRQL-based aggregation for service stats (failures, latency).
	Add necessary SRQL entity mappings to support required metrics (including rperf).
⚪	Validate in a real environment that dashboard load times improve and no 404s are produced in DevTools.
⚪	Verify that metrics correctness matches expectations across various deployments with multiple pollers/agents.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

⚪

No custom compliance provided

Follow the guide to enable custom compliance check.

Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1774#issuecomment-3406980488 Original created: 2025-10-15T15:18:05Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/1766>#1766</a></summary> <table width='100%'><tbody> <tr><td rowspan=5>🟢</td> <td>Fix slow loading of the analytics dashboard likely caused by 404s to deprecated sysmon <br>REST endpoints after SRQL switch.</td></tr> <tr><td>Ensure dashboard widgets hydrate without being blocked by missing endpoints.</td></tr> <tr><td>Use SRQL queries that aggregate across all sysmon metrics globally, not tied to specific <br>pollers/agents.</td></tr> <tr><td>Replace client-side computation with server-side/SRQL-based aggregation for service stats <br>(failures, latency).</td></tr> <tr><td>Add necessary SRQL entity mappings to support required metrics (including rperf).</td></tr> <tr><td rowspan=2>⚪</td> <td>Validate in a real environment that dashboard load times improve and no 404s are produced <br>in DevTools.</td></tr> <tr><td>Verify that metrics correctness matches expectations across various deployments with <br>multiple pollers/agents.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-10-15 15:19:43 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1774#issuecomment-3406988586
Original created: 2025-10-15T15:19:43Z

PR Code Suggestions ✨

Latest suggestions up to 0982dd8

Category	Suggestion	Impact
Incremental ^[*]	Guard state updates after unmount Add a cancellation flag to the `useEffect` hook to prevent state updates on the component after it has unmounted, avoiding potential React warnings and memory leaks. web/src/contexts/AnalyticsContext.tsx [40-79] export const AnalyticsProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const { token } = useAuth(); const [data, setData] = useState<AnalyticsData \| null>(null); const [loading, setLoading] = useState(true); const [error, setError] = useState<string \| null>(null); - const fetchData = useCallback(async () => { + const fetchData = useCallback(async (signal?: { cancelled: boolean }) => { try { console.log('[AnalyticsProvider] Fetching analytics data'); - setError(null); + if (!signal?.cancelled) setError(null); const analyticsData = await dataService.getAnalyticsData(token ?? undefined); - setData(analyticsData); + if (!signal?.cancelled) setData(analyticsData); } catch (err) { - setError(err instanceof Error ? err.message : 'Failed to fetch analytics data'); + if (!signal?.cancelled) { + setError(err instanceof Error ? err.message : 'Failed to fetch analytics data'); + } } finally { - setLoading(false); + if (!signal?.cancelled) setLoading(false); } }, [token]); useEffect(() => { - fetchData(); + let cancelled = false; + setLoading(true); + fetchData({ cancelled }); - // Set up refresh interval - every 60 seconds - const interval = setInterval(fetchData, 60000); - - // Subscribe to service updates + const interval = setInterval(() => !cancelled && fetchData({ cancelled }), 60000); const unsubscribe = dataService.subscribeAnalytics(() => { - fetchData(); + if (!cancelled) fetchData({ cancelled }); }); return () => { + cancelled = true; clearInterval(interval); unsubscribe(); }; }, [fetchData]); return ( - <AnalyticsContext.Provider value={{ data, loading, error, refresh: fetchData }}> + <AnalyticsContext.Provider value={{ data, loading, error, refresh: () => fetchData() }}> {children} </AnalyticsContext.Provider> ); }; `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential race condition that could lead to state updates on an unmounted component, which is a valid concern in React. The proposed fix using a cancellation flag is a standard and effective pattern to prevent this issue.	Medium
Possible issue	Prevent undefined disks crash To prevent potential runtime errors, add a nullish coalescing operator to default `summary.disks` to an empty array before iterating over it. web/src/components/Analytics/HighUtilizationWidget.tsx [180-217] -summary.disks.forEach((disk) => { +(summary.disks ?? []).forEach((disk) => { if (disk.usagePercent === undefined) { return; } if (disk.usagePercent > 85) { criticalDiskCount++; highUtilizationServices.push({ device_id: device?.device_id \|\| summary.deviceId \|\| (summary.pollerId ? `${summary.pollerId}:${hostKey}` : hostKey), hostname: device?.hostname \|\| summary.hostId \|\| summary.pollerId, ip_address: ipAddress, service_type: 'grpc', service_name: `sysmon:${disk.mountPoint}`, metric_value: disk.usagePercent, metric_type: 'disk', severity: 'critical', timestamp: disk.lastTimestamp ?? timestamp }); } else if (disk.usagePercent > 75) { warningDiskCount++; highUtilizationServices.push({ device_id: device?.device_id \|\| summary.deviceId \|\| (summary.pollerId ? `${summary.pollerId}:${hostKey}` : hostKey), hostname: device?.hostname \|\| summary.hostId \|\| summary.pollerId, ip_address: ipAddress, service_type: 'grpc', service_name: `sysmon:${disk.mountPoint}`, metric_value: disk.usagePercent, metric_type: 'disk', severity: 'warning', timestamp: disk.lastTimestamp ?? timestamp }); } }); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential runtime error if `summary.disks` is `undefined`, and the proposed nullish coalescing operator (`?? []`) is an effective safeguard, improving code robustness.	Medium
General	Default device counts safely Add fallback values for `totalDevices` and `offlineDevices` to prevent rendering `undefined` in case of partial data from the backend. web/src/components/Analytics/Dashboard.tsx [92-106] const stats = useMemo(() => { if (!analyticsData) { return { totalDevices: 0, offlineDevices: 0, highLatencyServices: 0, failingServices: 0 }; } const failingServices = analyticsData.failingServiceCount ?? 0; const highLatencyServices = analyticsData.highLatencyServiceCount ?? 0; return { - totalDevices: analyticsData.totalDevices, - offlineDevices: analyticsData.offlineDevices, + totalDevices: analyticsData.totalDevices ?? 0, + offlineDevices: analyticsData.offlineDevices ?? 0, highLatencyServices, failingServices, }; }, [analyticsData]); `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: This is a good defensive coding practice that ensures `totalDevices` and `offlineDevices` have a default value of `0`, preventing potential `undefined` values and improving the component's resilience to partial API responses.	Medium
More

Previous suggestions

Suggestions up to commit 2e378c5

Category	Suggestion	Impact
High-level	Consolidate data fetching into single service Consolidate the newly introduced `analyticsService`, `sysmonService`, and `rperfService` into a single data service. This change would centralize shared logic for caching, error handling, and SRQL queries, reducing code duplication. Examples: web/src/services/sysmonService.ts [89-124] `class SysmonService { private cache: CachedSysmonData \| null = null; private readonly CACHE_DURATION = 30_000; // 30 seconds cache private readonly subscribers: Set<() => void> = new Set(); async getSysmonData(token?: string): Promise<SysmonAgentSummary[]> { const now = Date.now(); if (this.cache && now - this.cache.timestamp < this.CACHE_DURATION) { return this.cache.data; ... (clipped 26 lines)` web/src/services/rperfService.ts [67-110] `class RperfService { private cache: CachedRperfData \| null = null; private readonly CACHE_DURATION = 30000; // 30 seconds cache private readonly subscribers: Set<() => void> = new Set(); async getRperfData(token?: string): Promise<RperfData[]> { const now = Date.now(); if (this.cache && now - this.cache.timestamp < this.CACHE_DURATION) { return this.cache.data; ... (clipped 34 lines)` Solution Walkthrough: Before: // rperfService.ts class RperfService { private cache: CachedRperfData \| null = null; async getRperfData(token?: string): Promise<RperfData[]> { // ... caching logic ... const promise = this.fetchRperfData(token); // ... } private async fetchRperfData(token?: string): Promise<RperfData[]> { const rows = await this.executeSrqlQuery(...); // ... } private async executeSrqlQuery(...) { /* fetch /api/query / } subscribe(...) { / ... / } } // sysmonService.ts has a similar structure // analyticsService.ts has a similar structure After: // DataService.ts class DataService { private cache = new Map<string, CachedData>(); private subscribers = new Set<() => void>(); private async executeSrqlQuery(...) { / fetch /api/query */ } private async fetchAndCache(key: string, fetcher: () => Promise<any>, token?: string) { // ... centralized caching logic ... } async getAnalyticsData(token?: string) { return this.fetchAndCache('analytics', () => this.fetchAnalytics(token)); } async getSysmonData(token?: string) { return this.fetchAndCache('sysmon', () => this.fetchSysmonSummaries(token)); } async getRperfData(token?: string) { return this.fetchAndCache('rperf', () => this.fetchRperf(token)); } // ... other private fetchers using executeSrqlQuery } Suggestion importance[1-10]: 8 __ Why: This is a significant architectural suggestion that correctly identifies substantial code duplication for caching, subscriptions, and SRQL execution logic across the new `analyticsService`, `rperfService`, and `sysmonService`.	Medium
General	Use helper for consistent data extraction Refactor `totalDevices` and `offlineDevices` extraction to use the `this.extractTotal` helper method for consistency and robustness. web/src/services/analyticsService.ts [219-264] -const totalDevices = Array.isArray(totalDevicesRes?.results) ? totalDevicesRes.results[0]?.total \|\| 0 : 0; -const offlineDevices = Array.isArray(offlineDevicesRes?.results) ? offlineDevicesRes.results[0]?.total \|\| 0 : 0; +const totalDevices = this.extractTotal(totalDevicesRes); +const offlineDevices = this.extractTotal(offlineDevicesRes); return { // Device stats totalDevices, offlineDevices, onlineDevices: Math.max(totalDevices - offlineDevices, 0), // Event stats totalEvents: this.extractTotal(totalEventsRes), criticalEvents: this.extractTotal(criticalEventsRes), highEvents: this.extractTotal(highEventsRes), mediumEvents: this.extractTotal(mediumEventsRes), lowEvents: this.extractTotal(lowEventsRes), recentCriticalEvents: this.sliceResults(recentCriticalEventsRes, 5), // Log stats totalLogs: this.extractTotal(totalLogsRes), fatalLogs: this.extractTotal(fatalLogsRes), errorLogs: this.extractTotal(errorLogsRes), warningLogs: this.extractTotal(warningLogsRes), infoLogs: this.extractTotal(infoLogsRes), debugLogs: this.extractTotal(debugLogsRes), recentErrorLogs: this.sliceResults(recentErrorLogsRes, 5), // Observability stats totalMetrics: this.extractTotal(totalMetricsRes), totalTraces: this.extractTotal(totalTracesRes), slowTraces: this.extractTotal(slowTracesRes), errorTraces: this.extractTotal(errorTracesRes), recentSlowSpans: this.sliceResults<SlowTraceResult>(slowTraceListRes, 5).map((trace): RecentSlowSpan => ({ trace_id: trace.trace_id ?? 'unknown_trace', service_name: trace.root_service_name \|\| trace.service_name \|\| 'Unknown Service', span_name: trace.root_span_name \|\| 'Root Span', duration_ms: trace.duration_ms \|\| 0, timestamp: trace.timestamp \|\| trace.start_time_unix_nano \|\| null, })), // Raw data for widgets devicesLatest: Array.isArray(devicesLatestRes?.results) ? devicesLatestRes.results : [], servicesLatest, failingServiceCount: failingCount, highLatencyServiceCount: highLatencyCount, serviceLatencyBuckets: latencyBuckets }; Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out an inconsistency and improves code maintainability by refactoring to use the existing `this.extractTotal` helper method.	Low
General	Remove redundant client-side sorting Remove the redundant client-side sorting of `rperf` metrics, as the data is already sorted by the initial database query. web/src/services/rperfService.ts [112-148] private async fetchRperfData(token?: string): Promise<RperfData[]> { const query = 'in:rperf_metrics time:last_2h sort:timestamp:asc limit:2000'; const rows = await this.executeSrqlQuery<SrqlRperfRow>(query, token); if (!rows.length) { return []; } const grouped = new Map<string, RperfMetric[]>(); const dedupe = new Set<string>(); rows.forEach((row) => { const parsed = this.rowToMetric(row); if (!parsed) { return; } const { pollerId, metric } = parsed; const key = `${pollerId}\|${metric.target}\|${metric.timestamp}\|${metric.bits_per_second}\|${metric.bytes_sent}\|${metric.bytes_received}`; if (dedupe.has(key)) { return; } dedupe.add(key); const entry = grouped.get(pollerId); if (entry) { entry.push(metric); } else { grouped.set(pollerId, [metric]); } }); - return Array.from(grouped.entries()).map(([pollerId, metrics]) => { - metrics.sort((a, b) => new Date(a.timestamp).getTime() - new Date(b.timestamp).getTime()); - return { pollerId, rperfMetrics: metrics }; - }); + return Array.from(grouped.entries()).map(([pollerId, rperfMetrics]) => ({ + pollerId, + rperfMetrics + })); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a redundant client-side sort and proposes its removal, which is a valid optimization, although with minor performance impact.	Low

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1774#issuecomment-3406988586 Original created: 2025-10-15T15:19:43Z --- ## PR Code Suggestions ✨  Latest suggestions up to 0982dd8 <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Incremental <sup><a href='https://qodo-merge-docs.qodo.ai/core-abilities/incremental_update/'>[*]</a></sup></td> <td> <details><summary>Guard state updates after unmount</summary> ___ **Add a cancellation flag to the <code>useEffect</code> hook to prevent state updates on the <br>component after it has unmounted, avoiding potential React warnings and memory <br>leaks.** [web/src/contexts/AnalyticsContext.tsx [40-79]](https://github.com/carverauto/serviceradar/pull/1774/files#diff-8bd8a5899b5763f65b9f01fe71cfb7e19526a6f463cbd724f279328a04853517R40-R79) ```diff export const AnalyticsProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const { token } = useAuth(); const [data, setData] = useState<AnalyticsData | null>(null); const [loading, setLoading] = useState(true); const [error, setError] = useState<string | null>(null); - const fetchData = useCallback(async () => { + const fetchData = useCallback(async (signal?: { cancelled: boolean }) => { try { console.log('[AnalyticsProvider] Fetching analytics data'); - setError(null); + if (!signal?.cancelled) setError(null); const analyticsData = await dataService.getAnalyticsData(token ?? undefined); - setData(analyticsData); + if (!signal?.cancelled) setData(analyticsData); } catch (err) { - setError(err instanceof Error ? err.message : 'Failed to fetch analytics data'); + if (!signal?.cancelled) { + setError(err instanceof Error ? err.message : 'Failed to fetch analytics data'); + } } finally { - setLoading(false); + if (!signal?.cancelled) setLoading(false); } }, [token]); useEffect(() => { - fetchData(); + let cancelled = false; + setLoading(true); + fetchData({ cancelled }); - // Set up refresh interval - every 60 seconds - const interval = setInterval(fetchData, 60000); - - // Subscribe to service updates + const interval = setInterval(() => !cancelled && fetchData({ cancelled }), 60000); const unsubscribe = dataService.subscribeAnalytics(() => { - fetchData(); + if (!cancelled) fetchData({ cancelled }); }); return () => { + cancelled = true; clearInterval(interval); unsubscribe(); }; }, [fetchData]); return ( - <AnalyticsContext.Provider value={{ data, loading, error, refresh: fetchData }}> + <AnalyticsContext.Provider value={{ data, loading, error, refresh: () => fetchData() }}> {children} </AnalyticsContext.Provider> ); }; ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential race condition that could lead to state updates on an unmounted component, which is a valid concern in React. The proposed fix using a cancellation flag is a standard and effective pattern to prevent this issue. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Prevent undefined disks crash</summary> ___ **To prevent potential runtime errors, add a nullish coalescing operator to <br>default <code>summary.disks</code> to an empty array before iterating over it.** [web/src/components/Analytics/HighUtilizationWidget.tsx [180-217]](https://github.com/carverauto/serviceradar/pull/1774/files#diff-716a93a815f8f58832dcd4a0e7b1b7cbc53722bf383fad88cc7732b684c2baa7R180-R217) ```diff -summary.disks.forEach((disk) => { +(summary.disks ?? []).forEach((disk) => { if (disk.usagePercent === undefined) { return; } if (disk.usagePercent > 85) { criticalDiskCount++; highUtilizationServices.push({ device_id: device?.device_id || summary.deviceId || (summary.pollerId ? `${summary.pollerId}:${hostKey}` : hostKey), hostname: device?.hostname || summary.hostId || summary.pollerId, ip_address: ipAddress, service_type: 'grpc', service_name: `sysmon:${disk.mountPoint}`, metric_value: disk.usagePercent, metric_type: 'disk', severity: 'critical', timestamp: disk.lastTimestamp ?? timestamp }); } else if (disk.usagePercent > 75) { warningDiskCount++; highUtilizationServices.push({ device_id: device?.device_id || summary.deviceId || (summary.pollerId ? `${summary.pollerId}:${hostKey}` : hostKey), hostname: device?.hostname || summary.hostId || summary.pollerId, ip_address: ipAddress, service_type: 'grpc', service_name: `sysmon:${disk.mountPoint}`, metric_value: disk.usagePercent, metric_type: 'disk', severity: 'warning', timestamp: disk.lastTimestamp ?? timestamp }); } }); ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential runtime error if `summary.disks` is `undefined`, and the proposed nullish coalescing operator (`?? []`) is an effective safeguard, improving code robustness. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Default device counts safely</summary> ___ **Add fallback values for <code>totalDevices</code> and <code>offlineDevices</code> to prevent rendering <br><code>undefined</code> in case of partial data from the backend.** [web/src/components/Analytics/Dashboard.tsx [92-106]](https://github.com/carverauto/serviceradar/pull/1774/files#diff-9b358d7a65a8625828453f8c8636f3b1077948967d32327f7357b97b257e5e31R92-R106) ```diff const stats = useMemo(() => { if (!analyticsData) { return { totalDevices: 0, offlineDevices: 0, highLatencyServices: 0, failingServices: 0 }; } const failingServices = analyticsData.failingServiceCount ?? 0; const highLatencyServices = analyticsData.highLatencyServiceCount ?? 0; return { - totalDevices: analyticsData.totalDevices, - offlineDevices: analyticsData.offlineDevices, + totalDevices: analyticsData.totalDevices ?? 0, + offlineDevices: analyticsData.offlineDevices ?? 0, highLatencyServices, failingServices, }; }, [analyticsData]); ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This is a good defensive coding practice that ensures `totalDevices` and `offlineDevices` have a default value of `0`, preventing potential `undefined` values and improving the component's resilience to partial API responses. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table> ___ #### Previous suggestions <details><summary>Suggestions up to commit 2e378c5</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Consolidate data fetching into single service</summary> ___ **Consolidate the newly introduced <code>analyticsService</code>, <code>sysmonService</code>, and <br><code>rperfService</code> into a single data service. This change would centralize shared <br>logic for caching, error handling, and SRQL queries, reducing code duplication.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-7994e212b8dcc74e40e26c5c45399aeaa91f2127b135d03cf317e78a363634bbR89-R124">web/src/services/sysmonService.ts [89-124]</a> </summary> ```typescript class SysmonService { private cache: CachedSysmonData | null = null; private readonly CACHE_DURATION = 30_000; // 30 seconds cache private readonly subscribers: Set<() => void> = new Set(); async getSysmonData(token?: string): Promise<SysmonAgentSummary[]> { const now = Date.now(); if (this.cache && now - this.cache.timestamp < this.CACHE_DURATION) { return this.cache.data; ... (clipped 26 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1774/files#diff-91b271ba4ebedfb079e4531649bf42c19fc6262e293d7b0dd2df697be0b781c1R67-R110">web/src/services/rperfService.ts [67-110]</a> </summary> ```typescript class RperfService { private cache: CachedRperfData | null = null; private readonly CACHE_DURATION = 30000; // 30 seconds cache private readonly subscribers: Set<() => void> = new Set(); async getRperfData(token?: string): Promise<RperfData[]> { const now = Date.now(); if (this.cache && now - this.cache.timestamp < this.CACHE_DURATION) { return this.cache.data; ... (clipped 34 lines) ``` </details> ### Solution Walkthrough: #### Before: ```typescript // rperfService.ts class RperfService { private cache: CachedRperfData | null = null; async getRperfData(token?: string): Promise<RperfData[]> { // ... caching logic ... const promise = this.fetchRperfData(token); // ... } private async fetchRperfData(token?: string): Promise<RperfData[]> { const rows = await this.executeSrqlQuery(...); // ... } private async executeSrqlQuery(...) { /* fetch /api/query */ } subscribe(...) { /* ... */ } } // sysmonService.ts has a similar structure // analyticsService.ts has a similar structure ``` #### After: ```typescript // DataService.ts class DataService { private cache = new Map<string, CachedData>(); private subscribers = new Set<() => void>(); private async executeSrqlQuery(...) { /* fetch /api/query */ } private async fetchAndCache(key: string, fetcher: () => Promise<any>, token?: string) { // ... centralized caching logic ... } async getAnalyticsData(token?: string) { return this.fetchAndCache('analytics', () => this.fetchAnalytics(token)); } async getSysmonData(token?: string) { return this.fetchAndCache('sysmon', () => this.fetchSysmonSummaries(token)); } async getRperfData(token?: string) { return this.fetchAndCache('rperf', () => this.fetchRperf(token)); } // ... other private fetchers using executeSrqlQuery } ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This is a significant architectural suggestion that correctly identifies substantial code duplication for caching, subscriptions, and SRQL execution logic across the new `analyticsService`, `rperfService`, and `sysmonService`. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Use helper for consistent data extraction</summary> ___ **Refactor <code>totalDevices</code> and <code>offlineDevices</code> extraction to use the <code>this.extractTotal</code> <br>helper method for consistency and robustness.** [web/src/services/analyticsService.ts [219-264]](https://github.com/carverauto/serviceradar/pull/1774/files#diff-1140b7ee45921b9100c38a5330d66377a73258a776733b422d4689dc4ce282c5R219-R264) ```diff -const totalDevices = Array.isArray(totalDevicesRes?.results) ? totalDevicesRes.results[0]?.total || 0 : 0; -const offlineDevices = Array.isArray(offlineDevicesRes?.results) ? offlineDevicesRes.results[0]?.total || 0 : 0; +const totalDevices = this.extractTotal(totalDevicesRes); +const offlineDevices = this.extractTotal(offlineDevicesRes); return { // Device stats totalDevices, offlineDevices, onlineDevices: Math.max(totalDevices - offlineDevices, 0), // Event stats totalEvents: this.extractTotal(totalEventsRes), criticalEvents: this.extractTotal(criticalEventsRes), highEvents: this.extractTotal(highEventsRes), mediumEvents: this.extractTotal(mediumEventsRes), lowEvents: this.extractTotal(lowEventsRes), recentCriticalEvents: this.sliceResults(recentCriticalEventsRes, 5), // Log stats totalLogs: this.extractTotal(totalLogsRes), fatalLogs: this.extractTotal(fatalLogsRes), errorLogs: this.extractTotal(errorLogsRes), warningLogs: this.extractTotal(warningLogsRes), infoLogs: this.extractTotal(infoLogsRes), debugLogs: this.extractTotal(debugLogsRes), recentErrorLogs: this.sliceResults(recentErrorLogsRes, 5), // Observability stats totalMetrics: this.extractTotal(totalMetricsRes), totalTraces: this.extractTotal(totalTracesRes), slowTraces: this.extractTotal(slowTracesRes), errorTraces: this.extractTotal(errorTracesRes), recentSlowSpans: this.sliceResults<SlowTraceResult>(slowTraceListRes, 5).map((trace): RecentSlowSpan => ({ trace_id: trace.trace_id ?? 'unknown_trace', service_name: trace.root_service_name || trace.service_name || 'Unknown Service', span_name: trace.root_span_name || 'Root Span', duration_ms: trace.duration_ms || 0, timestamp: trace.timestamp || trace.start_time_unix_nano || null, })), // Raw data for widgets devicesLatest: Array.isArray(devicesLatestRes?.results) ? devicesLatestRes.results : [], servicesLatest, failingServiceCount: failingCount, highLatencyServiceCount: highLatencyCount, serviceLatencyBuckets: latencyBuckets }; ```  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly points out an inconsistency and improves code maintainability by refactoring to use the existing `this.extractTotal` helper method. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Remove redundant client-side sorting</summary> ___ **Remove the redundant client-side sorting of <code>rperf</code> metrics, as the data is <br>already sorted by the initial database query.** [web/src/services/rperfService.ts [112-148]](https://github.com/carverauto/serviceradar/pull/1774/files#diff-91b271ba4ebedfb079e4531649bf42c19fc6262e293d7b0dd2df697be0b781c1R112-R148) ```diff private async fetchRperfData(token?: string): Promise<RperfData[]> { const query = 'in:rperf_metrics time:last_2h sort:timestamp:asc limit:2000'; const rows = await this.executeSrqlQuery<SrqlRperfRow>(query, token); if (!rows.length) { return []; } const grouped = new Map<string, RperfMetric[]>(); const dedupe = new Set<string>(); rows.forEach((row) => { const parsed = this.rowToMetric(row); if (!parsed) { return; } const { pollerId, metric } = parsed; const key = `${pollerId}|${metric.target}|${metric.timestamp}|${metric.bits_per_second}|${metric.bytes_sent}|${metric.bytes_received}`; if (dedupe.has(key)) { return; } dedupe.add(key); const entry = grouped.get(pollerId); if (entry) { entry.push(metric); } else { grouped.set(pollerId, [metric]); } }); - return Array.from(grouped.entries()).map(([pollerId, metrics]) => { - metrics.sort((a, b) => new Date(a.timestamp).getTime() - new Date(b.timestamp).getTime()); - return { pollerId, rperfMetrics: metrics }; - }); + return Array.from(grouped.entries()).map(([pollerId, rperfMetrics]) => ({ + pollerId, + rperfMetrics + })); } ```  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly identifies a redundant client-side sort and proposes its removal, which is a valid optimization, although with minor performance impact. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2">  </td><td></td></tr></tbody></table> </details>