-
Notifications
You must be signed in to change notification settings - Fork 28
fix: measure memory while containers are running, not after teardown #1765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Outputs structured JSON with mean, median, p95, p99 per metric. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { execSync, ExecSyncOptions } from "child_process"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { execSync, ExecSyncOptions, spawn, ChildProcess } from "child_process"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ── Configuration ────────────────────────────────────────────────── | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -159,45 +159,115 @@ function benchmarkHttpsLatency(): BenchmarkResult { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { metric: "squid_https_latency", unit: "ms", values, ...stats(values) }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function benchmarkMemory(): BenchmarkResult { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Wait for Docker containers to be running, polling at 500ms intervals. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Throws if containers are not running within timeoutMs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function waitForContainers(containerNames: string[], timeoutMs: number): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const start = Date.now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new Promise((resolve, reject) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const poll = (): void => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Date.now() - start > timeoutMs) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| reject(new Error(`Containers not running after ${timeoutMs}ms`)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const allRunning = containerNames.every((name) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = execSync( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `sudo docker ps --filter name=${name} --filter status=running --format '{{.Names}}' 2>/dev/null`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { encoding: "utf-8", timeout: 5_000 } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result.includes(name); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (allRunning) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolve(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // container not ready yet | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setTimeout(poll, 500); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| poll(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Parse a Docker memory usage string like "123.4MiB / 7.773GiB" into MB. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function parseMb(s: string): number { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const match = s.match(/([\d.]+)\s*(MiB|GiB|KiB)/i); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!match) return 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const val = parseFloat(match[1]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const unit = match[2].toLowerCase(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (unit === "gib") return val * 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (unit === "kib") return val / 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return val; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Kill a spawned background process and its process group, best-effort. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function killBackground(child: ChildProcess): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (child.pid) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Kill the process group (negative PID) to catch child processes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| process.kill(-child.pid, "SIGTERM"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Process may have already exited | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+223
to
+230
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | |
| if (child.pid) { | |
| // Kill the process group (negative PID) to catch child processes | |
| process.kill(-child.pid, "SIGTERM"); | |
| } | |
| } catch { | |
| // Process may have already exited | |
| } | |
| try { | |
| const pid = child.pid; | |
| if (!pid) { | |
| return; | |
| } | |
| try { | |
| // Kill the process group (negative PID) to catch child processes | |
| process.kill(-pid, "SIGTERM"); | |
| } catch { | |
| // Process group may have already exited | |
| } | |
| try { | |
| // Force-kill the entire process group so descendant processes do not survive | |
| process.kill(-pid, "SIGKILL"); | |
| } catch { | |
| // Process group may have already exited | |
| } | |
| try { | |
| // Best-effort fallback for the direct child process |
Copilot
AI
Apr 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path hard-codes sudo + awf in spawn(...) even though the script already defines AWF_CMD = "sudo awf" and uses it elsewhere. To avoid future drift (e.g., if AWF_CMD changes), consider deriving the spawned command/args from AWF_CMD or otherwise reusing that constant.
Copilot
AI
Apr 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the child process is spawned with detached: true and stdio: 'ignore', it’s typically a good idea to call child.unref() after spawning. This prevents the parent benchmark script from being kept alive if cleanup/kill logic fails in an edge case.
| ); | |
| ); | |
| child.unref(); |
Copilot
AI
Apr 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benchmark proceeds once containers are merely status=running. Since Squid has a Docker healthcheck (and the agent depends on it), sampling memory before Squid is healthy can make the metric noisy and can mask startup failures/restarts. Consider waiting for Squid to be healthy (e.g., via docker ps --filter health=healthy or docker inspect health status) before taking the memory sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForContainers()usesdocker ps --filter name=${name}and thenresult.includes(name). Docker's name filter is a regex/substring match, so this can return true for containers likeawf-squid-old, leading to false positives. Use an anchored name filter (e.g.,name=^${name}$) and/or require an exact match on the returned name(s).