Skip to content

add numa_node and missing test output file#2

Merged
jj-asama merged 2 commits intomasterfrom
pcidevice
Sep 20, 2025
Merged

add numa_node and missing test output file#2
jj-asama merged 2 commits intomasterfrom
pcidevice

Conversation

@jj-asama
Copy link
Copy Markdown
Member

@jj-asama jj-asama commented Sep 20, 2025

Summary by CodeRabbit

  • New Features

    • Added NUMA node metric for PCI devices (node_pcidevice_numa_node), emitted alongside existing PCIe metrics with consistent labels.
    • Reports -1 when NUMA information is unavailable, ensuring complete coverage across devices.
  • Tests

    • Updated fixtures to include NUMA node examples and a new PCI device instance, reflecting expanded metric output (link widths/transfers, power state, SR-IOV, D3cold, and device info) for validation.

Signed-off-by: Jain Johny <jj@asama.ai>
Signed-off-by: Jain Johny <jj@asama.ai>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Sep 20, 2025

Walkthrough

Adds a NUMA node metric for PCI devices to the pcidevice collector and updates fixtures. The collector now emits node_pcidevice_numa_node, defaulting to -1 when NUMA info is absent. E2E and names fixtures are expanded to include entries for a new PCI device on bus 45 and NUMA node samples.

Changes

Cohort / File(s) Summary of Changes
Collector logic (NUMA metric)
collector/pcidevice_linux.go
Added descriptor and emission for node_pcidevice_numa_node; Update path sets -1 when NumaNode is nil; registered alongside existing PCI metrics.
E2E fixture expansion (bus 45 and metrics)
collector/fixtures/e2e-output.txt
Added a new PCI device (bus 45) across existing metrics and included corresponding entries for NUMA node and SR-IOV-related metrics, mirroring existing patterns.
Names fixture (NUMA metric declaration and samples)
collector/fixtures/pcidevice-names-output.txt
Introduced HELP/TYPE and sample rows for node_pcidevice_numa_node gauge with three labeled instances (-1, -1, 0).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant P as Prometheus
  participant NE as node_exporter
  participant PC as pcidevice collector
  participant SYS as sysfs/PCI

  P->>NE: /metrics scrape
  NE->>PC: Collect()
  PC->>SYS: Enumerate PCI devices
  SYS-->>PC: Device info (incl. optional NUMA)
  alt NUMA node present
    PC->>NE: node_pcidevice_numa_node{labels}=<numa_id>
  else NUMA node absent
    PC->>NE: node_pcidevice_numa_node{labels}=-1
  end
  PC->>NE: Other pcidevice metrics (existing + bus 45 entries)
  NE-->>P: Expose metrics
  note over PC,NE: New metric emitted with same label set as other PCI metrics
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

In racks where silver buses hum,
I twitch my ears—new numbers come!
A forty-five hops in the dash,
With NUMA notes in tidy cache.
If none is found? We mark “-1.”
I thump my paws—the scrape is done. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "add numa_node and missing test output file" accurately and concisely summarizes the primary change in the PR—adding a NUMA node metric and adding the missing test/fixture output—matching the modifications to collector/pcidevice_linux.go and the fixture files in the provided summary, and is specific enough for repository history.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch pcidevice

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
collector/pcidevice_linux.go (1)

237-241: Bug: nil PowerState reports as D0 (should be Unknown)

When device.PowerState is nil, powerState stays 0, contradicting the HELP (-1=Unknown). Default to -1.

-    var powerState float64
+    powerState := -1.0
     if device.PowerState != nil {
       powerState = parsePowerStateFromEnum(*device.PowerState)
     }
🧹 Nitpick comments (1)
collector/pcidevice_linux.go (1)

300-314: Order & count verified — optional: convert to explicit per-descriptor sends

c.descs (11 entries — collector/pcidevice_linux.go:164–176) matches the values slice (11 values — collector/pcidevice_linux.go:300–314); emission order is correct. Optional: replace the index-coupled for-loop with explicit ch <- c.descs[...].mustNewConstMetric(...) sends to reduce fragility when adding metrics.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 51db1b3 and 81be21b.

📒 Files selected for processing (3)
  • collector/fixtures/e2e-output.txt (1 hunks)
  • collector/fixtures/pcidevice-names-output.txt (1 hunks)
  • collector/pcidevice_linux.go (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (go)
🔇 Additional comments (5)
collector/pcidevice_linux.go (3)

103-108: NUMA node metric descriptor looks correct

Name, labels, and HELP text align with the rest of the collector.


277-284: Nil-safe NUMA handling is consistent

Using -1 as the unknown sentinel matches the HELP and other fields’ conventions.


175-176: Descriptor ordering preserved

Appending pcideviceNumaNodeDesc at the end keeps index-based emissions stable. node_pcidevice_numa_node is present in collector/fixtures/pcidevice-names-output.txt and collector/fixtures/e2e-output.txt.

collector/fixtures/pcidevice-names-output.txt (1)

33-37: Names fixture updated for NUMA: OK

HELP/TYPE and samples align with code (-1 unknown, 0 for known node).

collector/fixtures/e2e-output.txt (1)

2861-2916: E2E fixture extended (bus 45 + NUMA): OK

Both fixtures expose node_pcidevice_numa_node and counts look sane — e2e: bus=45 -> 0 (NUMA 0), bus=00/01 -> -1 (unknown). Files: collector/fixtures/pcidevice-names-output.txt:34, collector/fixtures/e2e-output.txt:2878.

@jj-asama jj-asama merged commit 61bc349 into master Sep 20, 2025
4 checks passed
jj-asama added a commit that referenced this pull request Oct 21, 2025
* add numa_node and missing test output file

Signed-off-by: Jain Johny <jj@asama.ai>
jj-asama added a commit that referenced this pull request Oct 22, 2025
* add numa_node and missing test output file

Signed-off-by: Jain Johny <jj@asama.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant