Skip to content

Fix job execution duration when runner assign time is not set #4472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikola-jokic wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −1
Open

Fix job execution duration when runner assign time is not set #4472

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cmd/ghalistener/metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,13 @@ func (e *exporter) RecordJobCompleted(msg *scaleset.JobCompleted) {
l := e.completedJobLabels(msg)
e.incCounter(MetricCompletedJobsTotal, l)

executionDuration := msg.FinishTime.Unix() - msg.RunnerAssignTime.Unix()
// If the runner assign time is not set, it means that the job is never picked up by the runner.
// Set execution duration to 0, to observe that the job is completed without being executed.
Comment on lines +499 to +500
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

The added comment is a bit misleading/grammatically off: this case means the job was never picked up by a runner, and the wording “Set execution duration to 0, to observe…” reads awkwardly. Consider rephrasing to past tense and clarifying that there was no execution phase (especially if you keep recording 0s).

Suggested change
// If the runner assign time is not set, it means that the job is never picked up by the runner.
// Set execution duration to 0, to observe that the job is completed without being executed.
// If the runner assign time is not set, the job was never picked up by a runner.
// Leave execution duration at 0 to record that the job completed without an execution phase.

Copilot uses AI. Check for mistakes.
var executionDuration int64
if !msg.RunnerAssignTime.IsZero() {
executionDuration = msg.FinishTime.Unix() - msg.RunnerAssignTime.Unix()
}

Comment on lines +499 to +505
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Recording an execution duration of 0 when RunnerAssignTime is zero makes the gha_job_execution_duration_seconds histogram include jobs that never actually executed on a runner. This will show up as a spike in the smallest bucket (<=0.01s) and can still skew the bundled Grafana heatmap which sums buckets without filtering job_result (see docs/gha-runner-scale-set-controller/samples/grafana-dashboard/ARC-Autoscaling-Runner-Set-Monitoring.json:256). Consider skipping the histogram observation entirely when RunnerAssignTime.IsZero() (early return after incrementing the completed counter), or emit a separate metric for “completed before runner assignment” instead of encoding it as 0s execution time.

Suggested change
// If the runner assign time is not set, it means that the job is never picked up by the runner.
// Set execution duration to 0, to observe that the job is completed without being executed.
var executionDuration int64
if !msg.RunnerAssignTime.IsZero() {
executionDuration = msg.FinishTime.Unix() - msg.RunnerAssignTime.Unix()
}
// If the runner assign time is not set, it means that the job was never picked up by a runner.
// Skip recording execution duration so jobs that never executed do not pollute the histogram.
if msg.RunnerAssignTime.IsZero() {
return
}
executionDuration := msg.FinishTime.Unix() - msg.RunnerAssignTime.Unix()

Copilot uses AI. Check for mistakes.
e.observeHistogram(MetricJobExecutionDurationSeconds, l, float64(executionDuration))
Comment on lines +501 to 506
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

This change fixes a production-facing metric edge case, but there are currently no unit tests covering RecordJobCompleted. Please add a test for the zero RunnerAssignTime scenario to ensure we don’t regress back to recording an epoch-sized duration (and to lock in the intended behavior: either skipping the observation or recording a specific value).

Copilot uses AI. Check for mistakes.
}

Expand Down
Loading