Fix Kueue metric metadata and add log collection - #24700
Conversation
- 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>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 92dddc7 | Docs | Datadog PR Page | Give us feedback! |
…adata 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>
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>
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>
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>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
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>
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>
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>
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>
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>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`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>
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>
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>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
More details
The changed Kueue metric mappings and workload-event paths behaved correctly across the exercised counter, histogram, duplicate, namespace, eviction, and fallback scenarios. The repository kind E2E harness exited without producing a result file in this sandbox, so live-controller validation remains the only gap.
📊 Validated against 8 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 7a23490 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a234909b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - template: logs | ||
| example: | ||
| - type: docker | ||
| source: kueue | ||
| service: <SERVICE> |
There was a problem hiding this comment.
I assume this has no effect on k8s deployments right?
There was a problem hiding this comment.
Yes, this doesn’t affect Kubernetes deployments. I used type: docker because that is the standard log example defined in other integrations too, including Kubernetes-based ones like Kuma.
The README has a more detailed example of how to set them up for Kueue.
There was a problem hiding this comment.
Sounds good, although I'm not sure if Kueue can work outside of k8s. In any case, not blocking the PR on this.
There was a problem hiding this comment.
I could change it to type: file; the issue is that the template requires a type.
There was a problem hiding this comment.
I think we can leave it like this.
buraizu
left a comment
There was a problem hiding this comment.
Looks good overall, just requesting a few minor updates
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
Review from gjulianm is dismissed. Related teams and files:
- gpu-monitoring-agent
- kueue/README.md
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
Review from gjulianm is dismissed. Related teams and files:
- gpu-monitoring-agent
- kueue/README.md
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>
Validation ReportAll 21 validations passed. Show details
|
* 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
What does this PR do?
Fixes the Kueue integration's metric metadata and adds log-collection support.
metadata.csvlisted ~30 counter/histogram metrics under their base names asgauge(e.g.kueue.controller.runtime.reconcile,kueue.controller.runtime.reconcile_time.seconds,kueue.process.cpu.seconds, theworkqueue.*family). The check actually submits them as monotonic counters/histograms —.count/.bucket/.sum— so dashboards querying the base names showed no data. Corrected the entries to the real submitted names andcounttype.native_dynamic): Kueue exposes a few metrics as both a gauge and a_totalcounter (go.memstats.alloc_bytes,kueue.finished_workloads). The standard metric map collapsed the counter onto the gauge's name, so the counter was submitted as a gauge — polluting the gauge and never emitting a.count. Mapped these with thenative_dynamictype so both the gauge and its.countare submitted correctly, and added the missing.countmetadata rows.metadata.csvagainst Kueue v0.18 source (pkg/metrics/metrics.go) and the live endpoint. Fixedadmission_cycle.preemption_skips(it is aGauge, not a counter) and removedresource_flavor.quota_reserved_workloads, which Kueue v0.18 does not define.tests/fixtures/metrics.txtso the affected families use the real endpoint format,test_checknow runs the check twice (so OpenMetrics monotonic counters flush their.count) under the strict two-way metadata assertion, andtest_e2eusesrate=Trueso the e2e metadata assertion validates counters/histograms against the live endpoint.logstemplate tospec.yaml(regeneratedconf.yaml.example) and a "Log collection" section to the README.other) resource, a not-active ClusterQueue, and preemption / eviction.Motivation
Several Kueue dashboard widgets (reconciles, reconcile time, process CPU, workqueue, …) showed no data because the metadata metric names/types didn't match what the Agent submits. The metadata and the test fixture were a matched-but-wrong pair, and the existing metadata tests didn't catch it (the unit test stripped
.count/.bucket/.sumsuffixes; the e2e never flushed counters). This corrects the metadata, closes the test gap, and rounds out the integration's metric and log coverage.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🤖 Generated with Claude Code