diff --git a/CLAUDE.md b/CLAUDE.md index b4b397eb..620adce2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,7 +35,7 @@ Supporting document types: `param`, `tag`, `config_*` - Versions tracked as git branches and in `VERSION` file (currently `v10dev`) - `cdm.js` exports `supportedCdmVersions` array: `['v7dev', 'v8dev', 'v9dev', 'v10dev']` - Index naming pattern: `cdm{VERSION}-{DOCTYPE}*` (e.g., `cdmv10dev-metric_data*`) -- v10dev's key addition is `default-aggregation` — a per-metric field on `metric_desc` (`sum`/`avg`/`max`/`min`) telling query-time aggregation how to combine values across breakout dimensions, instead of always duration-weighted summing +- v10dev's key addition is `default-aggregation` — a per-metric field on `metric_desc` (`sum`/`avg`/`max`/`min`) telling query-time aggregation how to combine values across breakout dimensions, instead of always duration-weighted summing. Metric definitions may also provide `disallowed-aggregations` for combinations that are not meaningful for that specific metric. ## Templates (`templates/`) - Index mappings for every document type are defined in `queries/cdmq/cdm.js`'s `indexDefs` object (`v8dev` hand-written, `v9dev`/`v10dev` built forward via `deepClone`). This is the only live schema — `add-run.js`'s `checkCreateIndex()`/`updateIndexMappings()` use it to create/update OpenSearch indices, and also use it as a client-side validation gate, rejecting any document field not present in it before the document is ever sent to OpenSearch. diff --git a/queries/cdmq/README.md b/queries/cdmq/README.md index d434fa11..6feaeba7 100644 --- a/queries/cdmq/README.md +++ b/queries/cdmq/README.md @@ -32,6 +32,14 @@ Many of the scripts refer to different terms we associate with either running a Below are documented most common scripts used for this project. All of these scripts can be run via `node ./script-name.js`, and some have wrapper scripts `script-name.sh` which provide the casual user a more convenient invocation. If you are using [crucible](https://github.com/perftool-incubator/crucible), it may provide an alternative way to use this script (documented in each script subsection below). +### Aggregation overrides + +`--aggregation` overrides a metric's `default-aggregation`. Metric definitions +may declare combinations that are not meaningful for that metric using +`metric_desc.disallowed-aggregations`. Such a query returns an error by +default. Use `--allow-incompatible-aggregation` when the combination is +intentional and should still be evaluated. + ### get-result-summary.js This script produces a summary of a single run., including tags, metrics present, as well as all the iterations and their samples. To run this script, you must specify a run-id: `node ./get-result-summary.js --run 0bda53c3-f0b2-416a-be54-cee738b75010`. If you are using the crucible project, you will likely be using the crucible command-line `crucible get result --run 0bda53c3-f0b2-416a-be54-cee738b75010`. In this example, the following output is produced: diff --git a/queries/cdmq/cdm.js b/queries/cdmq/cdm.js index ea1136f6..66d5c787 100644 --- a/queries/cdmq/cdm.js +++ b/queries/cdmq/cdm.js @@ -382,6 +382,9 @@ indexDefs['v9dev']['metric_desc'] = deepClone(indexDefs['v8dev']['metric_desc']) indexDefs['v9dev']['metric_desc']['mappings']['properties']['metric_desc']['properties']['default-aggregation'] = { type: 'keyword' }; +indexDefs['v9dev']['metric_desc']['mappings']['properties']['metric_desc']['properties']['disallowed-aggregations'] = { + type: 'keyword' +}; // TODO: add new names for cdmv9 @@ -3490,6 +3493,40 @@ getDefaultAggregation = function (instance, run, source, type, yearDotMonth) { return 'sum'; }; +// A metric class alone is not sufficient to determine whether an aggregation is +// meaningful: for example, averaging boolean values can represent the true +// fraction. Metric producers can explicitly declare only combinations that are +// invalid for their metric, leaving unusual but intentional overrides to the +// caller's discretion. +getDisallowedAggregations = function (instance, run, source, type, yearDotMonth) { + var q = { + size: 10000, + _source: ['metric_desc.class', 'metric_desc.disallowed-aggregations'], + query: { + bool: { + filter: [ + { term: { 'run.run-uuid': run } }, + { term: { 'metric_desc.source': source } }, + { term: { 'metric_desc.type': type } } + ] + } + } + }; + var resp = esRequest(instance, 'metric_desc', '/_search', q, yearDotMonth); + var data = JSON.parse(resp.getBody()); + var result = []; + if (data.hits && data.hits.hits) { + data.hits.hits.forEach((hit) => { + var desc = hit._source && hit._source.metric_desc; + if (desc && Array.isArray(desc['disallowed-aggregations'])) { + result.push({ class: desc.class, aggregations: desc['disallowed-aggregations'] }); + } + }); + } + return result; +}; +exports.getDisallowedAggregations = getDisallowedAggregations; + // -------------------------------------------------------------------------------------------------------------- calcAvg = function ( thisBegin, @@ -3841,6 +3878,11 @@ getMetricDataSets = async function (instance, sets, yearDotMonth) { var retCode = 0; var retMsg = ''; for (var i = 0; i < sets.length; i++) { + // Keep the public request spelling hyphenated while accepting the internal + // camelCase form used by the query library during validation. + if (isDefined(sets[i]['allow-incompatible-aggregation'])) { + sets[i].allowIncompatibleAggregation = sets[i]['allow-incompatible-aggregation']; + } // If a begin and end are not defined, get it from the period.begin & period.end. // If a begin and/or end are not defined, and the period is not defined, error out. // If a run is not defined, get it from the period. @@ -3972,6 +4014,39 @@ getMetricDataSets = async function (instance, sets, yearDotMonth) { } var metricGroupIdsByLabelSets = resp['metric-id-sets']; + // Reject only combinations explicitly declared invalid by the metric + // definition. A caller can bypass this check when the unusual aggregation + // is intentional. + for (var idx = 0; idx < sets.length; idx++) { + if (sets[idx].aggregation && !sets[idx].allowIncompatibleAggregation) { + var disallowed = getDisallowedAggregations( + instance, + sets[idx].run, + sets[idx].source, + sets[idx].type, + yearDotMonth + ); + var invalid = disallowed.filter((entry) => entry.aggregations.includes(sets[idx].aggregation)); + if (invalid.length > 0) { + var classes = invalid + .map((entry) => entry.class) + .filter((value, position, values) => value && values.indexOf(value) === position) + .join(', '); + retMsg = + 'ERROR: aggregation [' + + sets[idx].aggregation + + '] is disallowed for [' + + sets[idx].source + + '::' + + sets[idx].type + + ']'; + if (classes) retMsg += ' (metric class: ' + classes + ')'; + retMsg += '. Use allow-incompatible-aggregation to run this query when the combination is intentional.'; + return { 'ret-code': 4, 'ret-msg': retMsg, code: 'INCOMPATIBLE_AGGREGATION' }; + } + } + } + // Check if any regex filters resulted in zero matches for (var idx = 0; idx < metricGroupIdsByLabelSets.length; idx++) { if (Object.keys(metricGroupIdsByLabelSets[idx]).length === 0) { diff --git a/queries/cdmq/get-metric-data.js b/queries/cdmq/get-metric-data.js index 73918df5..95790329 100644 --- a/queries/cdmq/get-metric-data.js +++ b/queries/cdmq/get-metric-data.js @@ -163,6 +163,10 @@ async function main() { '[optional] Filter out (do not output) metrics which do not pass the conditional. gt=greater-than, ge=greater-than-or-equal, lt=less-than, le=less-than-or-equal' ) .option('--aggregation ', '[optional] Override the default aggregation method for this query') + .option( + '--allow-incompatible-aggregation', + '[optional] Allow an aggregation explicitly disallowed by the metric definition' + ) .option('--output-format ', 'table') .option( '--date-format ', @@ -208,6 +212,7 @@ async function main() { breakout: program.breakout, // Send as array to preserve complex breakout syntax filter: program.filter, aggregation: program.aggregation, + 'allow-incompatible-aggregation': program.allowIncompatibleAggregation, instances: program.instances.length > 0 ? program.instances : undefined }; diff --git a/queries/cdmq/server.js b/queries/cdmq/server.js index 0ffce0d1..717ed635 100755 --- a/queries/cdmq/server.js +++ b/queries/cdmq/server.js @@ -1659,6 +1659,7 @@ app.post('/api/v1/metric-data', async (req, res) => { 'breakout', 'filter', 'aggregation', + 'allow-incompatible-aggregation', 'instances' ]; var fieldErr = validateBodyFields(req.body, knownFields); @@ -1676,6 +1677,7 @@ app.post('/api/v1/metric-data', async (req, res) => { breakout, filter, aggregation, + 'allow-incompatible-aggregation': allowIncompatibleAggregation, instances: reqInstances } = req.body; @@ -1781,12 +1783,13 @@ app.post('/api/v1/metric-data', async (req, res) => { resolution: resolution, breakout: breakout, filter: filter, - aggregation: aggregation + aggregation: aggregation, + allowIncompatibleAggregation: allowIncompatibleAggregation === true }; var resp = await cdm.getMetricDataSets(instance, [set], yearDotMonth); if (resp['ret-code'] != 0) { - return res.status(500).json({ - code: 'METRIC_QUERY_FAILED', + return res.status(resp.code === 'INCOMPATIBLE_AGGREGATION' ? 400 : 500).json({ + code: resp.code || 'METRIC_QUERY_FAILED', error: resp['ret-msg'] }); }