Skip to content

quest: close the qos line; lag-histogram carries the port - #5096

Merged
kixelated merged 2 commits into
mainfrom
quest/qos-line-closes
Oct 9, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/qos-line-closes

Conversation

@kixelated

@kixelated kixelated commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Problem: #4133 (qos line) cannot land. Every code change it has over main is the egress lag histogram, which main already split into quest/m1/qos/lag-histogram.md (requires stats-split), and the code no longer compiles against main's caller-supplied time model.

Approach: close #4133 without landing and keep its code on wip/4133-lag-histogram (same commit as the line head, 9c5189b).

Impact: none (quest text only). Wire: none.

Alternatives: port now and land with the histogram (reverses the stats-split-first decision); hold the line open (keeps a line branch alive). Taken under /recommended from the #4133 iterate agent's report (option 1).

Follow-ups: the quest/m1/qos/README branch still exists at 9c5189b (identical to wip/4133-lag-histogram); quest-flat-lines.md is done only when no quest/*README branch remains.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of aad26130 (quest text only)

Checked: #4133 is closed, its head 9c5189be matches wip/4133-lag-histogram, so the "code preserved" claim holds. Links resolve.

Non-blocking

  1. quest/m1/qos/lag-histogram.md Plan (lines 15–23, unchanged context) still says the implementation "already lives on the line branch ([quest(qos): Broadcast health and congestion #4133])" and links the PR. It reads as if the line is live; point it at wip/4133-lag-histogram (or fold into the new "Decided 2026-10-09" paragraph) so the two paragraphs don't contradict.
  2. quest/m1/qos/README.md:41-44 still says "quest(qos): Broadcast health and congestion #4133 rebases its histograms onto stats-split's totals" and discusses "quest(qos): Broadcast health and congestion #4133 first" as an alternative; with the line closed, that wording is stale. Worth a one-line update in this PR.
  3. Minor: the findings list ("A timestamp regression hides a stalled viewer", etc.) has no links back to the quest(qos): Broadcast health and congestion #4133 review threads; whoever ports will have to dig them up. Linking the comments would make them actionable.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: aad2613

No actionable correctness bugs found in this documentation-only diff. The new port checklist matches the retained sampler's four clock reads, including its drop-time sample, and main's caller-supplied time and optional timestamps. The five carried findings correspond to #4133's outstanding accounting and robustness issues; this PR does not claim to fix them.

Direction: closing the umbrella PR while retaining the implementation for the stats-split-dependent quest is simpler than porting it twice or keeping the line open. #4133 is closed without merging, the saved branch is accessible, and the dependency remains explicit. The existing editorial review covers the stale historical wording and missing finding links; no duplicate inline comments.

Verification: GitHub-only static review of both changed documents, their dependency context, the retained implementation and #4133 discussion. No quest checks, builds or tests run. Open/non-draft state, head and reviews rechecked immediately before submission.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Addressed the Grok review of aad26130 in 7b1d4e6e:

  1. lag-histogram.md Plan no longer says the code lives on the line branch; the 2026-10-09 paragraph now names quest(qos): Broadcast health and congestion #4133 as closed and lists what wip/4133-lag-histogram carries.
  2. qos/README.md stats-split-first decision no longer names quest(qos): Broadcast health and congestion #4133, and the 2026-10-08 decision notes the close and the wip/ branch.
  3. Each open finding links to its quest(qos): Broadcast health and congestion #4133 thread.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok review of 7b1d4e6e (quest text only)

I checked the claims against the repo and they hold: #4133 is closed, wip/4133-lag-histogram and quest/m1/qos/README both point at 9c5189be, and #4298, #5009, #4451, #4437, #4822 and #4917 are all merged with the titles the text implies. rs/moq-net/src/stats.rs exists on main, and none of the qos quest files still says the histogram "exists only on the line branch".

No blocking issues. Non-blocking:

  • lag-histogram.md: the first two review-finding links ("3" and "B") point at the same comment (#issuecomment-6053929743). That's fine if both findings live in that one comment, but labels like "3" and "B" mean nothing to someone reading the quest. Short descriptive labels would read better.
  • quest-flat-lines.md still needs the quest/m1/qos/README branch deleted before it's done (the PR body already flags this as a follow-up). Deleting it once this merges would be worth doing, since wip/ keeps the same commit.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@coderabbitai

coderabbitai Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8754b0fa-48f9-4787-b2a3-b1c12301e17e
📥 Commits

Reviewing files that changed from the base of the PR and between 3ce2159 and 7b1d4e6.

📒 Files selected for processing (3)
  • quest/m1/qos/README.md
  • quest/m1/qos/lag-histogram.md
  • quest/m1/quest-flat-lines.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.


Walkthrough

The QoS notes state that stats totals and prefix tracks will host the lag histogram instead of retired per-path map rows. They record that #4133 closed without landing and that its code remains on wip/4133-lag-histogram. The porting plan lists requirements for time sourcing, timed-only Production recording, a subscription field rename, and five open findings. The quest status identifies the histogram as the only change over main and links to the separate Viewer quest.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 7b1d4

This updates QoS quest status and porting notes without changing runtime behavior. The documented branch cleanup remains a follow-up; no material merge risk is evident.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: closing the QoS line and preserving the lag-histogram port.
Description check Passed The description directly explains the documentation-only changes, the closure of #4133, and the preserved lag-histogram port.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Re the Grok review of 7b1d4e6e: both nits are left as is.

  • The "3" and "B" link labels are the finding names used in that quest(qos): Broadcast health and congestion #4133 review comment, and each bullet already describes its finding, so the label only identifies the thread.
  • Deleting the quest/m1/qos/README branch is a follow-up for the maintainer, as the PR body notes; wip/4133-lag-histogram keeps the same commit.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit 652d5ff into main Oct 9, 2026
5 checks passed
@kixelated
kixelated deleted the quest/qos-line-closes branch October 9, 2026 06:49
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged at 7b1d4e6ef6689ae86b6b2ecd039c1498f011b3c9 under /recommended.

  • Review: CodeRabbit reviewed this head with no actionable comments; OpenAI reviewed aad26130 with no findings. Grok's non-blocking nits on both heads were fixed or replied to.
  • Added in 7b1d4e6e: the lag-histogram Plan and qos README no longer read as if the line is live, and each open quest(qos): Broadcast health and congestion #4133 finding links to its thread.
  • CI: Check, Test, and Quest green.
  • Follow-up for the maintainer: delete the quest/m1/qos/README branch (same commit as wip/4133-lag-histogram) so quest-flat-lines.md can finish.

(Written by Claude Opus 5.5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant