Uh oh!
There was an error while loading. Please reload this page.
fix: update queue duration calculation - #752
Conversation
- Parse queued_at as optional in github_client._to_job_info() - Parse started_at as optional in github_client._to_job_info() - Include queued_at when creating JobInfo in github_provider - Mark queued_at as Optional in both JobInfo dataclasses - Handle None started_at when calculating queue_duration
TICS Quality Gate✔️ Passedgithub-runner-operatorSee the results in the TICS Viewer The following files have been checked for this project
|
yhaliaw
left a comment
There was a problem hiding this comment.
Approved with minor changes needed
Note: the queued_at field may already exist on the GitHub Jobs API (mentioned previously) — if so, disregard the comment questioning whether it exists.
🤝 Human review with AI assistance.
| queued_at_raw = job.get("queued_at") | ||
| queued_at = ( | ||
| datetime.fromisoformat(queued_at_raw.replace("Z", "+00:00")) | ||
| if queued_at_raw | ||
| else None | ||
| ) |
There was a problem hiding this comment.
The GitHub Actions Jobs REST API ("Get a job for a workflow run") doesn't return a queued_at field at all — its response schema only has created_at/started_at/completed_at. That means job.get("queued_at") here always evaluates to None in production.
As a result, queued_or_created_at = job_info.queued_at or job_info.created_at in metrics/github.py:53 will always fall back to created_at, so the queue-duration value is unchanged from before this PR. The PR's stated goal — measuring queue duration from the true queue-entry timestamp — isn't actually achieved; only the missing-started_at handling is a real behavioral change. This is easy to miss because every unit test injects a synthetic queued_at key into the mocked response, so the suite can't detect the divergence from the real API shape.
Either drop the queued_at plumbing and scope this PR to the missing-started_at fix, or call out explicitly in the PR description that GitHub doesn't currently expose this field and the plumbing is forward-looking for a future data source (e.g. a workflow_job webhook payload, if that's the intent).
🤖 AI-assisted
| @@ -614,14 +618,18 @@ def _create_runner_start( | |||
| pre_job_metrics.timestamp - (runner_metrics.installation_end_timestamp or 0), 0 | |||
| ) | |||
| # GitHub API returns started_at < created_at in some rare cases. | |||
| if job_metrics and job_metrics.queue_duration < 0: | |||
| # GitHub API returns started_at < queued_at in some rare cases. | |||
| if job_metrics and job_metrics.queue_duration is not None and job_metrics.queue_duration < 0: | |||
| logger.warning( | |||
| "Queue duration for runner %s is negative: %f. Setting it to zero.", | |||
| runner_metrics.instance_id, | |||
| job_metrics.queue_duration, | |||
| ) | |||
| queue_duration = max(job_metrics.queue_duration, 0) if job_metrics else None | |||
| queue_duration = ( | |||
| max(job_metrics.queue_duration, 0) | |||
| if job_metrics and job_metrics.queue_duration is not None | |||
| else None | |||
| ) | |||
There was a problem hiding this comment.
The new queue_duration is None branches in _issue_runner_start (falls back to float("inf")) and _create_runner_start (falls back to None, skipping the negative-clamp/warning) have no test coverage. A regression here — e.g. reverting to job_metrics.queue_duration if job_metrics else float("inf") — would raise a TypeError on None < 0 and wouldn't be caught by the existing suite, since every GithubJobMetrics(...) construction in tests/unit/metrics/test_runner.py currently passes a concrete queue_duration.
Could you add cases constructing GithubJobMetrics(queue_duration=None, ...), asserting the float("inf") observation for _issue_runner_start and RunnerStart.queue_duration is None for _create_runner_start?
🤖 AI-assisted
Applicable spec:
Overview
Rationale
Juju Events Changes
Module Changes
Library Changes
Checklist
urgent,trivial,complex).github-runner-manager/pyproject.toml.