Skip to content

Add system metrics - #4076

Open
alrevuelta wants to merge 1 commit into
mainfrom
add-system-metrics-docker
Open

Add system metrics#4076
alrevuelta wants to merge 1 commit into
mainfrom
add-system-metrics-docker

Conversation

@alrevuelta

Copy link
Copy Markdown
Contributor

Describe your changes and provide context

Grafana and prometheus in monitornode dont expose system metrics, which are very important for observability. This PR adds node-exporter which exposes system metrics (mostly linux specific). See:

docker/monitornode/scripts/start-node-exporter.sh
docker/monitornode/scripts/stop-node-exporter.sh

For the new metrics this exposes, see:

curl -s http://localhost:9100/metrics

Some relevant ones:

node_nvme_namespace_capacity_bytes
node_pressure_cpu_waiting_seconds_total
node_pressure_io_stalled_seconds_total
node_pressure_io_waiting_seconds_total
node_pressure_memory_waiting_seconds_total
node_procs_blocked
... and many more

@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Observability-only Docker/bash changes with no application or auth logic; Linux-only guard reduces misuse on macOS/Windows.

Overview
Adds host-level system metrics (CPU, memory, disk, filesystem) to the local Prometheus/Grafana stack used by cryptosim benchmarks.

Prometheus config gains a node scrape job targeting host.docker.internal:9100 every 5s. New start-node-exporter.sh runs prom/node-exporter on Linux with --net=host, --pid=host, and a read-only host root mount so metrics reflect the bench machine, not the container; it refuses non-Linux hosts and optionally POSTs Prometheus reload on port 9091. stop-node-exporter.sh stops the sei-node-exporter container without removing it.

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.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedSep 2, 2026, 11:47 AM

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md is 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.sh alongside step 3 ("Start Prometheus Server") there.
  • [suggestion] The new node scrape job is unconditional in prometheus.yaml, but start-node-exporter.sh refuses 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 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The Prometheus reload is fully silent on failure and is only attempted on the container-creation path. Two gaps:

  1. curl is an undeclared dependency (the header lists only Docker). If it is missing, or if the reload returns non-2xx, the if simply falls through and the user is told "node_exporter is running" while an already-running Prometheus never picks up the node job.
  2. The two early exit 0 paths 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

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.16%. Comparing base (b68026f) to head (b443715).

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 108 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alrevuelta
alrevuelta added this pull request to the merge queue Sep 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants