Skip to content

feat: add NUMA node and shared-cache-domain breakouts to mpstat metrics - #69

Merged
k-rister merged 1 commit into
masterfrom
feat-cpu-node-and-cache-domain-breakouts
Aug 20, 2026
Merged

feat: add NUMA node and shared-cache-domain breakouts to mpstat metrics#69
k-rister merged 1 commit into
masterfrom
feat-cpu-node-and-cache-domain-breakouts

Conversation

@k-rister

Copy link
Copy Markdown
Contributor

Summary

  • Adds node (NUMA node) and shared-l1-domain through shared-l4-domain (per-cache-level CPU grouping) breakout dimensions to mpstat metrics, plus a cpu string alias for num matching tool-procstat/tool-kernel naming conventions.
  • Closes mpstat: add 'node' (NUMA node) and 'cpu' (alias for num) as CDM breakout dimensions #68 (the node-breakout ask; generalizes the L3-only cache-domain ask to every cache level a platform reports).
  • sysstat-start now also collects cache/index*/{level,shared_cpu_list} sysfs data and resolves each CPU's NUMA node into a cpu-numa-nodes.txt manifest (sysfs nodeN entries are symlinks, which report a zero apparent size and break cpio's readlink() call — same workaround pattern already used for netdev-types.txt).
  • sysstat-post-process.py wires the new toolbox helpers (get_cpu_node(), get_cpu_cache_domains()) into process_mpstat().

Dependencies (both merged)

  • toolbox PR#131 — build_cpu_topology()/get_cpu_node()/get_cpu_cache_domains()
  • CommonDataModel PR#208 — explicit shared-l1-domain..shared-l4-domain schema fields

Test plan

  • Verified end-to-end against this host's real sysfs tree: built a fixture with the actual updated collection commands, ran process_mpstat(), confirmed output correctly contains cpu/node/shared-lN-domain matching the host's real topology
  • Verified against live OpenSearch: shared-l3-domain accepted, unregistered shared-l5-domain correctly rejected
  • toolbox's 39-test suite (including 16 new tests for the underlying topology functions) green

🤖 Generated with Claude Code

Closes tool-sysstat#68 (node breakout item; generalizes the L3-only ask
to every cache level a platform reports). Also adds a `cpu` string
alias for `num` matching tool-procstat/tool-kernel naming. Depends on
toolbox's build_cpu_topology()/get_cpu_node()/get_cpu_cache_domains()
(toolbox PR#131) and CDM's explicit shared-lN-domain schema fields
(CommonDataModel PR#208), both merged.
@project-crucible-tracking project-crucible-tracking Bot moved this from Queued to In Progress in Crucible Tracking Aug 19, 2026
@k-rister
k-rister requested a review from a team August 19, 2026 13:32
@k-rister k-rister self-assigned this Aug 19, 2026
@k-rister

Copy link
Copy Markdown
Contributor Author

PR Review: tool-sysstat#69 — feat: add NUMA node and shared-cache-domain breakouts to mpstat metrics

Summary: Adds NUMA node and shared-cache-domain breakout dimensions to mpstat metrics and collects the required sysfs CPU topology and cache data via sysstat-start.
Changed files: 3
Review dimensions: Correctness, API & Contracts, Build & Deploy, Documentation, Style, Completeness

Issues

  • [sysstat-start:55] Append without truncation for cpu-numa-nodes.txtsysstat-start populates cpu-numa-nodes.txt using append redirection (>>) within its main loop but never truncates or cleans up the file at the start of the script (unlike sysstat-pids.txt which is explicitly removed via /bin/rm -f sysstat-pids.txt).

    • Triggering scenario: If the start script is run multiple times in the same working directory (e.g., during manual profiling, container/workspace reuse, or consecutive profile iterations), cpu-numa-nodes.txt will continually grow with duplicate entries of the same CPU-to-node mappings. While build_cpu_topology() is resilient to duplicate lines (simply overwriting keys in its internal dictionary), the file accumulates unnecessary duplicate lines and could lead to stale topology lookups if container CPU affinity or pinning changes between executions.
    • Recommendation: Clean up the file at the beginning of the sysfs check block in sysstat-start:
      # To be used by mpstat for post-processing
      if [ -e /sys/devices/system/cpu/ ]; then
          /bin/rm -f cpu-numa-nodes.txt
          find /sys/devices/system/cpu/ | grep -E 'topology|cache/index[0-9]+/(level|shared_cpu_list)$' | cpio -pdumv --quiet . 2>/dev/null
  • [sysstat-post-process.py:384] Generation of unregistered cache level breakout fields — On high-end or future platforms reporting cache index levels greater than L4 (e.g., L5 cache), sysstat-post-process.py will dynamically construct and generate breakout fields like shared-l5-domain.

    • Triggering scenario: Since CommonDataModel (PR#208) explicitly registers fields only from shared-l1-domain through shared-l4-domain, any generated shared-l5-domain breakout field will fail schema validation and be rejected at metrics ingestion/indexing time.
    • Recommendation: Restrict the level to registered schema fields (levels 1–4) before adding them to the breakout names dict:
      for level, domain in cache_domains.items():
          if level in ("1", "2", "3", "4"):
              names[f"shared-l{level}-domain"] = domain

File Coverage

  • CLAUDE.md — No issues found.
  • sysstat-post-process.py — 1 issue
  • sysstat-start — 1 issue

Limitations

  • Runtime validation on hardware: Cannot fully verify hardware-specific behaviors (e.g., multi-socket NUMA node layouts, non-sequential core-sibling/cache-domain lists, or physical CPU topology layouts) within a virtualized sandbox.

Verdict

Approve with comments — The PR is fully functional, safe to merge, and correctly resolves the requested features. The identified issues are minor suggestions that can be easily fixed by the author or in a follow-up.

@k-rister

Copy link
Copy Markdown
Contributor Author

Thanks for the review — responding to both findings:

cpu-numa-nodes.txt truncation concern: Declining. Tool start scripts run exactly once per engine per run in a freshly-created tool-data/sysstat/ directory (see start_tools() in docs/how-tool-collection-works.md in the crucible repo) — the "run multiple times in the same working directory" scenario isn't actually reachable. This also matches the existing netdev-types.txt manifest a few lines below, which uses the identical unguarded >> append pattern with no prior cleanup.

Unregistered cache-level (L5+) concern: Declining the suggested filter. This is intentional, not an oversight: add-run.js hard-aborts (process.exit(1)) when it encounters a document field not registered in CDM's indexDefs, and that's by design — it was chosen specifically so an unrecognized breakout dimension fails loudly and forces a CDM schema update, rather than silently dropping data. Filtering cache levels out in the Python collector would quietly swallow the field before it ever reaches that safety net, defeating the reason it exists. If a platform ever reports an L5+ cache, the correct fix is to extend CDM's shared-lN-domain schema, not suppress the field client-side.

@atheurer atheurer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Great work adding NUMA node and shared-cache-domain breakouts to mpstat metrics, and cleanly handling sysfs symlinks via cpu-numa-nodes.txt.

@k-rister
k-rister merged commit 1d2a382 into master Aug 20, 2026
38 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Crucible Tracking Aug 20, 2026
@k-rister
k-rister deleted the feat-cpu-node-and-cache-domain-breakouts branch August 20, 2026 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

mpstat: add 'node' (NUMA node) and 'cpu' (alias for num) as CDM breakout dimensions

2 participants