Fix code review issues: div-by-zero, duplicate logic, cron_name fallback

Agent-Logs-Url: https://github.com/whyour/qinglong/sessions/3db54913-03d2-4721-b720-8ccbf8d0f00e

Co-authored-by: whyour <22700758+whyour@users.noreply.github.com>

back/services/cron.ts

@@ -179,10 +179,11 @@ export default class CronService {
       );
 
       if (status === CrontabStatus.idle && last_running_time > 0) {
+        const cronName = (cron.name || cron.command || '').substring(0, 255);
         await CronLogModel.create(
           new CronLog({
             cron_id: id,
-            cron_name: cron.name || cron.command || '',
+            cron_name: cronName,
             start_time: last_execution_time,
             duration: last_running_time,
           }),

back/services/cronStats.ts

@@ -1,34 +1,64 @@
 import { Service, Inject } from 'typedi';
 import winston from 'winston';
-import { CrontabModel, CrontabStatus } from '../data/cron';
+import { CrontabModel } from '../data/cron';
-import { CronLogModel } from '../data/cronLog';
+import { CronLog, CronLogModel } from '../data/cronLog';
 import { Op } from 'sequelize';
 import dayjs from 'dayjs';
 
+type GroupedLog = {
+  cron_id: number;
+  cron_name: string;
+  durations: number[];
+};
+
 @Service()
 export default class CronStatsService {
   constructor(@Inject('logger') private logger: winston.Logger) {}
 
+  private groupLogsByCronId(logs: CronLog[]): Record<number, GroupedLog> {
+    const grouped: Record<number, GroupedLog> = {};
+    for (const log of logs) {
+      if (!grouped[log.cron_id]) {
+        grouped[log.cron_id] = {
+          cron_id: log.cron_id,
+          cron_name: log.cron_name,
+          durations: [],
+        };
+      }
+      grouped[log.cron_id].durations.push(log.duration);
+    }
+    return grouped;
+  }
+
+  private avgOf(nums: number[]): number {
+    if (nums.length === 0) return 0;
+    return Math.round(nums.reduce((a, b) => a + b, 0) / nums.length);
+  }
+
+  private getTodayRange() {
+    return {
+      start: dayjs().startOf('day').unix(),
+      end: dayjs().endOf('day').unix(),
+    };
+  }
+
   public async stats() {
-    const todayStart = dayjs().startOf('day').unix();
+    const { start, end } = this.getTodayRange();
-    const todayEnd = dayjs().endOf('day').unix();
 
     const [allCrons, todayLogs] = await Promise.all([
       CrontabModel.findAll({ where: {} }),
       CronLogModel.findAll({
-        where: {
+        where: { start_time: { [Op.between]: [start, end] } },
-          start_time: { [Op.between]: [todayStart, todayEnd] },
-        },
       }),
     ]);
 
     const total = allCrons.length;
-    const enabled = allCrons.filter((c) => c.isDisabled !== 1).length;
+    const enabled = allCrons.filter((c: any) => c.isDisabled !== 1).length;
-    const disabled = allCrons.filter((c) => c.isDisabled === 1).length;
+    const disabled = allCrons.filter((c: any) => c.isDisabled === 1).length;
 
     const todayCount = todayLogs.length;
     const todayTotalDuration = todayLogs.reduce(
-      (sum, l) => sum + (l.duration || 0),
+      (sum: number, l: any) => sum + (l.duration || 0),
       0,
     );
     const todayAvgDuration =
@@ -47,10 +77,7 @@ export default class CronStatsService {
 
   public async trend() {
     const days = 7;
-    const result: Array<{
+    const result: Array<{ date: string; count: number }> = [];
-      date: string;
-      count: number;
-    }> = [];
 
     for (let i = days - 1; i >= 0; i--) {
       const dayStart = dayjs().subtract(i, 'day').startOf('day').unix();
@@ -58,107 +85,53 @@ export default class CronStatsService {
       const date = dayjs().subtract(i, 'day').format('MM-DD');
 
       const logs = await CronLogModel.findAll({
-        where: {
+        where: { start_time: { [Op.between]: [dayStart, dayEnd] } },
-          start_time: { [Op.between]: [dayStart, dayEnd] },
-        },
       });
 
-      result.push({
+      result.push({ date, count: logs.length });
-        date,
-        count: logs.length,
-      });
     }
 
     return result;
   }
 
   public async topDuration(limit = 5) {
-    const todayStart = dayjs().startOf('day').unix();
+    const { start, end } = this.getTodayRange();
-    const todayEnd = dayjs().endOf('day').unix();
 
     const logs = await CronLogModel.findAll({
-      where: {
+      where: { start_time: { [Op.between]: [start, end] } },
-        start_time: { [Op.between]: [todayStart, todayEnd] },
-      },
     });
 
-    const grouped: Record<
+    const grouped = this.groupLogsByCronId(logs as any);
-      number,
-      { cron_id: number; cron_name: string; durations: number[] }
-    > = {};
 
-    for (const log of logs) {
+    return Object.values(grouped)
-      if (!grouped[log.cron_id]) {
+      .map((g) => ({
-        grouped[log.cron_id] = {
+        cron_id: g.cron_id,
-          cron_id: log.cron_id,
+        cron_name: g.cron_name,
-          cron_name: log.cron_name,
+        count: g.durations.length,
-          durations: [],
+        avgDuration: this.avgOf(g.durations),
-        };
+        maxDuration: Math.max(...g.durations),
-      }
+      }))
-      grouped[log.cron_id].durations.push(log.duration);
-    }
-
-    const result = Object.values(grouped)
-      .map((g) => {
-        const avgDuration = Math.round(
-          g.durations.reduce((a, b) => a + b, 0) / g.durations.length,
-        );
-        const maxDuration = Math.max(...g.durations);
-        return {
-          cron_id: g.cron_id,
-          cron_name: g.cron_name,
-          count: g.durations.length,
-          avgDuration,
-          maxDuration,
-        };
-      })
       .sort((a, b) => b.avgDuration - a.avgDuration)
       .slice(0, limit);
-
-    return result;
   }
 
   public async topCount(limit = 5) {
-    const todayStart = dayjs().startOf('day').unix();
+    const { start, end } = this.getTodayRange();
-    const todayEnd = dayjs().endOf('day').unix();
 
     const logs = await CronLogModel.findAll({
-      where: {
+      where: { start_time: { [Op.between]: [start, end] } },
-        start_time: { [Op.between]: [todayStart, todayEnd] },
-      },
     });
 
-    const grouped: Record<
+    const grouped = this.groupLogsByCronId(logs as any);
-      number,
-      { cron_id: number; cron_name: string; durations: number[] }
-    > = {};
 
-    for (const log of logs) {
+    return Object.values(grouped)
-      if (!grouped[log.cron_id]) {
+      .map((g) => ({
-        grouped[log.cron_id] = {
+        cron_id: g.cron_id,
-          cron_id: log.cron_id,
+        cron_name: g.cron_name,
-          cron_name: log.cron_name,
+        count: g.durations.length,
-          durations: [],
+        avgDuration: this.avgOf(g.durations),
-        };
+      }))
-      }
-      grouped[log.cron_id].durations.push(log.duration);
-    }
-
-    const result = Object.values(grouped)
-      .map((g) => {
-        const avgDuration = Math.round(
-          g.durations.reduce((a, b) => a + b, 0) / g.durations.length,
-        );
-        return {
-          cron_id: g.cron_id,
-          cron_name: g.cron_name,
-          count: g.durations.length,
-          avgDuration,
-        };
-      })
       .sort((a, b) => b.count - a.count)
       .slice(0, limit);
-
-    return result;
   }
 }

src/pages/statistics/index.tsx

@@ -73,7 +73,7 @@ const TrendChart = ({ data }: { data: TrendItem[] }) => {
   const maxCount = Math.max(...data.map((d) => d.count), 1);
 
   const points = data.map((d, i) => ({
-    x: paddingLeft + (i / (data.length - 1)) * chartWidth,
+    x: paddingLeft + (i / Math.max(data.length - 1, 1)) * chartWidth,
     y: paddingTop + chartHeight - (d.count / maxCount) * chartHeight,
     ...d,
   }));