Label Task completion and duration metrics by spawner - #1690
Open
knechtionscoding wants to merge 1 commit into
Open
Label Task completion and duration metrics by spawner#1690knechtionscoding wants to merge 1 commit into
knechtionscoding wants to merge 1 commit into
Conversation
knechtionscoding
had a problem deploying
to
ok-to-test
August 25, 2026 15:42 — with
GitHub Actions
Error
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
knechtionscoding
force-pushed
the
feat/task-metrics-spawner-label
branch
from
August 25, 2026 16:12
c2faf5f to
85aad5a
Compare
knechtionscoding
had a problem deploying
to
ok-to-test
August 25, 2026 16:12 — with
GitHub Actions
Error
The cost and token metrics already carry a spawner label, but the task created, completed and duration metrics did not, so per-spawner success rate and latency could not be computed from metrics at all. Add a spawner label to kelos_task_created_total, kelos_task_completed_total and kelos_task_duration_seconds, in the same position the cost metrics use, and derive it everywhere through a new resolveTaskSpawner helper that reads the existing taskbuilder.SpawnerLabel constant, so there is a single definition of which spawner owns a Task. Tasks created manually or through the API report an empty spawner, matching the existing cost metrics. Because taskCompletedTotal and taskDurationSeconds take positional label values, build their label tuples in recordTaskCompleted and observeTaskDuration next to the metric definitions rather than respelling them at each call site, where a future reordering of the label slices would silently mislabel series. Also fix the WorkerPool completion path, which set the terminal phase and recorded cost metrics but never incremented kelos_task_completed_total or observed kelos_task_duration_seconds, leaving every Task served by a WorkerPool absent from both. completeTask now records them on the transition into a terminal phase, guarding against double counting on a re-reconcile and skipping the duration observation when no start time was recorded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
knechtionscoding
force-pushed
the
feat/task-metrics-spawner-label
branch
from
August 26, 2026 14:15
85aad5a to
4c6b65d
Compare
knechtionscoding
requested a deployment
to
ok-to-test
August 26, 2026 14:16 — with
GitHub Actions
Waiting
Contributor
Author
|
@gjkim42 this should be ready for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
kelos_task_cost_usd_total,kelos_task_input_tokens_totalandkelos_task_output_tokens_totalalready carry aspawnerlabel, so spend can be attributed per TaskSpawner.kelos_task_created_total,kelos_task_completed_totalandkelos_task_duration_secondsdo not, so there is no way to compute a per-spawner success rate or latency from metrics at all. This PR addsspawnerto those three, making queries like the following meaningful:spawneris placed directly aftertype, the same position the cost metrics use, so all six task metrics read consistently and the three task counters stay mutually joinable — a created-vs-completed reconciliation per spawner is how you spot Tasks that never reach a terminal phase.The label is derived through a new
resolveTaskSpawnerhelper next toresolveTaskTypeinjob_builder.go, which reads the existing exportedtaskbuilder.SpawnerLabelconstant rather than respelling the key.RecordCostTokenMetricsnow uses the helper too, so there is exactly one definition of which spawner owns a Task instead of a barekelos.dev/taskspawnerstring literal in the metrics path. The remaining literal injob_builder.go, which injectsKELOS_TASKSPAWNERinto the agent pod, goes through the same helper.taskCompletedTotalandtaskDurationSecondstake positional label values, so a future reordering of their label slices would silently mislabel every series if the tuple were respelled at each of the four call sites. The tuples are therefore built inrecordTaskCompletedandobserveTaskDurationnext to the metric definitions inmetrics.go. These deliberately stay two separate helpers rather than one combinedrecordTaskCompletionMetrics: inTaskReconciler.updateStatusthe counter increment happens during phase determination (before the status update) while the duration observation happens after it, so a combined helper would require moving the increment across the status update — a control-flow change to the reconciler that is out of scope here. Each call site keeps its original position.While tracing the metric paths, a second and larger gap turned up, fixed here in the same change:
WorkerPoolReconciler.completeTasksets the terminal phase, sets the completion time and records cost/token metrics, but never incrementskelos_task_completed_totaland never observeskelos_task_duration_seconds. Every Task served by a WorkerPool was therefore entirely absent from both metrics.completeTasknow records both. Adding thespawnerlabel without fixing this would have produced per-spawner dashboards that silently omit all WorkerPool-backed Tasks.Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
created_totalis not comparable tocompleted_totalfor WorkerPool Tasks.kelos_task_created_totalfires only inTaskReconciler.createJob, so WorkerPool-backed Tasks — which now do incrementkelos_task_completed_totalthanks to the fix above — are still absent from it. A created-vs-completed reconciliation will therefore show completed > created for any spawner routing to a WorkerPool.docs/reference.mdnow states this under the metrics table. Makingcreated_totalcover WorkerPool Tasks would change what the metric means (there is no Job to create), so it is left for a separate change.Known pre-existing issue, deliberately not fixed here.
RecordCostTokenMetricsincompleteTaskis unguarded, so a second completion of an already-terminal Task whose pod logs are still readable re-adds cost and tokens. ThealreadyTerminalflag added here would suppress it, but the fix is not quite that simple: unlike the Job path — which bounds re-recording viaretryOutputs, gated onlen(Status.Results) == 0— the WorkerPool path has no late-outputs retry, so a plain!alreadyTerminalguard would also skip cost recording in the case where the second pass is the first one to capture results at all, leavingStatus.Usagepopulated but the cost counters not. Since this touches budget accounting rather than the labels this PR is about, it is left for a follow-up with its own test.Cardinality is bounded.
spawneris the name of a TaskSpawner in the cluster, so the added dimension is the number of spawners — on the order of tens — times the existing worker types and phases. No cardinality mitigation is needed.Series identity changes. Adding a label changes the series identity of the three metrics, so any existing recording rule, alert or dashboard selecting on them would need updating. No compatibility shim or duplicate unlabelled metric is included here deliberately, on the assumption these metrics have no rule/dashboard consumers in tree; please flag if that assumption is wrong for your deployments.
Empty spawner is intentional. Tasks created manually or through the API carry no
kelos.dev/taskspawnerlabel and reportspawner="". This matches exactly what the cost and token metrics already do; substituting"none"or"unknown"would make the two metric families disagree for the same Task.Double-counting in the WorkerPool path. The
TaskReconcilerpaths guard their increments withif task.Status.Phase != <terminal>, socompleteTaskneeds an equivalent. It capturesisTerminalTaskPhase(task.Status.Phase)inside theRetryOnConflictclosure, immediately after theGetand before the phase is overwritten, and only records when the Task was not already terminal. Assigning inside the closure rather than before it keeps the value consistent with the attempt that actually persisted the status update. The duration observation is additionally skipped whenStatus.StartTimeis nil, matching theTaskReconcilerbehaviour.Note on reachability: the phase switch in
ReconcileroutesSucceeded/Failedaway frommonitorTaskCompletion, so the guard is not load-bearing in the common flow. It matters because that switch reads a possibly-stale cached Task whilecompleteTaskre-Gets from the API server, so a terminal Task can still reach it.The guard uses
isTerminalTaskPhase, which is intentionally broader than theTaskReconciler's per-phase checks: aSucceeded->Failedcorrection records nothing here, whereas the Job path would count a second completion for the same Task. Keeping one completion count per Task avoids inflating the denominator of a success-rate query, which is the point of the PR; the code comment records this as a decision.Tests.
TestWorkerPoolReconciler_CompleteTaskRecordsCompletionMetricsasserts the counter increments exactly once and the histogram records exactly one observation for a Task completed viacompleteTask, and that neither moves on a second call for an already-terminal Task. Both halves were checked to fail without the corresponding production change: removing the metric block fails the first assertions, and replacing the!alreadyTerminalguard with an unconditional increment fails the second. A second test covers a Task with noStartTime, which must still count as completed but record no duration. The existingmetrics_test.golabel arities were updated with a real spawner value rather than"", so an arity regression fails loudly.make updateregenerated nothing.make verify,make testandmake test-integrationall pass locally.Does this PR introduce a user-facing change?