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 5 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Prometheus-backed Ascend task compute and memory metrics with conversion tests, and updates task admin trend charts to render from available data while clearing failed requests. ChangesAscend NPU metrics
Trend chart rendering simplification
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/web/projects/vgpu/views/task/admin/Detail.vueParsing error: Cannot find module '
Make sure that all the Babel plugins and presets you are using Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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>
51a399e to
ef1240b
Compare
… 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 <52284317+pingxin403@users.noreply.github.com>
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>
8516305 to
2d2366e
Compare
- Fix gofmt indentation error in Ascend/SGPU/Default case labels - Fix memory unit semantics: npu_chip_info_hbm_used_memory returns bytes, not MB; remove incorrect *1000*1000 MB-to-bytes conversion - Add device-scoped limitation comments for Ascend PromQL queries (taskCoreUsed, taskMemoryUsed) noting they return whole-device values - Add conversion tests: deviceMemUsed, memory bytes→MiB, core util math - Fix frontend empty-chart regression: guard VChart with data.length > 0, show el-empty fallback when no data, add .catch() for API failures Signed-off-by: Fixer <fixer@users.noreply.github.com> Signed-off-by: pingxin403 <pingxin403@users.noreply.github.com>
…WebUI into feat/ascend-npu-metrics Signed-off-by: pingxin403 <pingxin403@users.noreply.github.com> # Conflicts: # server/internal/exporter/exporter.go # server/internal/exporter/exporter_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/internal/exporter/exporter_test.go (2)
119-167: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftExercise the production conversion path.
These tests duplicate the formulas from
GenerateContainerMetricslocally, so a regression in the Ascend core or memory switch branches would still pass. DriveGenerateContainerMetricswith fake devices/containers and assert the emitted metrics, or extract the shared conversion logic into a helper and test that helper directly.🤖 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 119 - 167, Update TestMetricsGenerator_AscendMemoryConversion and TestMetricsGenerator_AscendCoreConversion to exercise GenerateContainerMetrics rather than duplicating its conversion formulas locally. Configure fake Ascend devices and containers, invoke GenerateContainerMetrics, and assert the emitted memory, core-used, and utilization metrics; alternatively extract the shared conversion logic into a helper and test that helper directly.
23-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the actual PromQL request.
The fake handler ignores the query and returns the configured value for every request, so these tests would pass even if the implementation used the wrong metric or omitted the
vdie_idselector. Accept an expected query per test and compare it with the request’s query parameter/body.🤖 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 23 - 38, Update testPromHandler to accept an expected PromQL query and assert it against the request’s query parameter or POST body before returning the configured response. Update each test using testPromHandler to provide the expected metric query, including the required vdie_id selector, so incorrect or missing queries fail.
🤖 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 119-167: Update TestMetricsGenerator_AscendMemoryConversion and
TestMetricsGenerator_AscendCoreConversion to exercise GenerateContainerMetrics
rather than duplicating its conversion formulas locally. Configure fake Ascend
devices and containers, invoke GenerateContainerMetrics, and assert the emitted
memory, core-used, and utilization metrics; alternatively extract the shared
conversion logic into a helper and test that helper directly.
- Around line 23-38: Update testPromHandler to accept an expected PromQL query
and assert it against the request’s query parameter or POST body before
returning the configured response. Update each test using testPromHandler to
provide the expected metric query, including the required vdie_id selector, so
incorrect or missing queries fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86f8ae8d-9ad2-4202-ac00-3b61b343e747
📒 Files selected for processing (3)
packages/web/projects/vgpu/views/task/admin/Detail.vueserver/internal/exporter/exporter.goserver/internal/exporter/exporter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/projects/vgpu/views/task/admin/Detail.vue
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