Add system metrics - #4076
Conversation
PR SummaryLow Risk Overview Prometheus config gains a The cryptosim README now lists the start/stop scripts alongside Prometheus and Grafana. Reviewed by Cursor Bugbot for commit b443715. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
Adds a node_exporter start/stop script pair plus a matching Prometheus scrape job for host system metrics. The scripts mirror the existing start-prometheus.sh/start-grafana.sh conventions and the container invocation matches upstream node_exporter's documented Docker recipe; only non-blocking hardening and documentation gaps remain.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The "Running in AWS" section of
sei-db/state_db/bench/cryptosim/README.mdis not updated to start node_exporter, even though the remote bench host is exactly where host CPU/disk/PSI metrics matter most (the script's own header says the numbers should be "the machine running the bench"). Consider adding./docker/monitornode/scripts/start-node-exporter.shalongside step 3 ("Start Prometheus Server") there. - [suggestion] The new
nodescrape job is unconditional inprometheus.yaml, butstart-node-exporter.shrefuses to run on macOS/Windows by design. macOS users following the local setup section will get a permanently DOWN target with no explanation beyond the YAML comment; a note in the README code block ("Linux only — skipped on macOS") would save some confusion. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| echo "Creating and starting node_exporter container..." | ||
| docker run -d \ | ||
| --name "$CONTAINER_NAME" \ | ||
| --net=host \ |
There was a problem hiding this comment.
[suggestion] With --net=host and no --web.listen-address, node_exporter binds 0.0.0.0:9100 on the host, publishing fairly detailed host telemetry (filesystems, NVMe namespaces, PSI, process counts) to anything that can reach the machine. This matters most on the AWS bench flow in the cryptosim README, where the deliberate use of an SSH tunnel for Prometheus (ssh -L 9091:localhost:9091) suggests the intent is not to open monitoring ports at all.
Note that the obvious fix does not work: --web.listen-address=127.0.0.1:9100 would break the scrape, since the Prometheus container reaches the host via the host-gateway bridge address rather than loopback. The workable options are binding to the docker bridge gateway explicitly, or documenting in the script header that access control is left to the host firewall / security group. A one-line note in the header would be enough.
| echo "Grafana: import dashboard 1860 (Node Exporter Full), or query node_* in Explore." | ||
|
|
||
| # Config is bind-mounted; reload so a running Prometheus picks up the node job. | ||
| if curl -sf -X POST "http://localhost:${PROMETHEUS_UI_PORT}/-/reload" >/dev/null 2>&1; then |
There was a problem hiding this comment.
[suggestion] The Prometheus reload is fully silent on failure and is only attempted on the container-creation path. Two gaps:
curlis an undeclared dependency (the header lists only Docker). If it is missing, or if the reload returns non-2xx, theifsimply falls through and the user is told "node_exporter is running" while an already-running Prometheus never picks up thenodejob.- The two early
exit 0paths above (container already running, container stopped-then-started) skip the reload entirely.
Adding an else branch that prints something like "Could not reload Prometheus; restart it to pick up the 'node' scrape job" — and moving the reload before the early exits, or repeating it on the restart path — makes the failure self-diagnosing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4076 +/- ##
==========================================
- Coverage 61.23% 60.16% -1.07%
==========================================
Files 2177 2069 -108
Lines 190632 178134 -12498
==========================================
- Hits 116729 107178 -9551
+ Misses 62892 60987 -1905
+ Partials 11011 9969 -1042
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Describe your changes and provide context
Grafana and prometheus in
monitornodedont expose system metrics, which are very important for observability. This PR addsnode-exporterwhich exposes system metrics (mostly linux specific). See:For the new metrics this exposes, see:
Some relevant ones: