Preserve Kueue resource tags for other metrics - #24760
Conversation
Retain the originating resource tag when several resource types share the other metric name.
Document the resource tag preservation fix in the release notes.
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: a5746ca | Docs | Datadog PR Page | Give us feedback! |
Validation ReportAll 21 validations passed. Show details
|
There was a problem hiding this comment.
More details
The resource transformer preserves the original resource tag only for metrics normalized to other, while mapped resources such as GPU still omit that tag. Existing and adversarial executions covered unknown vendor resources and explicit other mappings without exposing a behavioral regression.
📊 Validated against 4 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit a5746ca · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| aggregator.assert_metric(metric) | ||
| aggregator.assert_metric_has_tag(metric, 'test:tag') | ||
|
|
||
| aggregator.assert_metric_has_tag('kueue.cluster_queue.resource_usage.other', 'resource:example.com/fpga') |
There was a problem hiding this comment.
suggestion (non-blocking): this proves the tag is preserved, but not the collision it guards against. The fixture has exactly one unmapped resource (example.com/fpga; cpu/memory/nvidia.com/gpu are all in RESOURCE_NAME_MAP), so kueue.cluster_queue.resource_usage.other only ever carries a single series here. The bug being fixed is that two distinct unmapped resources previously collapsed onto the same .other name with identical tags and overwrote each other (gauge is last-write-wins), and that path never fires with one resource. Add a second unmapped resource to a resource metric in metrics.txt (e.g. example.com/tpu) and assert ...resource_usage.other yields two series with distinct resource: tags. That exercises the no-collision property directly instead of inferring it from the single-series case.
|
|
||
| resource_tags = [tag for tag in tags if tag != f'resource:{resource}'] | ||
| resource_tags = tags | ||
| if resource_name != OTHER_RESOURCE_NAME: |
There was a problem hiding this comment.
suggestion (non-blocking): this closes the collision for the implicit other bucket, but the same class of bug survives for custom mappings. The guard keys off resource_name == 'other', so any resource_name_map that collapses two distinct resources onto one name still strips their resource: tags and emits colliding series. {'a.com/x': 'accel', 'b.com/y': 'accel'} → both normalize to accel at check.py:104, both take the != 'other' branch, both lose the distinguishing tag, second overwrites first. Scoping this PR to other is defensible since duplicate custom names are an unusual config. Worth either preserving the tag whenever the resolved name is non-injective, or noting in the config docs that custom names must be unique.
There was a problem hiding this comment.
For now we only have one resource type for non-other resources
PR #24760 added a third `_get_metric_tags` call site on master while this branch renamed the helper to `get_metric_tags`. The two changes touched different regions, so the merge was textually clean but left the new call site referencing the old name, failing lint with F821. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Fix Kueue metric metadata and add log collection - Correct metadata.csv for counter/histogram metrics: they are submitted as .count/.bucket/.sum (monotonic_count), not base-name gauges, so dashboards querying the base names showed no data. Corrects ~30 entries. - Rebuild tests/fixtures/metrics.txt to the real endpoint format for the affected families so unit tests reflect what Kueue actually exposes. - Strengthen tests so this can't regress: test_check runs the check twice so monotonic counters flush and the strict metadata assertion validates them; test_e2e uses rate=True so the e2e metadata check validates counters too. - Add Kueue log collection support (logs template in spec.yaml, regenerated conf.yaml.example, README section). - Expand the E2E environment to exercise cohort/fair-sharing, GPU, a custom resource, a not-active ClusterQueue, and preemption/eviction metrics. - Make the E2E env robust locally: choose non-colliding kind subnets and disable Kueue's visibility server, whose cert bootstrap crashloops some clusters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Add changelog entries for #24700 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Remove non-existent metric and fix preemption_skips type in Kueue metadata Verified every metric in metadata.csv against Kueue v0.18 source (pkg/metrics/metrics.go) and the live endpoint: - kueue.resource_flavor.quota_reserved_workloads does not exist in Kueue v0.18 (no resource_flavor metric is defined). Removed it from the metadata, metric map, test fixture, and expected-metrics list. - kueue.admission_cycle.preemption_skips is a Gauge, not a counter. Reverted it to the base gauge name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Collect kueue.finished_workloads.count via native_dynamic kueue_finished_workloads is a dual-form metric: Kueue exposes both a gauge (kueue_finished_workloads) and a counter (kueue_finished_workloads_total). The standard metric map collapsed the counter onto the gauge's name, so the counter was submitted as a gauge — polluting kueue.finished_workloads and never emitting a .count. Map it as native_dynamic (the same mechanism used for go.memstats.alloc_bytes and the local_queue transformer) so the gauge and the cumulative .count are both submitted correctly. Added the kueue.finished_workloads.count metadata row and fixture coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Restore original description quoting in Kueue metadata.csv 41 rows carried over from master differed only by having the surrounding double quotes stripped from the description column, with no change to the description text. Restoring the original quoting shrinks the diff for this file from 135 lines to 53, leaving only the substantive changes: the 22 base-name gauge rows replaced by 31 .count/.bucket/.sum count rows and the resource_flavor.quota_reserved_workloads removal. Parsing the file before and after with csv.reader yields identical rows, so this is a formatting-only change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop dead local_queue finished_workloads mapping and cover its counter The parser strips '_total' from counter families, so the kueue_local_queue_finished_workloads_total key was never looked up. Both forms already resolve correctly because the local-queue custom transformer wraps get_native_dynamic_transformer; add the resulting .count metric to the fixture assertions so that stays covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Request a 24h metrics-reader token so --dev sessions survive Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Document why both Kueue cert-bootstrap mitigations are needed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correct the stale reason for excluding counters from e2e assertions Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make the fixture histogram families well-formed The four families converted from gauges to histograms had no le="+Inf" bucket and had bucket lines on only the first series, a shape no real endpoint produces. Emit the full bucket set plus +Inf for every series so the +Inf submission path is exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Harden kind subnet selection and clean up its temp file Derive the prefix from the octet count so macOS netstat's abbreviated network routes (10 meaning 10.0.0.0/8) are not narrowed to /32 and missed. Log the chosen pair, including the fallback, since the subnets are the first thing to check when the controller crashloops. Turn the builder into a context manager so the rendered config is unlinked on teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Select the Kueue manager container by name when patching feature gates Patching containers/0 blind succeeds even if the flag lands on a sidecar, turning an upstream manifest change into a crashloop with an unrelated error. Resolve the index by container name and fail loudly if it is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Right-size the preemption scenario to 200m of quota Both preempt Jobs asked for a full core against a 1-core quota, so the admitted pod oversubscribed a 2-vCPU runner on top of kube-system and the existing workloads. Preemption is quota accounting, so the metrics are identical at a fifth of the resources and no pods are left Pending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Derive the e2e env coordinates instead of hardcoding them The ddev config path and kind cluster name both encoded py3.13-v0.18.0 as a literal, so bumping python or KUEUE_VERSION broke the workload-events test with FileNotFoundError. Build both from get_active_env(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Tighten remaining Kueue env hygiene Side-load the workload image once instead of pulling it from Docker Hub for every Job pod, explain why min_collection_interval dropped to 30, narrow the enabled integration frameworks to the one the tests exercise, and drive the workload-events check through run() so the test stops calling a private initialization directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix preempted_by tagging and rework the Kueue e2e test env Only derive `kueue_preempted_by` from evictions whose reason is `Preempted`. A `FlavorMigration` eviction carries a lookalike message ("Evicted to accommodate a workload (UID: ...)") that was being mined for a preemptor UID that does not exist. Also correct the log collection section of the README, extract the kubectl helpers into tests/kube.py, and extend the e2e coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove stale comment on the local queue finished_workloads mapping Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove comment on the preempting workload UID pattern Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove comment on the preemption reason gate Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue test metric lists Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue e2e tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue unit tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use the built-in tag assertion for the e2e metric tags `assert_metric_has_tags` covers every metric in `EXPECTED_METRIC_TAGS`. The custom helper is now reserved for the status families, whose tags carry no information on their own: a series exists for every state of every queue, and only the value says which state is current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Derive the Kueue version and workload images instead of duplicating them The version literal lived in both hatch.toml and a conftest fallback, and the workload image lived in a constant kept in sync with the Job manifests by a comment. Read both from their single source instead, so a bump has one place to change and a manifest using a different image is still preloaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop the custom series assertion in favour of assert_metric The status families needed a value paired with tags, which the custom helper provided by matching a tag subset. `assert_metric` covers it by matching the exact tag set, with the per-run endpoint tag taken from the saved instance config, so the tests now use only built-in assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue test constants Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Gate the Kueue workload-events e2e test through the dd_agent_check fixture Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue test conftest Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Simplify kind subnet selection in the Kueue test conftest Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Let the Kueue Workload discovery loop retry instead of raising on an empty result Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Restore Kueue metric-surface changelog coverage and harden the test env Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Clarify the two kueue_preempted_by changelog entries Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make kind image side-loading best effort in the Kueue test env kind load docker-image fails against node images whose containerd config version the installed kind does not support. The Job pods pull the image themselves in that case, so a failed side-load must not abort env start. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Tolerate a missing cluster when minting the Kueue metrics reader token The dd_environment body re-runs on the tear-down pass, where set_up_env is false so no cluster is available. A strict token mint aborted teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Update kueue/README.md Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Update kueue/README.md Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Update kueue/README.md Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Reconcile the metric tag helper rename after merging master PR #24760 added a third `_get_metric_tags` call site on master while this branch renamed the helper to `get_metric_tags`. The two changes touched different regions, so the merge was textually clean but left the new call site referencing the old name, failing lint with F821. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> f0987e3
* Fix Kueue metric metadata and add log collection - Correct metadata.csv for counter/histogram metrics: they are submitted as .count/.bucket/.sum (monotonic_count), not base-name gauges, so dashboards querying the base names showed no data. Corrects ~30 entries. - Rebuild tests/fixtures/metrics.txt to the real endpoint format for the affected families so unit tests reflect what Kueue actually exposes. - Strengthen tests so this can't regress: test_check runs the check twice so monotonic counters flush and the strict metadata assertion validates them; test_e2e uses rate=True so the e2e metadata check validates counters too. - Add Kueue log collection support (logs template in spec.yaml, regenerated conf.yaml.example, README section). - Expand the E2E environment to exercise cohort/fair-sharing, GPU, a custom resource, a not-active ClusterQueue, and preemption/eviction metrics. - Make the E2E env robust locally: choose non-colliding kind subnets and disable Kueue's visibility server, whose cert bootstrap crashloops some clusters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Add changelog entries for DataDog#24700 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Remove non-existent metric and fix preemption_skips type in Kueue metadata Verified every metric in metadata.csv against Kueue v0.18 source (pkg/metrics/metrics.go) and the live endpoint: - kueue.resource_flavor.quota_reserved_workloads does not exist in Kueue v0.18 (no resource_flavor metric is defined). Removed it from the metadata, metric map, test fixture, and expected-metrics list. - kueue.admission_cycle.preemption_skips is a Gauge, not a counter. Reverted it to the base gauge name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Collect kueue.finished_workloads.count via native_dynamic kueue_finished_workloads is a dual-form metric: Kueue exposes both a gauge (kueue_finished_workloads) and a counter (kueue_finished_workloads_total). The standard metric map collapsed the counter onto the gauge's name, so the counter was submitted as a gauge — polluting kueue.finished_workloads and never emitting a .count. Map it as native_dynamic (the same mechanism used for go.memstats.alloc_bytes and the local_queue transformer) so the gauge and the cumulative .count are both submitted correctly. Added the kueue.finished_workloads.count metadata row and fixture coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Restore original description quoting in Kueue metadata.csv 41 rows carried over from master differed only by having the surrounding double quotes stripped from the description column, with no change to the description text. Restoring the original quoting shrinks the diff for this file from 135 lines to 53, leaving only the substantive changes: the 22 base-name gauge rows replaced by 31 .count/.bucket/.sum count rows and the resource_flavor.quota_reserved_workloads removal. Parsing the file before and after with csv.reader yields identical rows, so this is a formatting-only change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop dead local_queue finished_workloads mapping and cover its counter The parser strips '_total' from counter families, so the kueue_local_queue_finished_workloads_total key was never looked up. Both forms already resolve correctly because the local-queue custom transformer wraps get_native_dynamic_transformer; add the resulting .count metric to the fixture assertions so that stays covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Request a 24h metrics-reader token so --dev sessions survive Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Document why both Kueue cert-bootstrap mitigations are needed Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correct the stale reason for excluding counters from e2e assertions Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make the fixture histogram families well-formed The four families converted from gauges to histograms had no le="+Inf" bucket and had bucket lines on only the first series, a shape no real endpoint produces. Emit the full bucket set plus +Inf for every series so the +Inf submission path is exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Harden kind subnet selection and clean up its temp file Derive the prefix from the octet count so macOS netstat's abbreviated network routes (10 meaning 10.0.0.0/8) are not narrowed to /32 and missed. Log the chosen pair, including the fallback, since the subnets are the first thing to check when the controller crashloops. Turn the builder into a context manager so the rendered config is unlinked on teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Select the Kueue manager container by name when patching feature gates Patching containers/0 blind succeeds even if the flag lands on a sidecar, turning an upstream manifest change into a crashloop with an unrelated error. Resolve the index by container name and fail loudly if it is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Right-size the preemption scenario to 200m of quota Both preempt Jobs asked for a full core against a 1-core quota, so the admitted pod oversubscribed a 2-vCPU runner on top of kube-system and the existing workloads. Preemption is quota accounting, so the metrics are identical at a fifth of the resources and no pods are left Pending. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Derive the e2e env coordinates instead of hardcoding them The ddev config path and kind cluster name both encoded py3.13-v0.18.0 as a literal, so bumping python or KUEUE_VERSION broke the workload-events test with FileNotFoundError. Build both from get_active_env(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Tighten remaining Kueue env hygiene Side-load the workload image once instead of pulling it from Docker Hub for every Job pod, explain why min_collection_interval dropped to 30, narrow the enabled integration frameworks to the one the tests exercise, and drive the workload-events check through run() so the test stops calling a private initialization directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix preempted_by tagging and rework the Kueue e2e test env Only derive `kueue_preempted_by` from evictions whose reason is `Preempted`. A `FlavorMigration` eviction carries a lookalike message ("Evicted to accommodate a workload (UID: ...)") that was being mined for a preemptor UID that does not exist. Also correct the log collection section of the README, extract the kubectl helpers into tests/kube.py, and extend the e2e coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove stale comment on the local queue finished_workloads mapping Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove comment on the preempting workload UID pattern Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove comment on the preemption reason gate Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue test metric lists Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue e2e tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue unit tests Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use the built-in tag assertion for the e2e metric tags `assert_metric_has_tags` covers every metric in `EXPECTED_METRIC_TAGS`. The custom helper is now reserved for the status families, whose tags carry no information on their own: a series exists for every state of every queue, and only the value says which state is current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Derive the Kueue version and workload images instead of duplicating them The version literal lived in both hatch.toml and a conftest fallback, and the workload image lived in a constant kept in sync with the Job manifests by a comment. Read both from their single source instead, so a bump has one place to change and a manifest using a different image is still preloaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop the custom series assertion in favour of assert_metric The status families needed a value paired with tags, which the custom helper provided by matching a tag subset. `assert_metric` covers it by matching the exact tag set, with the per-run endpoint tag taken from the saved instance config, so the tests now use only built-in assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue test constants Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Gate the Kueue workload-events e2e test through the dd_agent_check fixture Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Remove explanatory comments from the Kueue test conftest Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Simplify kind subnet selection in the Kueue test conftest Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Let the Kueue Workload discovery loop retry instead of raising on an empty result Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Restore Kueue metric-surface changelog coverage and harden the test env Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Clarify the two kueue_preempted_by changelog entries Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make kind image side-loading best effort in the Kueue test env kind load docker-image fails against node images whose containerd config version the installed kind does not support. The Job pods pull the image themselves in that case, so a failed side-load must not abort env start. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Tolerate a missing cluster when minting the Kueue metrics reader token The dd_environment body re-runs on the tear-down pass, where set_up_env is false so no cluster is available. A strict token mint aborted teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Update kueue/README.md Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Update kueue/README.md Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Update kueue/README.md Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Reconcile the metric tag helper rename after merging master PR DataDog#24760 added a third `_get_metric_tags` call site on master while this branch renamed the helper to `get_metric_tags`. The two changes touched different regions, so the merge was textually clean but left the new call site referencing the old name, failing lint with F821. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
What does this PR do?
Retains the original resource tag on Kueue metrics emitted as
other.Motivation
Unmapped resources otherwise cannot be distinguished after sharing the
othermetric name.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged