Skip to content

Label Task completion and duration metrics by spawner - #1690

Open
knechtionscoding wants to merge 1 commit into
kelos-dev:mainfrom
datagravity-ai:feat/task-metrics-spawner-label
Open

Label Task completion and duration metrics by spawner#1690
knechtionscoding wants to merge 1 commit into
kelos-dev:mainfrom
datagravity-ai:feat/task-metrics-spawner-label

Conversation

@knechtionscoding

@knechtionscoding knechtionscoding commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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_total and kelos_task_output_tokens_total already carry a spawner label, so spend can be attributed per TaskSpawner. kelos_task_created_total, kelos_task_completed_total and kelos_task_duration_seconds do not, so there is no way to compute a per-spawner success rate or latency from metrics at all. This PR adds spawner to those three, making queries like the following meaningful:

sum(rate(kelos_task_completed_total{spawner="my-spawner"}[1w])) by (phase)

spawner is placed directly after type, 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 resolveTaskSpawner helper next to resolveTaskType in job_builder.go, which reads the existing exported taskbuilder.SpawnerLabel constant rather than respelling the key. RecordCostTokenMetrics now uses the helper too, so there is exactly one definition of which spawner owns a Task instead of a bare kelos.dev/taskspawner string literal in the metrics path. The remaining literal in job_builder.go, which injects KELOS_TASKSPAWNER into the agent pod, goes through the same helper.

taskCompletedTotal and taskDurationSeconds take 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 in recordTaskCompleted and observeTaskDuration next to the metric definitions in metrics.go. These deliberately stay two separate helpers rather than one combined recordTaskCompletionMetrics: in TaskReconciler.updateStatus the 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.completeTask sets the terminal phase, sets the completion time and records cost/token metrics, but never increments kelos_task_completed_total and never observes kelos_task_duration_seconds. Every Task served by a WorkerPool was therefore entirely absent from both metrics. completeTask now records both. Adding the spawner label 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_total is not comparable to completed_total for WorkerPool Tasks. kelos_task_created_total fires only in TaskReconciler.createJob, so WorkerPool-backed Tasks — which now do increment kelos_task_completed_total thanks 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.md now states this under the metrics table. Making created_total cover 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. RecordCostTokenMetrics in completeTask is unguarded, so a second completion of an already-terminal Task whose pod logs are still readable re-adds cost and tokens. The alreadyTerminal flag added here would suppress it, but the fix is not quite that simple: unlike the Job path — which bounds re-recording via retryOutputs, gated on len(Status.Results) == 0 — the WorkerPool path has no late-outputs retry, so a plain !alreadyTerminal guard would also skip cost recording in the case where the second pass is the first one to capture results at all, leaving Status.Usage populated 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. spawner is 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/taskspawner label and report spawner="". 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 TaskReconciler paths guard their increments with if task.Status.Phase != <terminal>, so completeTask needs an equivalent. It captures isTerminalTaskPhase(task.Status.Phase) inside the RetryOnConflict closure, immediately after the Get and 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 when Status.StartTime is nil, matching the TaskReconciler behaviour.

Note on reachability: the phase switch in Reconcile routes Succeeded/Failed away from monitorTaskCompletion, so the guard is not load-bearing in the common flow. It matters because that switch reads a possibly-stale cached Task while completeTask re-Gets from the API server, so a terminal Task can still reach it.

The guard uses isTerminalTaskPhase, which is intentionally broader than the TaskReconciler's per-phase checks: a Succeeded -> Failed correction 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_CompleteTaskRecordsCompletionMetrics asserts the counter increments exactly once and the histogram records exactly one observation for a Task completed via completeTask, 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 !alreadyTerminal guard with an unconditional increment fails the second. A second test covers a Task with no StartTime, which must still count as completed but record no duration. The existing metrics_test.go label arities were updated with a real spawner value rather than "", so an arity regression fails loudly.

make update regenerated nothing. make verify, make test and make test-integration all pass locally.

Does this PR introduce a user-facing change?

The `kelos_task_created_total`, `kelos_task_completed_total` and `kelos_task_duration_seconds` metrics now carry a `spawner` label identifying the TaskSpawner that created the Task, matching the existing cost and token metrics, so per-spawner success rate and latency can be queried. Tasks not created by a spawner report an empty `spawner` value. Note that adding this label changes the series identity of these three metrics, so any queries, dashboards or alerting rules selecting on them may need updating. Additionally, Tasks completed by a WorkerPool are now counted in `kelos_task_completed_total` and `kelos_task_duration_seconds`; previously they were omitted from both entirely.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread internal/controller/job_builder.go Outdated
Comment thread internal/controller/job_builder.go Outdated
Comment thread internal/controller/workerpool_controller.go Outdated
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

Copy link
Copy Markdown
Contributor Author

@gjkim42 this should be ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature needs-actor needs-priority needs-triage release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant