FIX: Publish PR Performance Reports from ready artifacts - #801
Gaurav Sharma (bewithgaurav) wants to merge 4 commits into
Conversation
Align artifact trust with coverage, publish before unrelated matrix jobs finish, and terminalize exact-head comments across PR lifecycle changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR Performance ReportPerformance could not be assessed because no environment produced a complete result. Coverage: 0 of 4 environments completed. Advisory result; does not block merging.
Affected phases and call countsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsBuild, commits and measurement detailsPR head: A consistent slowdown requires more than 20% median paired slowdown, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold. An inconsistent slowdown crosses the first two thresholds without enough pair agreement. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Unavailable or rejected data: Windows-SQL2022 (invalid artifact), Windows-SQL2025 (invalid artifact), Linux-SQL2022 (invalid artifact), Linux-SQL2025 (invalid artifact), workload version differs from trusted base Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate issues remain in the publication and comment-state handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates PR performance-report publication to rely on ready profiler artifacts instead of whole-suite equality checks.
Changes:
- Adds artifact polling, propagation grace periods, and merge finalization.
- Handles abandoned/superseded PRs and bounded service failures.
- Updates reporting code, documentation, and contract tests.
File summaries
| File | Summary |
|---|---|
tests/test_036_profiler_ci.py |
Updates tests for the revised publication behavior. |
eng/profiler_benchmarks/report.py |
Removes suite identity validation. |
eng/profiler_benchmarks/README.md |
Documents artifact-readiness publication. |
eng/profiler_benchmarks/controller.py |
Stops emitting suite hashes. |
.github/scripts/post_profiler_comment.py |
Implements polling and terminal states; requires fixes for comment race handling, usable download URL validation, merged-base derivation, and preserving existing final reports. |
Review details
Suppressed comments (3)
.github/scripts/post_profiler_comment.py:124
- If the PR head or base changes after the comment list is read but before this revalidation,
current_body()returnsNoneand this branch exits without updating the already-found marker comment. That can leave the initial “Performance assessment pending” comment permanently pending when the superseding run is canceled or never publishes; the post-write path below already terminalizes this race. Reuse that conditional superseded-state update for the existingcommentbefore returning here.
message = current_body()
if message is None:
return
.github/scripts/post_profiler_comment.py:307
- Readiness is based only on artifact names. An ADO item can have a
resourcewithout a usable, non-emptydownloadUrl; it then passes this check, the loop breaks even while the build is running, andassessimmediately publishes a partial report instead of waiting for usable artifacts. Require each required item to have a non-empty download URL before declaring readiness.
required = {"profiler-" + leg for leg in reporting.LEGS}
# Artifact readiness is the report signal; unrelated matrix legs do
# not need to finish before the four profiler legs are assessed.
if required <= {item["name"] for item in artifacts}:
.github/scripts/post_profiler_comment.py:125
- When a closed, unmerged PR is observed,
current_body()returns the terminal message without considering the existing comment body, so this branch can overwrite an already-published final report (or another terminal state) with “closed before assessment completed.” The requested terminalization is for pending comments; preserve an existing non-pending report and only replace the pending comment.
if comment:
message = current_body()
if message is None:
return
github(f"issues/comments/{comment['id']}", method="PATCH", data={"body": message})
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -215,6 +253,15 @@ def run(number, head, wait_minutes): | |||
| current_head = pr["head"].get("sha") | |||
| current_base = pr["base"].get("sha") | |||
| pr_base = current_base | |||
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.row.py: 83.4%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.py_type_cache.hpp: 91.6%🔗 Quick Links
|
Require usable artifact URLs and protect pending, completed, and superseded comments across lifecycle races. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in comment preservation, grace-boundary polling, SHA validation, and stale-head handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
.github/scripts/post_profiler_comment.py:176
- If an existing completed report is updated while the PR is open, the PR can close before this verification and
current_body()returnsclosed_message(), causing this branch to overwrite that completed report with an unavailable terminal state. Preserve completed comments here; only terminalize a comment that was pending before the write, while the supersession branch may separately handle a newly-created report whose head changed.
if verified is not None and verified != message:
github(f"issues/comments/{comment_id}", method="PATCH", data={"body": verified})
elif verified is None:
latest = github(f"issues/comments/{comment_id}")
if isinstance(latest, dict) and latest.get("body") == message:
.github/scripts/post_profiler_comment.py:267
- The
<loop condition exits exactly when the two-minute grace expires, so after the poll at roughly T+90 seconds there is no artifact request at T+120. Artifacts that propagate during that final interval are therefore ignored and a partial report is published even though all four artifacts were available before the grace ended; perform a final artifact poll at the grace boundary before deciding readiness.
while time.monotonic() < (
max(deadline, completed_at + ARTIFACT_GRACE_SECONDS)
if completed_at is not None
else deadline
):
.github/scripts/post_profiler_comment.py:281
- This validation only checks that
headandbaseare dictionaries. If GitHub returns a malformed response with a missing or invalidhead.sha,current_headbecomesNone(or a bad string) and this early return silently leaves the pending comment forever instead of retrying and reaching the bounded terminal fallback. Validate both SHA fields before treating a mismatch as a superseded revision.
current_head = pr["head"].get("sha")
current_base = pr["base"].get("sha")
pr_base = current_base
if current_head != head:
return
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: bewithgaurav <8655500+bewithgaurav@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved publication lifecycle and artifact URL validation issues remain in the posting script.
Review details
Suppressed comments (3)
.github/scripts/post_profiler_comment.py:281
- When the PR head changes during the polling loop, this return leaves the pending comment created at line 254 in place indefinitely. The new superseded-terminalization logic only runs inside
publish; invoke it here (with the old head) before returning so a stale run cannot leave anassessment pendingcomment behind.
# Supersede the pending comment through the compare-and-update
# path instead of leaving it posted for the stale head.
.github/scripts/post_profiler_comment.py:334
- A non-empty string is treated as a usable artifact URL here, but
fetch()rejects URLs that are not HTTPS or outside the permitted artifact hosts. If ADO returns a placeholder or malformed URL for all four legs, this breaks while the build is still running and immediately publishes invalid legs instead of waiting for usable URLs.
failures = 0
required = {"profiler-" + leg for leg in reporting.LEGS}
usable = {
item["name"]
for item in artifacts
if isinstance(item["resource"].get("downloadUrl"), str)
.github/scripts/post_profiler_comment.py:161
current_body()returns the caller-suppliedbodyfor an open PR, somessage == closed_message()does not prove that the PR is still closed. A close/reopen race between the checks can therefore overwrite a pending comment with the closed terminal state, and the verification below repeats the same ambiguity; re-read and require the actual PR state to remain closed before this PATCH.
if comment["body"] != pending_message(head):
return
latest = github(f"issues/comments/{comment['id']}")
if not isinstance(latest, dict) or latest.get("body") != comment["body"]:
return
github(f"issues/comments/{comment['id']}", method="PATCH", data={"body": message})
comment_id = comment["id"]
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The comment lifecycle has an unresolved edge case that can leave a closed PR permanently marked “assessment pending.”
Review details
Suppressed comments (1)
.github/scripts/post_profiler_comment.py:159
- When a PR is closed after its head has advanced, the existing bot comment can still be the pending message for the previous head. This guard compares only with
pending_message(head), sorun()'s terminalunavailable()call returns without replacing that old pending comment, leaving a permanent "assessment pending" state. Match an unchanged pending body independent of the current head while retaining the compare-and-update re-read.
if comment["body"] != pending_message(head):
return
latest = github(f"issues/comments/{comment['id']}")
if not isinstance(latest, dict) or latest.get("body") != comment["body"]:
return
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Work Item / Issue Reference
Summary
Make PR Performance Report publication follow the same artifact-readiness model as code coverage.
assessment pending.This PR's own report may remain unavailable because the trusted publisher on
mainstill enforces the previous suite-hash contract. Subsequent PRs will use the relaxed artifact model.Validation
109 profiler CI contract tests passed. Black, Flake8, and diff checks passed.