fix: address Globalping code review findings
## Changes ### Critical fixes - Fix Switch `isChecked` → `checked` prop in CreateMonitor Globalping toggle - Merge double `getDBSettings()` calls in globalpingService into single `getClientAndLocations()` method to eliminate redundant DB reads per check cycle - Add in-flight lock (Set) to prevent overlapping Globalping checks for the same monitor - Add hostname validation before calling Globalping API to prevent invalid targets ### Dead code removal - Remove unused `locations` field from server Monitor type, Mongoose model, Joi validation, and repository mappers - Remove unused `locations` field from client Monitor type - Remove dead `getLimits()` method from globalpingService - Remove dead `locations` translation keys from en.json - Remove dead `monitorType`, `groupedUpChecks`, `groupedDownChecks` fields from findLocationChecksByMonitorId return value ### Medium/low fixes - Make globalping/locations API fetch conditional on `monitor.globalpingEnabled` in Details page to avoid unnecessary requests - Add useEffect to reset selectedLocationTab when location keys change to prevent out-of-bounds tab index - Add `globalpingEnabled: false` to default case in useMonitorForm - Replace console.log with comment in CreateMonitor onError handler - Add aria-label to Globe icon for accessibility - Add warning log when Globalping buildCheck returns undefined - Fix step numbering in SuperSimpleQueueHelper (was duplicated Step 4) ## Benefits - Eliminates one DB round-trip per monitor check cycle - Prevents race conditions with overlapping Globalping checks - Validates hostnames before external API calls - Removes ~30 lines of dead code across server and client - Improves accessibility with proper aria labels
This commit is contained in:
1 parent
bfdf28a2cc
commit
b7c9e2710a
13 files changed
+72
-73
No files matched your search
@@ -102,6 +102,7 @@ export const useMonitorForm = ({
|
||||
matchMethod: "",
|
||||
expectedValue: "",
|
||||
jsonPath: "",
|
||||
globalpingEnabled: false,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -222,8 +222,8 @@ const CreateMonitorPage = () => {
|
||||
}
|
||||
};
|
||||
|
||||
const onError = (errors: unknown) => {
|
||||
console.log(errors);
|
||||
const onError = (_errors: unknown) => {
|
||||
// Validation errors are shown inline via react-hook-form field errors
|
||||
};
|
||||
|
||||
return (
|
||||
@@ -500,7 +500,7 @@ const CreateMonitorPage = () => {
|
||||
render={({ field }) => (
|
||||
<Switch
|
||||
id="globalpingEnabled"
|
||||
isChecked={field.value ?? false}
|
||||
checked={field.value ?? false}
|
||||
onChange={(e) => field.onChange(e.target.checked)}
|
||||
/>
|
||||
)}
|
||||
|
||||
@@ -17,7 +17,7 @@ import { MonitorStatBoxes } from "@/Components/v2/monitors";
|
||||
|
||||
import { useTheme } from "@mui/material/styles";
|
||||
import { useIsAdmin } from "@/Hooks/useIsAdmin";
|
||||
import { useState, useMemo } from "react";
|
||||
import { useState, useMemo, useEffect } from "react";
|
||||
import { useParams } from "react-router-dom";
|
||||
import { useSelector } from "react-redux";
|
||||
import { useGet } from "@/Hooks/UseApi";
|
||||
@@ -113,10 +113,11 @@ const UptimeDetailsPage = () => {
|
||||
const checks = checksData?.checks ?? [];
|
||||
const checksCount = checksData?.checksCount ?? 0;
|
||||
|
||||
const globalpingLocationsUrl = monitor?.globalpingEnabled ? "/monitors/globalping/locations" : null;
|
||||
const { data: globalpingData } = useGet<{
|
||||
locations: { id: string; label: string }[];
|
||||
labels: Record<string, string>;
|
||||
}>("/monitors/globalping/locations");
|
||||
}>(globalpingLocationsUrl);
|
||||
const locationLabels = globalpingData?.labels ?? {};
|
||||
|
||||
const locationChecks = monitorData?.locationChecks;
|
||||
@@ -124,6 +125,12 @@ const UptimeDetailsPage = () => {
|
||||
() => (locationChecks ? Object.keys(locationChecks).sort() : []),
|
||||
[locationChecks]
|
||||
);
|
||||
|
||||
// Reset tab index when location keys change to prevent out-of-bounds
|
||||
useEffect(() => {
|
||||
setSelectedLocationTab(0);
|
||||
}, [locationKeys.length]);
|
||||
|
||||
const selectedLocation = locationKeys[selectedLocationTab] ?? null;
|
||||
const selectedLocationData: LocationCheckData | null =
|
||||
selectedLocation && locationChecks ? locationChecks[selectedLocation] : null;
|
||||
|
||||
@@ -177,7 +177,7 @@ export const MonitorTable = ({
|
||||
{row?.name}
|
||||
{row.globalpingEnabled && (
|
||||
<Tooltip title={t("pages.uptime.table.headers.globalpingEnabled")}>
|
||||
<Globe size={14} />
|
||||
<Globe size={14} aria-label={t("pages.uptime.table.headers.globalpingEnabled")} />
|
||||
</Tooltip>
|
||||
)}
|
||||
</Stack>
|
||||
|
||||
@@ -58,7 +58,6 @@ export interface Monitor {
|
||||
selectedDisks: string[];
|
||||
gameId?: string;
|
||||
group: string | null;
|
||||
locations: string[];
|
||||
globalpingEnabled: boolean;
|
||||
recentChecks: CheckSnapshot[];
|
||||
createdAt: string;
|
||||
|
||||
@@ -496,12 +496,6 @@
|
||||
"hardware": "Monitor CPU, memory, disk usage, and temperature for your infrastructure."
|
||||
}
|
||||
},
|
||||
"locations": {
|
||||
"title": "Global locations",
|
||||
"description": "Run uptime checks from multiple geographic regions using Globalping. Select up to 5 locations.",
|
||||
"label": "Check locations",
|
||||
"placeholder": "Select locations"
|
||||
},
|
||||
"ignoreTls": {
|
||||
"description": "Configure TLS/SSL certificate validation for HTTPS connections.",
|
||||
"option": {
|
||||
@@ -1158,7 +1152,6 @@
|
||||
"table": {
|
||||
"headers": {
|
||||
"responseTime": "Response time",
|
||||
"locations": "Locations",
|
||||
"globalpingEnabled": "Global monitoring enabled"
|
||||
}
|
||||
},
|
||||
|
||||
@@ -160,10 +160,6 @@ const MonitorSchema = new Schema<MonitorDocument>(
|
||||
gameId: {
|
||||
type: String,
|
||||
},
|
||||
locations: {
|
||||
type: [String],
|
||||
default: [],
|
||||
},
|
||||
globalpingEnabled: {
|
||||
type: Boolean,
|
||||
default: false,
|
||||
|
||||
@@ -472,10 +472,7 @@ class MongoChecksRepository implements IChecksRepository {
|
||||
const uptimePercentage =
|
||||
loc.totalAllChecks > 0 ? loc.totalUpChecks / loc.totalAllChecks : 0;
|
||||
locationChecks[loc._id] = {
|
||||
monitorType: "http" as const,
|
||||
groupedChecks: loc.groupedChecks,
|
||||
groupedUpChecks: [],
|
||||
groupedDownChecks: [],
|
||||
uptimePercentage,
|
||||
avgResponseTime: loc.overallAvgResponseTime ?? 0,
|
||||
};
|
||||
|
||||
@@ -343,7 +343,6 @@ class MongoMonitorsRepository implements IMonitorsRepository {
|
||||
selectedDisks: doc.selectedDisks ?? [],
|
||||
gameId: doc.gameId ?? undefined,
|
||||
group: doc.group ?? null,
|
||||
locations: doc.locations ?? [],
|
||||
globalpingEnabled: doc.globalpingEnabled ?? false,
|
||||
recentChecks: (doc.recentChecks ?? []).map((check: any) => this.toCheckSnapshot(check)),
|
||||
createdAt: toDateString(doc.createdAt),
|
||||
@@ -400,7 +399,6 @@ class MongoMonitorsRepository implements IMonitorsRepository {
|
||||
selectedDisks: doc.selectedDisks ?? [],
|
||||
gameId: doc.gameId ?? undefined,
|
||||
group: doc.group ?? null,
|
||||
locations: doc.locations ?? [],
|
||||
globalpingEnabled: doc.globalpingEnabled ?? false,
|
||||
recentChecks: (doc.recentChecks ?? []).map((check: any) => this.toCheckSnapshot(check)),
|
||||
createdAt: toDateString(doc.createdAt),
|
||||
|
||||
@@ -136,6 +136,12 @@ class SuperSimpleQueueHelper {
|
||||
const locCheck = this.checkService.buildCheck(locStatus);
|
||||
if (locCheck) {
|
||||
this.buffer.addToBuffer(locCheck);
|
||||
} else {
|
||||
this.logger.warn({
|
||||
message: `Failed to build Globalping check for monitor ${monitorId}, location: ${locStatus.location}`,
|
||||
service: SERVICE_NAME,
|
||||
method: "getMonitorJob",
|
||||
});
|
||||
}
|
||||
}
|
||||
})
|
||||
@@ -148,7 +154,7 @@ class SuperSimpleQueueHelper {
|
||||
});
|
||||
}
|
||||
|
||||
// Step 3. Build check
|
||||
// Step 3. Build check
|
||||
const check = await this.checkService.buildCheck(status);
|
||||
if (!check) {
|
||||
this.logger.warn({
|
||||
@@ -159,15 +165,15 @@ class SuperSimpleQueueHelper {
|
||||
});
|
||||
return;
|
||||
}
|
||||
// Step 4 Add check to buffer
|
||||
// Step 4. Add check to buffer
|
||||
this.buffer.addToBuffer(check);
|
||||
// Step 4. Update monitor status
|
||||
// Step 5. Update monitor status
|
||||
const statusChangeResult = await this.statusService.updateMonitorStatus(status, check);
|
||||
|
||||
// Step 5. Get decisions
|
||||
// Step 6. Get decisions
|
||||
const decision = this.evaluateMonitorAction(statusChangeResult);
|
||||
|
||||
// Step 6. Handle notifications (best effort, continue even in event of failure, don't wait)
|
||||
// Step 7. Handle notifications (best effort, continue even in event of failure, don't wait)
|
||||
if (decision.shouldSendNotification) {
|
||||
this.notificationsService.handleNotifications(statusChangeResult.monitor, status, decision).catch((error: any) => {
|
||||
this.logger.error({
|
||||
@@ -180,7 +186,7 @@ class SuperSimpleQueueHelper {
|
||||
});
|
||||
}
|
||||
|
||||
// Step 7. Handle incidents (best effort, don't wait)
|
||||
// Step 8. Handle incidents (best effort, don't wait)
|
||||
this.incidentService.handleIncident(statusChangeResult.monitor, statusChangeResult.code, decision, status).catch((error: any) => {
|
||||
this.logger.warn({
|
||||
message: error.message,
|
||||
|
||||
@@ -5,7 +5,7 @@ import type { MonitorStatusResponse } from "@/types/network.js";
|
||||
|
||||
const SERVICE_NAME = "GlobalpingService";
|
||||
|
||||
interface GlobalpingLocationOption {
|
||||
export interface GlobalpingLocationOption {
|
||||
id: string;
|
||||
label: string;
|
||||
}
|
||||
@@ -80,6 +80,8 @@ for (const tier of Object.values(LOCATIONS_BY_TIER)) {
|
||||
}
|
||||
}
|
||||
|
||||
const HOSTNAME_REGEX = /^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*$/;
|
||||
|
||||
export const getGlobalpingLocationsByTier = (tier: number): GlobalpingLocationOption[] => {
|
||||
return LOCATIONS_BY_TIER[tier] ?? LOCATIONS_TIER_6;
|
||||
};
|
||||
@@ -91,6 +93,7 @@ class GlobalpingService {
|
||||
private settingsService: ISettingsService;
|
||||
private client: Globalping<false> | null = null;
|
||||
private lastApiKey: string | undefined;
|
||||
private inFlightChecks = new Set<string>();
|
||||
|
||||
constructor({ settingsService, logger }: { settingsService: ISettingsService; logger: any }) {
|
||||
this.settingsService = settingsService;
|
||||
@@ -101,31 +104,30 @@ class GlobalpingService {
|
||||
return GlobalpingService.SERVICE_NAME;
|
||||
}
|
||||
|
||||
private async getClient(): Promise<Globalping<false> | null> {
|
||||
/**
|
||||
* Reads settings once, returns both the SDK client and configured location IDs.
|
||||
* Returns null client if the API key is not configured.
|
||||
*/
|
||||
private async getClientAndLocations(): Promise<{
|
||||
client: Globalping<false> | null;
|
||||
locationIds: string[];
|
||||
}> {
|
||||
const dbSettings = await this.settingsService.getDBSettings();
|
||||
const apiKey = dbSettings.globalpingApiKey;
|
||||
|
||||
if (!apiKey) {
|
||||
return null;
|
||||
return { client: null, locationIds: [] };
|
||||
}
|
||||
|
||||
if (this.client && this.lastApiKey === apiKey) {
|
||||
return this.client;
|
||||
if (!this.client || this.lastApiKey !== apiKey) {
|
||||
this.client = new Globalping({ auth: apiKey, throwApiErrors: false, timeout: 30000 });
|
||||
this.lastApiKey = apiKey;
|
||||
}
|
||||
|
||||
this.client = new Globalping({ auth: apiKey, throwApiErrors: false, timeout: 30000 });
|
||||
this.lastApiKey = apiKey;
|
||||
return this.client;
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets the current tier's location IDs from settings.
|
||||
* This is the source of truth for which locations to check.
|
||||
*/
|
||||
private async getTierLocationIds(): Promise<string[]> {
|
||||
const dbSettings = await this.settingsService.getDBSettings();
|
||||
const tier = dbSettings.globalpingLocationsTier ?? 6;
|
||||
return getGlobalpingLocationsByTier(tier).map((loc) => loc.id);
|
||||
const locationIds = getGlobalpingLocationsByTier(tier).map((loc) => loc.id);
|
||||
|
||||
return { client: this.client, locationIds };
|
||||
}
|
||||
|
||||
async runChecks(monitor: Monitor): Promise<MonitorStatusResponse[]> {
|
||||
@@ -133,13 +135,22 @@ class GlobalpingService {
|
||||
return [];
|
||||
}
|
||||
|
||||
const client = await this.getClient();
|
||||
if (!client) {
|
||||
// Prevent overlapping checks for the same monitor
|
||||
if (this.inFlightChecks.has(monitor.id)) {
|
||||
return [];
|
||||
}
|
||||
|
||||
const locationIds = await this.getTierLocationIds();
|
||||
if (locationIds.length === 0) {
|
||||
this.inFlightChecks.add(monitor.id);
|
||||
try {
|
||||
return await this.executeChecks(monitor);
|
||||
} finally {
|
||||
this.inFlightChecks.delete(monitor.id);
|
||||
}
|
||||
}
|
||||
|
||||
private async executeChecks(monitor: Monitor): Promise<MonitorStatusResponse[]> {
|
||||
const { client, locationIds } = await this.getClientAndLocations();
|
||||
if (!client || locationIds.length === 0) {
|
||||
return [];
|
||||
}
|
||||
|
||||
@@ -158,6 +169,16 @@ class GlobalpingService {
|
||||
target = monitorUrl;
|
||||
}
|
||||
|
||||
if (!target || !HOSTNAME_REGEX.test(target)) {
|
||||
this.logger.warn({
|
||||
message: `Invalid target hostname for Globalping: "${target}"`,
|
||||
service: SERVICE_NAME,
|
||||
method: "executeChecks",
|
||||
monitorId: monitor.id,
|
||||
});
|
||||
return [];
|
||||
}
|
||||
|
||||
const locations = locationIds
|
||||
.map((loc) => LOCATION_MAP[loc])
|
||||
.filter(Boolean);
|
||||
@@ -184,7 +205,7 @@ class GlobalpingService {
|
||||
this.logger.warn({
|
||||
message: "Failed to create Globalping measurement",
|
||||
service: SERVICE_NAME,
|
||||
method: "runChecks",
|
||||
method: "executeChecks",
|
||||
details: JSON.stringify(createResult.data),
|
||||
});
|
||||
return [];
|
||||
@@ -197,7 +218,7 @@ class GlobalpingService {
|
||||
this.logger.warn({
|
||||
message: "Failed to await Globalping measurement",
|
||||
service: SERVICE_NAME,
|
||||
method: "runChecks",
|
||||
method: "executeChecks",
|
||||
details: JSON.stringify(awaitResult.data),
|
||||
});
|
||||
return [];
|
||||
@@ -223,6 +244,7 @@ class GlobalpingService {
|
||||
location: locationId,
|
||||
});
|
||||
} else {
|
||||
// Ping: loss < 100% is considered up (consistent with local ping checks)
|
||||
const stats = result.stats;
|
||||
results.push({
|
||||
monitorId: monitor.id,
|
||||
@@ -242,7 +264,7 @@ class GlobalpingService {
|
||||
this.logger.warn({
|
||||
message: `Globalping check failed: ${error.message}`,
|
||||
service: SERVICE_NAME,
|
||||
method: "runChecks",
|
||||
method: "executeChecks",
|
||||
stack: error.stack,
|
||||
});
|
||||
return [];
|
||||
@@ -281,23 +303,6 @@ class GlobalpingService {
|
||||
|
||||
return continent && country ? `${continent}-${country}` : continent || country || "unknown";
|
||||
}
|
||||
|
||||
async getLimits(): Promise<any | null> {
|
||||
const client = await this.getClient();
|
||||
if (!client) {
|
||||
return null;
|
||||
}
|
||||
|
||||
try {
|
||||
const result = await client.getLimits();
|
||||
if (result.ok) {
|
||||
return result.data;
|
||||
}
|
||||
return null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export { LOCATION_LABELS };
|
||||
|
||||
@@ -43,7 +43,6 @@ export interface Monitor {
|
||||
selectedDisks: string[];
|
||||
gameId?: string;
|
||||
group: string | null;
|
||||
locations: string[];
|
||||
globalpingEnabled: boolean;
|
||||
recentChecks: CheckSnapshot[];
|
||||
createdAt: string;
|
||||
|
||||
@@ -172,7 +172,6 @@ const createMonitorBodyValidation = joi.object({
|
||||
gameId: joi.string().allow(""),
|
||||
selectedDisks: joi.array().items(joi.string()).optional(),
|
||||
group: joi.string().max(50).trim().allow(null, "").optional(),
|
||||
locations: joi.array().items(joi.string()).max(5).optional(),
|
||||
globalpingEnabled: joi.boolean().optional(),
|
||||
});
|
||||
|
||||
@@ -205,7 +204,6 @@ const editMonitorBodyValidation = joi
|
||||
gameId: joi.string().allow(""),
|
||||
selectedDisks: joi.array().items(joi.string()).optional(),
|
||||
group: joi.string().max(50).trim().allow(null, "").optional(),
|
||||
locations: joi.array().items(joi.string()).max(5).optional(),
|
||||
globalpingEnabled: joi.boolean().optional(),
|
||||
})
|
||||
.options({ stripUnknown: true });
|
||||
|
||||
Reference in new issue
Block a user