Skip to content

feat: fix metric classes and add default-aggregation to all metrics - #64

Merged
k-rister merged 1 commit into
masterfrom
feat-class-and-aggregation
Aug 5, 2026
Merged

feat: fix metric classes and add default-aggregation to all metrics#64
k-rister merged 1 commit into
masterfrom
feat-class-and-aggregation

Conversation

@k-rister

@k-rister k-rister commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • mpstat/pidstat: migrate Busy-CPU and NonBusy-CPU from class=throughput to class=percentage — these are 0-1 CPU utilization fractions, not throughput rates
  • iostat: migrate percent-utilization from class=throughput to class=percentage
  • iostat: migrate percent-merged from class=count to class=percentage
  • iostat: migrate avg-service-time-ms from class=count to class=latency
  • Add explicit default-aggregation to all metric descriptors: sum for throughput rates, avg for percentages/latency/counts

Part of PERFNFV-421 / epic PERFNFV-409

Test plan

  • Run benchmark with sysstat tool, verify class assignments on mpstat/pidstat/iostat metrics
  • Verify all metrics have default-aggregation in metric_desc documents

🤖 Generated with Claude Code

mpstat/pidstat Busy-CPU and NonBusy-CPU: throughput -> percentage
(these are 0-1 CPU utilization fractions, not throughput rates).

iostat percent-utilization: throughput -> percentage.
iostat percent-merged: count -> percentage.
iostat avg-service-time-ms: count -> latency.

Add explicit default-aggregation to all metric descriptors: sum for
throughput rates, avg for percentages/latency/counts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@k-rister k-rister self-assigned this Aug 4, 2026
@k-rister
k-rister requested a review from a team August 4, 2026 20:51
@project-crucible-tracking project-crucible-tracking Bot moved this to In Progress in Crucible Tracking Aug 4, 2026

@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.

Approving per team decision, but flagging the concern found during review: default-aggregation is written directly into the metric_desc document, but CommonDataModel's metric_desc index mapping is dynamic: strict and only declares class/type/source/names-list/names — an aggregation field (aggregation-method, a different name) is called out in cdm.js as planned for a future schema version but not yet enabled. This could cause metric_desc writes to be rejected by OpenSearch under strict mapping.

This same field is being added in parallel by several other PRs (tool-dpdk#9, bench-fio#67, bench-pytorch#15, bench-hwlatdetect#16, bench-hwnoise#21, bench-oslat#89, bench-osnoise#18, bench-timerlat#14) — recommend verifying CDM schema support once, centrally, before any of these land in production, rather than per-repo.

Also noting the new percentage/latency class values used here are novel relative to other tool-* repos (which mostly use throughput/count/logging/pass) — worth confirming CDM's aggregation logic actually branches on these values.

@atheurer

atheurer commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Correction to my earlier approval comment: I checked upstream CommonDataModel and default-aggregation IS already declared in the metric_desc index mapping (templates/metric_desc.base, type keyword — added in CDM PR #202). Despite dynamic: strict, this field is explicitly mapped, so it will not be rejected by OpenSearch. Retracting that concern. The note about percentage/latency being novel class values relative to other tool-* repos still stands as a minor consistency observation, not a blocker.

@k-rister
k-rister merged commit 30a2b21 into master Aug 5, 2026
72 of 74 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Crucible Tracking Aug 5, 2026
@k-rister
k-rister deleted the feat-class-and-aggregation branch August 5, 2026 16:17
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.

2 participants