fix: enable Ascend NPU container-level metrics in exporter and chart display#111
fix: enable Ascend NPU container-level metrics in exporter and chart display#111pingxin403 wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pingxin403 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Ascend task metric queries in the exporter, with tests covering core, memory, and empty-result behavior, and updates the task admin trend chart to render the chart whenever a task type is present. ChangesAscend NPU metrics
Trend chart rendering simplification
Estimated code review effort: 2 (Simple) | ~12 minutes Related Issues: Suggested labels: exporter, ascend, frontend Suggested reviewers: none identified from the provided context Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/internal/exporter/exporter.go`:
- Around line 566-570: The Ascend GPU branch in GenerateContainerMetrics is
returning HBM usage in MB from npu_chip_info_hbm_used_memory, but the later
memory metrics path still assumes bytes and divides again, so the reported
values are far too small. Update the AscendGPUDevice handling in exporter.go to
convert the queried HBM value from MB to bytes before it is assigned to
taskMemoryUsed and used by HamiContainerMemoryUsed/HamiContainerMemoryUtil,
keeping the behavior consistent with the other device branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae090b7c-497b-4471-b580-29aa33ebadc9
📒 Files selected for processing (3)
packages/web/projects/vgpu/views/task/admin/Detail.vueserver/internal/exporter/exporter.goserver/internal/exporter/exporter_test.go
…display Replace hardcoded return 0, nil with real PromQL queries (npu_chip_info_utilization, npu_chip_info_hbm_used_memory) for Ascend taskCoreUsed/taskMemoryUsed. Add AscendGPUDevice case to GenerateContainerMetrics switch for core unit conversion. Remove NVIDIA/MXC type guards in task detail view so all vendors show trend charts. Closes Project-HAMi#86 Signed-off-by: pingxin403 <pingxin403@users.noreply.github.com>
pingxin403
force-pushed
the
feat/ascend-npu-metrics
branch
from
51a399e to
ef1240b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/internal/exporter/exporter_test.go (3)
1-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the outgoing PromQL query in
testPromHandler.The fake handler (per description) only checks HTTP method/path, not the query string sent by the client. Asserting that the request's
queryparameter matches the expected Ascend PromQL (metric name,vdie_idlabel) would guard against regressions like a typo in the metric name that would otherwise silently still return a valid-looking response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/exporter/exporter_test.go` around lines 1 - 38, Update testPromHandler in exporter_test.go to also verify the outgoing PromQL query, not just the method and /api/v1/query path. Read the request's query parameter and assert it matches the expected Ascend query shape used by the exporter, including the metric name and vdie_id label. Keep the fake response behavior the same so the existing tests still exercise the client through the exporter code paths.
90-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a matching empty-result test for
taskMemoryUsed.An empty-result test exists for
taskCoreUsedbut not fortaskMemoryUsed, even though both share the samequeryInstantValempty-vector fallback path. Adding the symmetric case improves coverage consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/exporter/exporter_test.go` around lines 90 - 101, Add a symmetric empty-result test for taskMemoryUsed in exporter_test.go to match TestMetricsGenerator_taskCoreUsed_Ascend_EmptyResult. Reuse newTestGenerator and testPromHandler with an empty response, then call gen.taskMemoryUsed with the same AscendGPUDevice-style arguments and assert it returns 0 with no error. Keep the test alongside the existing taskCoreUsed empty-result case so the shared queryInstantVal fallback path is covered consistently.
1-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an Ascend
GenerateContainerMetricstest
server/internal/exporter/exporter_test.goonly coverstaskCoreUsed/taskMemoryUsed; it doesn’t assert the finalHamiContainerMemoryUsedvalue fromGenerateContainerMetricsforbiz.AscendGPUDevice. Add a case that feeds MB-sized HBM data and checks the exported MiB value after the*1000*1000→/1024/1024conversion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/internal/exporter/exporter_test.go` around lines 1 - 101, Add a test for GenerateContainerMetrics on biz.AscendGPUDevice that verifies the final HamiContainerMemoryUsed output, not just taskMemoryUsed. Reuse the existing exporter test setup in MetricsGenerator/newTestGenerator with a fake Prometheus response that returns MB-sized HBM data, then call GenerateContainerMetrics and assert the exported memory value reflects the expected MiB conversion from the underlying *1000*1000 to /1024/1024 path. Use the existing taskMemoryUsed and GenerateContainerMetrics symbols to locate the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/internal/exporter/exporter_test.go`:
- Around line 1-38: Update testPromHandler in exporter_test.go to also verify
the outgoing PromQL query, not just the method and /api/v1/query path. Read the
request's query parameter and assert it matches the expected Ascend query shape
used by the exporter, including the metric name and vdie_id label. Keep the fake
response behavior the same so the existing tests still exercise the client
through the exporter code paths.
- Around line 90-101: Add a symmetric empty-result test for taskMemoryUsed in
exporter_test.go to match TestMetricsGenerator_taskCoreUsed_Ascend_EmptyResult.
Reuse newTestGenerator and testPromHandler with an empty response, then call
gen.taskMemoryUsed with the same AscendGPUDevice-style arguments and assert it
returns 0 with no error. Keep the test alongside the existing taskCoreUsed
empty-result case so the shared queryInstantVal fallback path is covered
consistently.
- Around line 1-101: Add a test for GenerateContainerMetrics on
biz.AscendGPUDevice that verifies the final HamiContainerMemoryUsed output, not
just taskMemoryUsed. Reuse the existing exporter test setup in
MetricsGenerator/newTestGenerator with a fake Prometheus response that returns
MB-sized HBM data, then call GenerateContainerMetrics and assert the exported
memory value reflects the expected MiB conversion from the underlying *1000*1000
to /1024/1024 path. Use the existing taskMemoryUsed and GenerateContainerMetrics
symbols to locate the relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 872df6dc-8540-4bbd-9609-09a7529451ef
📒 Files selected for processing (2)
server/internal/exporter/exporter.goserver/internal/exporter/exporter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/exporter/exporter.go
… metrics npu_chip_info_hbm_used_memory returns MB (megabytes, 10^6 bytes), not bytes. The downstream /1024/1024 conversion expects bytes input for MiB output. Fix: multiply by 1000*1000 to convert MB to bytes before the division. Address CodeRabbit review comment on MB vs byte unit mismatch. Signed-off-by: pingxin403 <pingxin403@users.noreply.github.com>
pingxin403
force-pushed
the
feat/ascend-npu-metrics
branch
from
8516305 to
2d2366e
Compare
1 participant
Description
Fix Ascend NPU container metrics always returning 0 in exporter, and enable monitoring charts for non-NVIDIA/MXC vendors.
Backend changes
taskCoreUsed: replacereturn 0, nilwithnpu_chip_info_utilizationPromQL querytaskMemoryUsed: replacereturn 0, nilwithnpu_chip_info_hbm_used_memoryPromQL queryGenerateContainerMetrics: addAscendGPUDevicecase for core unit conversionFrontend changes
Closes #86
Summary by CodeRabbit