Remove value field from agg:#323
Conversation
Signed-off-by: Raul Sevilla <rsevilla@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull request overview
This PR aligns aggregation handling around metric_of_interest by removing the redundant agg.value field from metric configurations, and updates the query/aggregation code plus docs/examples accordingly.
Changes:
- Update aggregation query + parsing to name aggregations using
metric_of_interestinstead ofagg.value. - Remove
agg.valuefrom CI configuration metrics and many example metric YAMLs. - Refresh documentation examples to match the new aggregation config shape.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| orion/utils.py | Updates aggregation processing to use metric_of_interest; docstring updated. |
| orion/matcher.py | Updates OpenSearch aggregation naming/parsing to use metric_of_interest. |
| hack/ci-tests/configurations/metrics.yaml | Removes agg.value from CI metrics config. |
| hack/ci-tests/configurations/local_metrics.yaml | Removes agg.value from local CI metrics config. |
| hack/ci-tests/configurations/ci-tests.yaml | Removes agg.value from CI tests config. |
| hack/ci-tests/configurations/ci-tests-metrics-only.yaml | Removes agg.value from metrics-only CI config. |
| hack/ci-tests/configurations/ci-tests-inherits.yaml | Removes agg.value from inherited CI config. |
| hack/ci-tests/configurations/ci-tests-inherits-local-metrics.yaml | Removes agg.value from inherited local-metrics CI config. |
| hack/ci-tests/configurations/ci-tests-inherits-local-metrics-with-ignore.yaml | Removes agg.value from inherited local-metrics-with-ignore CI config. |
| hack/ci-tests/configurations/ci-tests-inherits-local-metadata.yaml | Removes agg.value from inherited local-metadata CI config. |
| hack/ci-tests/configurations/ci-tests-inherits-ignore-global.yaml | Removes agg.value from inherited ignore-global CI config. |
| hack/ci-tests/configurations/ci-tests-early-cp.yaml | Removes agg.value from early-cp CI config. |
| hack/ci-tests/configurations/ci-tests-browbeat.yaml | Removes agg.value / some percentile percents entries from browbeat CI config. |
| examples/trt-external-payload-udn-l3.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-udn-l2.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-node-density.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-node-density-inherits.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-node-density-cni.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-crd-scale.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-cpu.yaml | Removes agg.value from example metrics. |
| examples/trt-external-payload-cluster-density.yaml | Removes agg.value from example metrics. |
| examples/small-scale-udn-l3.yaml | Removes agg.value from example metrics. |
| examples/small-scale-udn-l2.yaml | Removes agg.value from example metrics. |
| examples/small-scale-node-density-cni.yaml | Removes agg.value from example metrics. |
| examples/small-scale-cluster-density.yaml | Removes agg.value from example metrics. |
| examples/small-scale-cluster-density-report.yaml | Removes agg.value from report example metrics. |
| examples/small-rosa-control-plane-node-density.yaml | Removes agg.value from example metrics. |
| examples/servicemesh-netperf-tcp.yaml | Removes agg.value from netperf example metrics. |
| examples/servicemesh-ingress.yaml | Removes agg.value from ingress example metrics. |
| examples/readout-netperf-tcp.yaml | Removes agg.value from readout netperf example metrics. |
| examples/readout-ingress.yaml | Removes agg.value from readout ingress example metrics. |
| examples/readout-control-plane-node-density.yaml | Removes agg.value from readout node-density example metrics. |
| examples/readout-control-plane-cdv2.yaml | Removes agg.value from readout cdv2 example metrics. |
| examples/quay-load-test-stable.yaml | Removes agg.value from quay load-test example metrics. |
| examples/quay-load-test-stable-stage.yaml | Removes agg.value from quay stage example metrics. |
| examples/pod_disruption_scenarios.yaml | Removes agg.value from disruption scenario example metrics. |
| examples/payload-scale.yaml | Removes agg.value from payload-scale example metrics. |
| examples/ols-load-generator.yaml | Removes agg.value from OLS load-generator example metrics. |
| examples/olmv1.yaml | Removes agg.value from OLMv1 example metrics. |
| examples/okd-data-plane-data-path.yaml | Removes agg.value from OKD datapath example metrics. |
| examples/okd-control-plane-node-density.yaml | Removes agg.value from OKD node-density example metrics. |
| examples/okd-control-plane-node-density-cni.yaml | Removes agg.value from OKD node-density-cni example metrics. |
| examples/okd-control-plane-crd-scale.yaml | Removes agg.value from OKD crd-scale example metrics. |
| examples/okd-control-plane-cluster-density.yaml | Removes agg.value from OKD cluster-density example metrics. |
| examples/node_scenarios.yaml | Removes agg.value from node scenario example metrics. |
| examples/netobserv-diff-meta-index.yaml | Removes agg.value from netobserv example metrics. |
| examples/metrics/chaos-scenarios-metrics.yaml | Removes agg.value from chaos-scenarios metrics examples. |
| examples/metrics.yaml | Removes agg.value from metrics examples. |
| examples/metal-perfscale-cpt-virt-udn-density.yaml | Removes agg.value from metal perfscale virt UDN density example metrics. |
| examples/metal-perfscale-cpt-virt-density.yaml | Removes agg.value from metal perfscale virt density example metrics. |
| examples/metal-perfscale-cpt-virt-density-nomount.yaml | Removes agg.value from metal perfscale virt density (nomount) example metrics. |
| examples/metal-perfscale-cpt-virt-datapath.yaml | Removes agg.value from metal perfscale virt datapath example metrics. |
| examples/metal-perfscale-cpt-rds-core.yaml | Removes agg.value from metal perfscale rds-core example metrics. |
| examples/metal-perfscale-cpt-node-density.yaml | Removes agg.value from metal perfscale node-density example metrics. |
| examples/metal-perfscale-cpt-node-density-heavy.yaml | Removes agg.value from metal perfscale node-density-heavy example metrics. |
| examples/metal-perfscale-cpt-ingress.yaml | Removes agg.value from metal perfscale ingress example metrics. |
| examples/metal-perfscale-cpt-data-path.yaml | Removes agg.value from metal perfscale data-path example metrics. |
| examples/med-scale-udn-l3.yaml | Removes agg.value from medium-scale UDN L3 example metrics. |
| examples/med-scale-udn-l2.yaml | Removes agg.value from medium-scale UDN L2 example metrics. |
| examples/large-scale-udn-l3.yaml | Removes agg.value from large-scale UDN L3 example metrics. |
| examples/large-scale-udn-l2.yaml | Removes agg.value from large-scale UDN L2 example metrics. |
| examples/label-small-scale-cluster-density.yaml | Removes agg.value from label-based small-scale example metrics. |
| examples/chaos_tests.yaml | Removes agg.value from chaos tests example metrics. |
| docs/usage.md | Updates docs examples by removing agg.value. |
| docs/configuration.md | Updates aggregation configuration docs/examples by removing agg.value. |
Comments suppressed due to low confidence (1)
orion/matcher.py:362
- This change switches the aggregation name/key from
metrics['agg']['value']tometrics['metric_of_interest'], which changes the shape ofget_agg_metric_query/parse_agg_resultsoutputs (and breaks existing unit tests and any configs still providingagg.value). Consider keeping backward compatibility by usingmetrics['agg'].get('value', metrics['metric_of_interest'])as the aggregation name while still usingmetric_of_interestas the field to aggregate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| """Returns a dataframe of the aggregation metric | ||
|
|
||
| Args: | ||
| uuids (List[str]): _description_ | ||
| metric (Dict[str, Any]): _description_ | ||
| match (Matcher): _description_ | ||
| uuids (List[str]): list of uuids |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| - name: keystone_v3_list_users_P90 | ||
| metric_of_interest: raw | ||
| action.keyword: "keystone_v3.list_users" | ||
| threshold: 15 | ||
| direction: 1 | ||
| agg: | ||
| value: raw | ||
| agg_type: percentiles | ||
| percents: [90] | ||
| target_percentile: 90 | ||
|
|
There was a problem hiding this comment.
@copilot apply changes based on this feedback
82d857e to
43cbccf
Compare
Type of change
Description
This field is confusing and not actually necessary, it is used to name the OS/ES metric name, however, we can just use metric_of_interest instead with the same result
Related Tickets & Documents