Repository navigation
AI-6485 Emit Dispatcher execution and operation metrics - #25317
Conversation
4c4c88b to
e7d484c
Compare
❌ Dispatcher tests: failed
Caution 1 integration failed. 1 of 855 jobs failed.
✅ 854 passed · ❌ 1 failed Batches · ❌ batch-01 240/240 · ✅ batch-02 217/217 · ✅ batch-03 220/220 · ✅ batch-04 178/178 ❌
|
evalya-impact-summaryevalya impact analysis |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🔗 Commit SHA: 7d1efb1 | Docs | View more details | Give us feedback! |
e7d484c to
2ea7447
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: 2ea7447314
ℹ️ 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".
| url = self._artifact_download_url(artifact) | ||
| if url is None: | ||
| continue |
There was a problem hiding this comment.
When GitHub lists an expired artifact or one without an archive_download_url, this branch silently skips it without adding it to failures or setting degraded. Consequently, result.failed remains false and operations.failed{operation:collect_artifacts} reports a healthy collection even though a listed artifact was not collected. Treat these skipped artifacts as failures so partial artifact results are reflected in the operation metric.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 67cc5493e4. Expired or missing-URL artifacts now mark collect_artifacts as failed while collection continues for usable artifacts. The existing test covers both cases and checks that the workflow outcome stays unchanged.
| # The launch fact survives on the batch itself, so terminal accounting can find it. | ||
| assert batch.run_id == 123 |
There was a problem hiding this comment.
This assertion only verifies that _dispatch_batch stores the workflow ID on its mutable input object; it does not verify the relevant behavior that a launched job is later emitted as incomplete when no final result arrives. Tracking launches in a separate registry would preserve that behavior but break this test, so remove this redundant assertion or replace it with an assertion on terminal jobs.incomplete output.
AGENTS.md reference: AGENTS.md:L202-L204
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed the assertion and its comment in 67cc5493e4. The existing test_shutdown_before_progress_counts_only_launched_jobs covers the terminal behavior: launched jobs are counted as incomplete even when their progress updates are discarded, and unlaunched jobs are excluded.
71f6f4e to
febc339
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: febc3391b4
ℹ️ 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".
| raise | ||
| finally: | ||
| try: | ||
| run_summary( |
There was a problem hiding this comment.
When changes_for_run or DispatcherConfig.from_repo_config raises after a run has been resolved, this unconditional summary runs before monitoring.set_run_fields at line 283. The resulting runs.failed metric and terminal log therefore lack the resolved context, branches, and commit identity, making the failure impossible to attribute to its PR or commit; bind the resolved run identity immediately after loading/resolving it, before these fallible steps.
AGENTS.md reference: AGENTS.md:L108-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b7c495652a. Run identity is now bound immediately after resolution or manifest loading, before manifest writes, change detection and configuration loading.
The existing failure test covers those boundaries and checks emitted metric tags and terminal-log identity. Metric tag policy is unchanged: PR numbers and commit SHAs remain log-only.
b7c4956 to
28b20ae
Compare
| @@ -180,6 +163,8 @@ async def on_message_received(self, message: BaseMessage): | |||
| async def on_finalize(self, exception: Exception | None): | |||
| request = self.shutdown_request | |||
| try: | |||
There was a problem hiding this comment.
Can we add an except to this try/finally bloc ? if the _report_incomplete_jobs fails it will prevent the _shutdown_cleanup from running
There was a problem hiding this comment.
IF you concern is that report_incomplete_jobs raises the except should not be added to the outer block, but around the call. The method already catches and logs (not raise) exceptions when emitting metrics. The only case where an exception can occur when calling this method is in the accounting logic, where I could not find any case where that would be an issue.
I am adding the try/except in a way that makes the method itself not raise any exception so we avoid poluting the finalize method with more try/except and nested exception handling.
| @@ -210,6 +216,10 @@ def dispatch_tests( | |||
There was a problem hiding this comment.
Can you seed default team and a context placeholder before the try? If load_run_manifest or resolve_run aborts, the new finally still emits runs.count/runs.failed but without the tags (context , head_branch, is_default_branch or is_fork).
context can be set to unresolved and the team can be integrations-core
There was a problem hiding this comment.
There are tags being added in the resolve_run method that should probably be added beforehand, like team. For every other tag I would add unresolved until the run is resolved.
4c617e0 to
003f883
Compare
003f883 to
7d1efb1
Compare
Validation ReportAll 21 validations passed. Show details
|
2 participants
What does this PR do?
Emits Dispatcher execution and component-operation metrics through the buffered monitoring sink. All names below are under
agent_integrations.test_dispatcher.runs.count,runs.failed,runs.planning_failed,runs.cancelled,runs.timed_out,runs.no_oprun.durationbatches.count,jobs.countbatch.jobs.countbatch.durationupdated_at - run_started_at, in seconds. Excludes local artifact collection and parsing; unavailable or invalid timing is omitted.artifacts.download.durationbatches.failedjobs.failed,jobs.skippedjobs.incompleteoperations.count,operations.failedEach processor emits through its own monitor. A thin helper handles operation outcomes and timing; batch/progress state supplies job accounting without a separate metrics registry. Job metrics share the same run/job dimensions while retaining the emitting component tag. Incomplete-job accounting catches and logs its own errors so final reporting and remote cancellation still run. Report completion (
dispatcher.report.done) is log-only.Listed artifacts that are expired or lack a download URL mark collection as failed. Usable artifacts are still downloaded, and workflow outcomes remain unchanged.
Uses
ResolvedRunthroughout instead of copying identity into a separate Dispatcher context. Workflow settings stay inDispatcherConfig; tags and pytest arguments remain invocation options. Monitoring starts with protected service ownership (team:agent-integrations), repository identity, andunresolvedvalues for context, head/base branches and fork status. These placeholders keep pre-resolution metrics available to grouped queries and monitors. Resolved identity replaces them immediately after resolution or manifest loading, so setup failures retain their context. Caller tags cannot override service ownership.Preserves Dispatcher outcomes and exit behavior. Emits nothing for
--dry-runor--resolve-only. HTTP-attempt, retry and throttle instrumentation remains in AI-7163.Validation: 52 filtered lifecycle-logging, operation-metric, cleanup, identity, attribute-mapping and CLI tests passed after rebasing onto the merged logging changes.
ddev --no-interactive test --lint ddevpassed formatting, lint and mypy (235 source files). Live Dispatcher/Datadog count comparison is still pending.Motivation
Implements the execution inventory in AI-6485, following the buffered sink in #25274. Counting jobs at launch gives incomplete-job observations a matching denominator. Component-operation failures identify which Dispatcher work failed separately from individual GitHub request attempts.
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