Skip to content

get-metric --aggregation has no validation against the metric's class #207

Description

@k-rister

Problem

crucible get metric --aggregation <sum|avg|max|min> overrides a metric's stored default-aggregation for a single query. Traced the implementation (queries/cdmq/server.js, cdm.js) while adding crucible-ci integration test coverage for this flag:

  • No client-side validation at all. get-metric-data.js's Commander .option('--aggregation <sum|avg|max|min>', ...) is a help-text placeholder only — no .choices() — so the CLI will send any string.
  • Server-side validation only checks the flag is one of the four literal values (server.js):
    var validAggregations = ['sum', 'avg', 'max', 'min'];
    if (aggregation && !validAggregations.includes(aggregation)) {
        return res.status(400).json({ code: 'INVALID_AGGREGATION', ... });
    }
  • Nothing cross-checks the requested aggregation against the target metric's class. A query can request --aggregation avg (or any of the four) against a boolean-class metric, or max/min against a pass/fail-class one, and the query engine will happily compute and return a number.

This is distinct from toolbox/python/toolbox/cdm_metrics.py's validate_metric_desc() (from the PERFNFV-458 work), which only validates that a metric_desc's own default-aggregation field is one of the four known values at ingest time — it doesn't touch query-time --aggregation overrides, and even at ingest time it never cross-validates class against default-aggregation either.

What this issue is not asking for

Please don't read this as "restrict which aggregations a user can request per class." Part of the power of --aggregation is that a user can deliberately ask for something non-standard on a specific query — that's an intentional, valid use of the feature and shouldn't be locked down by class.

What this issue is asking for

A request that's not just non-standard but actually nonsensical for the metric's class — e.g. an average of boolean-class values — should produce a clear error instead of silently returning a number that doesn't mean anything. The user should still be able to ask for it if they have a real reason to (or the definition of "nonsensical" turns out to be too strict for some class), but today there's no signal at all that the combination was suspect.

Open design question

Which class/aggregation combinations actually count as nonsensical (vs. merely unusual) needs a real decision, not just this issue's initial framing — e.g. is avg of boolean always meaningless, or does it have a legitimate reading as "fraction true"? Does pass/fail behave the same way? This needs discussion with whoever owns the CDM metric model before implementing a specific rejection list.

Context

Found while adding crucible-ci integration test coverage for --aggregation (crucible-ci PR in progress, gated on CI_RELEASE since the flag itself only landed in commit 34539bb, 2026-07-30, postdating even the current 2026.3 release branch).

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    Status
    Queued

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions