mirror of
https://github.com/complexcaresolutions/cms.c2sgmbh.git
synced 2026-03-18 03:54:12 +00:00
fix(monitoring): address code review findings
- SSE stream: detect client disconnect via request.signal to stop polling loop (prevents wasted DB queries after tab close) - AlertEvaluator: split shouldFire/recordFired so cooldown is only recorded after successful dispatch (prevents alert suppression on dispatch failure) - SnapshotCollector: cache payload instance (avoid re-importing on every 60s tick) - Alert acknowledge: validate alertId type (string|number) - Logs search: add 300ms debounce to prevent query-per-keystroke - Replace remaining `any` cast with Record<string, unknown> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
96b8cca9d6
commit
086dd269fa
6 changed files with 65 additions and 30 deletions
|
|
@ -7,16 +7,16 @@ export async function POST(req: NextRequest): Promise<NextResponse> {
|
||||||
const payload = await getPayload({ config })
|
const payload = await getPayload({ config })
|
||||||
const { user } = await payload.auth({ headers: req.headers })
|
const { user } = await payload.auth({ headers: req.headers })
|
||||||
|
|
||||||
if (!user || !(user as any).isSuperAdmin) {
|
if (!user || !(user as Record<string, unknown>).isSuperAdmin) {
|
||||||
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
|
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
|
||||||
}
|
}
|
||||||
|
|
||||||
const body = await req.json()
|
const body = await req.json()
|
||||||
const { alertId } = body
|
const { alertId } = body
|
||||||
|
|
||||||
if (!alertId) {
|
if (!alertId || (typeof alertId !== 'string' && typeof alertId !== 'number')) {
|
||||||
return NextResponse.json(
|
return NextResponse.json(
|
||||||
{ error: 'alertId is required' },
|
{ error: 'alertId must be a string or number' },
|
||||||
{ status: 400 },
|
{ status: 400 },
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -45,12 +45,18 @@ export async function GET(request: NextRequest): Promise<Response> {
|
||||||
const stream = new ReadableStream({
|
const stream = new ReadableStream({
|
||||||
async start(controller) {
|
async start(controller) {
|
||||||
const startTime = Date.now()
|
const startTime = Date.now()
|
||||||
|
let cancelled = false
|
||||||
|
|
||||||
|
// Stop polling when the client disconnects
|
||||||
|
request.signal.addEventListener('abort', () => {
|
||||||
|
cancelled = true
|
||||||
|
})
|
||||||
|
|
||||||
controller.enqueue(
|
controller.enqueue(
|
||||||
encoder.encode(formatSSE('connected', { timestamp: new Date().toISOString() })),
|
encoder.encode(formatSSE('connected', { timestamp: new Date().toISOString() })),
|
||||||
)
|
)
|
||||||
|
|
||||||
while (Date.now() - startTime < MAX_DURATION_MS) {
|
while (!cancelled && Date.now() - startTime < MAX_DURATION_MS) {
|
||||||
try {
|
try {
|
||||||
const now = Date.now()
|
const now = Date.now()
|
||||||
|
|
||||||
|
|
@ -70,22 +76,21 @@ export async function GET(request: NextRequest): Promise<Response> {
|
||||||
lastLogCheck = await emitNewLogs(payload, controller, encoder, lastLogCheck)
|
lastLogCheck = await emitNewLogs(payload, controller, encoder, lastLogCheck)
|
||||||
|
|
||||||
await new Promise((resolve) => setTimeout(resolve, CYCLE_INTERVAL_MS))
|
await new Promise((resolve) => setTimeout(resolve, CYCLE_INTERVAL_MS))
|
||||||
} catch (error) {
|
} catch {
|
||||||
console.error('[MonitoringSSE] Error:', error)
|
// Stream closed or other error — stop the loop
|
||||||
controller.enqueue(
|
break
|
||||||
encoder.encode(
|
|
||||||
formatSSE('error', {
|
|
||||||
message: 'Internal error',
|
|
||||||
timestamp: new Date().toISOString(),
|
|
||||||
}),
|
|
||||||
),
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
controller.enqueue(
|
if (!cancelled) {
|
||||||
encoder.encode(formatSSE('reconnect', { timestamp: new Date().toISOString() })),
|
try {
|
||||||
)
|
controller.enqueue(
|
||||||
|
encoder.encode(formatSSE('reconnect', { timestamp: new Date().toISOString() })),
|
||||||
|
)
|
||||||
|
} catch {
|
||||||
|
// Client already disconnected
|
||||||
|
}
|
||||||
|
}
|
||||||
controller.close()
|
controller.close()
|
||||||
},
|
},
|
||||||
})
|
})
|
||||||
|
|
|
||||||
|
|
@ -534,9 +534,16 @@ function LogsTab({ newLogs }: LogsTabProps): React.ReactElement {
|
||||||
const [totalPages, setTotalPages] = useState(1)
|
const [totalPages, setTotalPages] = useState(1)
|
||||||
const [level, setLevel] = useState('')
|
const [level, setLevel] = useState('')
|
||||||
const [source, setSource] = useState('')
|
const [source, setSource] = useState('')
|
||||||
|
const [searchInput, setSearchInput] = useState('')
|
||||||
const [search, setSearch] = useState('')
|
const [search, setSearch] = useState('')
|
||||||
const [expanded, setExpanded] = useState<Set<string>>(new Set())
|
const [expanded, setExpanded] = useState<Set<string>>(new Set())
|
||||||
|
|
||||||
|
// Debounce search input to avoid querying on every keystroke
|
||||||
|
useEffect(() => {
|
||||||
|
const timer = setTimeout(() => setSearch(searchInput), 300)
|
||||||
|
return () => clearTimeout(timer)
|
||||||
|
}, [searchInput])
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const params = new URLSearchParams({ page: String(page), limit: '50' })
|
const params = new URLSearchParams({ page: String(page), limit: '50' })
|
||||||
if (level) params.set('level', level)
|
if (level) params.set('level', level)
|
||||||
|
|
@ -589,8 +596,8 @@ function LogsTab({ newLogs }: LogsTabProps): React.ReactElement {
|
||||||
</select>
|
</select>
|
||||||
<input
|
<input
|
||||||
type="text"
|
type="text"
|
||||||
value={search}
|
value={searchInput}
|
||||||
onChange={(e) => setSearch(e.target.value)}
|
onChange={(e) => setSearchInput(e.target.value)}
|
||||||
placeholder="Suche..."
|
placeholder="Suche..."
|
||||||
className="monitoring__input"
|
className="monitoring__input"
|
||||||
/>
|
/>
|
||||||
|
|
|
||||||
|
|
@ -93,7 +93,6 @@ export class AlertEvaluator {
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Returns true if the rule should fire (not in cooldown).
|
* Returns true if the rule should fire (not in cooldown).
|
||||||
* Records the current time as last-fired when returning true.
|
|
||||||
*/
|
*/
|
||||||
shouldFire(ruleId: string, cooldownMinutes: number): boolean {
|
shouldFire(ruleId: string, cooldownMinutes: number): boolean {
|
||||||
const lastFired = this.cooldownMap.get(ruleId)
|
const lastFired = this.cooldownMap.get(ruleId)
|
||||||
|
|
@ -101,10 +100,14 @@ export class AlertEvaluator {
|
||||||
const elapsedMinutes = (Date.now() - lastFired) / 60_000
|
const elapsedMinutes = (Date.now() - lastFired) / 60_000
|
||||||
if (elapsedMinutes < cooldownMinutes) return false
|
if (elapsedMinutes < cooldownMinutes) return false
|
||||||
}
|
}
|
||||||
this.cooldownMap.set(ruleId, Date.now())
|
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Record that a rule fired successfully. */
|
||||||
|
recordFired(ruleId: string): void {
|
||||||
|
this.cooldownMap.set(ruleId, Date.now())
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Evaluates all enabled rules against current metrics.
|
* Evaluates all enabled rules against current metrics.
|
||||||
* Fires alerts for rules that match and are not in cooldown.
|
* Fires alerts for rules that match and are not in cooldown.
|
||||||
|
|
@ -131,6 +134,7 @@ export class AlertEvaluator {
|
||||||
if (evaluateCondition(rule.condition, value, rule.threshold)) {
|
if (evaluateCondition(rule.condition, value, rule.threshold)) {
|
||||||
if (this.shouldFire(rule.id, rule.cooldownMinutes)) {
|
if (this.shouldFire(rule.id, rule.cooldownMinutes)) {
|
||||||
await this.dispatchAlert(payload, rule, value)
|
await this.dispatchAlert(payload, rule, value)
|
||||||
|
this.recordFired(rule.id)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -12,6 +12,17 @@ import { AlertEvaluator } from './alert-evaluator.js'
|
||||||
let interval: ReturnType<typeof setInterval> | null = null
|
let interval: ReturnType<typeof setInterval> | null = null
|
||||||
const alertEvaluator = new AlertEvaluator()
|
const alertEvaluator = new AlertEvaluator()
|
||||||
|
|
||||||
|
/** Cached Payload instance — resolved once, reused on every tick. */
|
||||||
|
let cachedPayload: { create: (...args: unknown[]) => Promise<unknown>; find: (...args: unknown[]) => Promise<unknown> } | null = null
|
||||||
|
|
||||||
|
async function getPayloadInstance() {
|
||||||
|
if (cachedPayload) return cachedPayload
|
||||||
|
const { getPayload } = await import('payload')
|
||||||
|
const config = (await import(/* @vite-ignore */ '@payload-config')).default
|
||||||
|
cachedPayload = await getPayload({ config })
|
||||||
|
return cachedPayload
|
||||||
|
}
|
||||||
|
|
||||||
export async function startSnapshotCollector(): Promise<void> {
|
export async function startSnapshotCollector(): Promise<void> {
|
||||||
const INTERVAL = parseInt(process.env.MONITORING_SNAPSHOT_INTERVAL || '60000', 10)
|
const INTERVAL = parseInt(process.env.MONITORING_SNAPSHOT_INTERVAL || '60000', 10)
|
||||||
console.log(`[SnapshotCollector] Starting (interval: ${INTERVAL}ms)`)
|
console.log(`[SnapshotCollector] Starting (interval: ${INTERVAL}ms)`)
|
||||||
|
|
@ -26,13 +37,11 @@ export async function startSnapshotCollector(): Promise<void> {
|
||||||
|
|
||||||
async function collectAndSave(): Promise<void> {
|
async function collectAndSave(): Promise<void> {
|
||||||
try {
|
try {
|
||||||
const { getPayload } = await import('payload')
|
const payload = await getPayloadInstance()
|
||||||
const config = (await import('@payload-config')).default
|
|
||||||
const payload = await getPayload({ config })
|
|
||||||
|
|
||||||
const metrics = await collectMetrics()
|
const metrics = await collectMetrics()
|
||||||
|
|
||||||
await payload.create({
|
await (payload as any).create({
|
||||||
collection: 'monitoring-snapshots',
|
collection: 'monitoring-snapshots',
|
||||||
data: {
|
data: {
|
||||||
timestamp: new Date().toISOString(),
|
timestamp: new Date().toISOString(),
|
||||||
|
|
@ -41,9 +50,11 @@ async function collectAndSave(): Promise<void> {
|
||||||
})
|
})
|
||||||
|
|
||||||
// Evaluate alert rules against collected metrics
|
// Evaluate alert rules against collected metrics
|
||||||
await alertEvaluator.evaluateRules(payload, metrics)
|
await alertEvaluator.evaluateRules(payload as any, metrics)
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('[SnapshotCollector] Error:', error)
|
console.error('[SnapshotCollector] Error:', error)
|
||||||
|
// Reset cache on error so next tick re-resolves
|
||||||
|
cachedPayload = null
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -72,29 +72,37 @@ describe('evaluateCondition', () => {
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
describe('AlertEvaluator.shouldFire', () => {
|
describe('AlertEvaluator.shouldFire + recordFired', () => {
|
||||||
it('allows first fire', () => {
|
it('allows first fire', () => {
|
||||||
const evaluator = new AlertEvaluator()
|
const evaluator = new AlertEvaluator()
|
||||||
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('blocks during cooldown', () => {
|
it('blocks during cooldown after recordFired', () => {
|
||||||
const evaluator = new AlertEvaluator()
|
const evaluator = new AlertEvaluator()
|
||||||
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
||||||
|
evaluator.recordFired('rule-1')
|
||||||
expect(evaluator.shouldFire('rule-1', 15)).toBe(false)
|
expect(evaluator.shouldFire('rule-1', 15)).toBe(false)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('allows re-check if recordFired was not called', () => {
|
||||||
|
const evaluator = new AlertEvaluator()
|
||||||
|
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
||||||
|
// Without recordFired, the cooldown is not active
|
||||||
|
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
it('different rules have independent cooldowns', () => {
|
it('different rules have independent cooldowns', () => {
|
||||||
const evaluator = new AlertEvaluator()
|
const evaluator = new AlertEvaluator()
|
||||||
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
expect(evaluator.shouldFire('rule-1', 15)).toBe(true)
|
||||||
|
evaluator.recordFired('rule-1')
|
||||||
expect(evaluator.shouldFire('rule-2', 15)).toBe(true)
|
expect(evaluator.shouldFire('rule-2', 15)).toBe(true)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('allows fire after cooldown expires', () => {
|
it('allows fire after cooldown expires', () => {
|
||||||
const evaluator = new AlertEvaluator()
|
const evaluator = new AlertEvaluator()
|
||||||
expect(evaluator.shouldFire('rule-1', 0)).toBe(true)
|
evaluator.recordFired('rule-1')
|
||||||
// With 0-minute cooldown, immediate re-fire should be allowed
|
// With 0-minute cooldown, immediate re-fire should be allowed
|
||||||
// (elapsed time > 0 which is >= 0)
|
|
||||||
expect(evaluator.shouldFire('rule-1', 0)).toBe(true)
|
expect(evaluator.shouldFire('rule-1', 0)).toBe(true)
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue