Repository navigation
AI-7163 Monitor Dispatcher GitHub requests and throttling - #25331
Conversation
3472602 to
4335b72
Compare
evalya-impact-summaryevalya impact analysis |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 9d4e558 | Docs | View more details | Give us feedback! |
4335b72 to
e590db7
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e590db7f15
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| response.raise_for_status() | ||
| except httpx.HTTPStatusError as exc: | ||
| error = exc | ||
| self._report_attempt(attempts, method, endpoint, duration_seconds, response, error) |
There was a problem hiding this comment.
When the artifact endpoint returns a non-302 success such as HTTP 200, a different redirect status, or a 302 without Location, this line reports the attempt with error=None before _resolve_artifact_redirect rejects the response. The artifact download therefore fails and produces a terminal error log, but requests.fault records 0 and the attempt is logged as completed, understating faults for malformed artifact responses. Validate the endpoint-specific 302/Location contract before notifying the observer, or otherwise report the later validation error against this attempt.
AGENTS.md reference: ddev/src/ddev/utils/github_async/AGENTS.md:L151-L157
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The 302/Location validation now runs before the attempt is reported, so malformed artifact responses count as faults. Added coverage for 200, 301, and missing or empty Location headers.
003f883 to
7d1efb1
Compare
e590db7 to
8de32b6
Compare
❌ Dispatcher tests: failed
Caution 3 integrations failed. 4 of 855 jobs failed.
✅ 851 passed · ❌ 4 failed Batches · ✅ batch-01 240/240 · ✅ batch-02 217/217 · ❌ batch-03 220/220 · ✅ batch-04 178/178 ❌
|
6f64223 to
a089a7a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a089a7a36a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if slept: | ||
| self.on_event( | ||
| WaitEvent( | ||
| reason=WaitReason(reason), |
There was a problem hiding this comment.
If the wall clock moves backward between reserve() and the following self.now() call, a healthy request can have a positive delay while its reason remains PacingReason.NONE. After the sleep completes, converting that value with WaitReason(reason) raises ValueError because WaitReason has no NONE member, turning an otherwise successful limiter acquisition into a failed GitHub request. This is also reproducible with the public injectable now clock; handle the neutral reason explicitly rather than letting instrumentation alter request behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The moment we get comments asking about preventing an issue with a backward running clock you know we have reached the point in which now we are clawing hyper weird edge cases.
I just want to leave here for the record the response Astra game me when it read the comment and I told it "if we start having problems with clocks running backward we have bigger problems than Dispatcher thinking it is rate limited in a unexpected way".
I’m thinking about how to explain that the clock on the machine can go backwards without contradicting physics. I could phrase it as, "The machine's clock can go backwards without breaking any physical laws." It looks like the user responded humorously, which is nice! They acknowledged this point as an edge case. I wouldn't want to ignore this detail, though I’d prefer to keep the explanation concise.
You have to love this dystopian present so we can be prepare for a very awkward future 😅
PS: I am ignoring the NONE case so this is not a concern. At least we are covered the day this happens. I don't trust the GH runners xD
a089a7a to
9d4e558
Compare
Validation ReportAll 21 validations passed. Show details
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4e55821d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| error = httpx.HTTPError(f"Expected 302 redirect from {endpoint}, got {response.status_code}") | ||
| elif not response.headers.get("location"): | ||
| error = httpx.HTTPError(f"Missing Location header on redirect from {endpoint}") | ||
| self._report_attempt(attempts, method, endpoint, duration_seconds, response, error) |
There was a problem hiding this comment.
When cancel_workflow_run receives the documented 409 for a run that has already become terminal, this reports the attempt as CLIENT_ERROR, and _request also emits a terminal failure before cancel_workflow_run catches and accepts that status. Consequently, the normal shutdown race where a workflow completes before cancellation increments requests.fault and produces error logs even though the endpoint call returns successfully; handle the accepted 409 before notifying the observer.
AGENTS.md reference: ddev/src/ddev/utils/github_async/AGENTS.md:L164-L165
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm keeping this as a request fault. These metrics and logs describe the HTTP request, not the Dispatcher outcome. A 409 is still a failed request, even when the cancellation code tolerates it because the run is already terminal.
2 participants
What does this PR do?
Adds GitHub request and throttle metrics under
agent_integrations.test_dispatcher, using the existing buffered monitoring sink.requests.countrequests.faultrequests.throttledrequests.retriedrequests.durationthrottle.wait.durationrequests.polling_intervalrequests.rate_limit.remaining,requests.rate_limit.limit,requests.rate_limit.usedrequests.rate_limit.reset_inAll Dispatcher metrics reuse
GITHUB_RUN_IDasci.pipeline.id, bound before resolution and protected from caller overrides. Reruns retain the same ID; local invocations without it omit the tag. GitHub metrics carry only that ID and relevant status/reason dimensions. Budget gauges additionally carrygithub.rate_limit.resource, withunknownfor an absent resource header. Polling keeps the runner's normal dimensions, including its component.Sum event counts;
requests.throttled / requests.countgives the rate-limit rejection fraction over matching windows. Duration distributions aggregate observations across pipelines. Keep budget resources separate, and do not sum pipeline observations of the same shared budget; use minimum remaining or maximum used to monitor pressure. Missing, malformed or non-finite budget values produce no sample, never a fabricated zero.A generic optional client observer reports attempts and terminal failures; the Dispatcher owns emission, including PR-resolution requests and waits. Failed attempts and terminal failures log at ERROR; successful requests, cancellation and measured waits log at DEBUG. Logging is injected; response headers are not copied into log tags, and signed queries and credentials are excluded. Budget emission is separate from log-field construction.
Observer failures cannot change request behavior. Cancellation propagates without retry or terminal failure. Metrics remain disabled for
--dry-runand--resolve-only.Validation: 55 filtered tests passed after rebasing onto
master, covering GitHub metrics, pipeline identity, polling, logging, unresolved run dimensions, cleanup and mode suppression.ddev --no-interactive test --lint ddevpassed formatting, lint and mypy (237 source files). No live GitHub/Datadog validation yet.Motivation
Implements AI-7163 to distinguish GitHub latency, faults, retries and rate-limit rejections from local pacing and Dispatcher execution outcomes.
Builds on the execution metrics merged in #25317. Developer tooling only;
qa/skip-qa.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