Skip to content

refactor(stats): rename InclusionTracker to OutcomeTracker, and close out the spec - #81

Open
bdchatham wants to merge 12 commits into
brandon2/plt-1080-handoff-channelfrom
brandon2/plt-1081-polish
Open

refactor(stats): rename InclusionTracker to OutcomeTracker, and close out the spec#81
bdchatham wants to merge 12 commits into
brandon2/plt-1080-handoff-channelfrom
brandon2/plt-1081-polish

Conversation

@bdchatham

Copy link
Copy Markdown
Contributor

Eighth and last in the outcome-tracking stack. Closes the Polish phase, and the feature.

The rename

InclusionTracker stopped being about inclusion when it started reading receipts. It resolves a transaction to committed or reverted; inclusion is one of the states it reports rather than the thing it measures.

Type and filenames only, as its own commit. Log strings keep inclusion tracker: — they are what an operator greps and what the runbooks name, so renaming them would break the thing the rename is supposed to clarify.

The conservation identity

sender/doc.go said dropped_at_handoff was a term of the identity that nothing produced yet. #80 gave it a producer, so the note now says what it is.

make verify — what it actually said

step result
check-lint-pin pass, v2.12.2 in sync
lint pass, 0 issues
test pass, 42.7% statement coverage
build pass
--help smoke pass
check-bindings did not run

check-bindings needs Docker to recompile the Solidity, and the daemon is not running on this machine. No contract changed anywhere in this stack, so the bindings cannot have drifted, but I did not verify that locally and CI will.

One finding worth naming

lint failed the first time I ran it, on three QF1012 findings in stats/execution_outcomes.go from #78. I had earlier reported staticcheck as clean on that file, and that was wrong: bare staticcheck does not enable the QF category, and golangci-lint does. Fixed at the bottom of the stack in #78 and merged forward, so every PR here is lint-clean rather than only the last one.

Mutation pass

Every guard in this stack was broken on purpose before it was believed. Fourteen mutations across the five PRs, all caught:

PR mutations
#77 read deadline budget ignores head age; past-deadline height read anyway; read unbounded by a tight reapAfter
#78 report all-expired run mislabelled as measurement failure; untracked run prints counts; revert share over accepted; status_unavailable row dropped
#79 goodput both ratios collapse to one; unobserved excused from the denominator; Rejected() underflows
#80 hand-off full hand-off blocks the sender; full hand-off drops in silence; drain performs a chain read; zero send rate gives a zero-depth channel

Deferred, with its condition intact

D-2, whether a hand-off drop voids the run. Un-defer when a run first drops a hand-off outside a deliberate overload test. That has not happened, and inventing a verdict rule before it does would be guessing at a threshold with no run behind it.

bdchatham and others added 3 commits August 29, 2026 10:39
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The type stopped being about inclusion when it started reading receipts. It
resolves a transaction to committed or reverted, and inclusion is one of the
states it reports rather than the thing it measures.

Type and filenames only. Log strings keep "inclusion tracker", because they
are what an operator greps and what the runbooks name.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 29, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes how load runs select the node for outcome tracking and renames a core stats type used on the send path; mistakes could skew inclusion/outcome metrics or add load on the SUT node, though new tests pin the endpoint rules.

Overview
Renames InclusionTracker to OutcomeTracker across main, sender, and stats (constructors, types, tests, and package docs). Operator log lines still use inclusion tracker: so runbooks and greps stay stable.

Wires the run through stats.NewOutcomeTracker and utils.Option[*stats.OutcomeTracker] on ShardedSender with no intended behavior change beyond naming.

Extracts trackingEndpoint(cfg) in main.go so receipt/status reads and the head WebSocket always come from one node (TOT-022): receiptEndpoint when set, otherwise endpoints[0] with an explicit shared-with-load flag. The warning about competing with send load is emitted only when the tracker shares the load node (TOT-021).

Adds main_test.go coverage for dedicated receipt node vs single-node fallback and for WebSocket derivation from the tracking HTTP endpoint.

Reviewed by Cursor Bugbot for commit eb67539. Bugbot is set up for automated code reviews on this repo. Configure here.

@bdchatham

Copy link
Copy Markdown
Contributor Author

@seidroid review

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mechanical rename of InclusionTracker to OutcomeTracker (types, methods, and two filenames) plus a doc-comment correction noting that dropped_at_handoff now has a producer. The rename is complete with no dangling references anywhere in the tree, exported metric names and inclusion tracker: log strings are deliberately preserved, and the updated doc text matches the actual non-blocking Submit implementation.

Findings: 0 blocking | 0 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • None at the file/PR level.

T018 and T020, the last two open tasks in the spec.

The behaviour was already right and nothing held it there. Extracting the
choice into trackingEndpoint makes both halves of TOT-021 testable: reads land
on a peer when one is configured, and fall back to the load node when it is
the only one, saying so rather than competing with the workload in silence.

TOT-022 follows from the shape. The tracker takes one endpoint and derives the
head stream from it, so a caller cannot take heads from one node and status
from another.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bdchatham

Copy link
Copy Markdown
Contributor Author

Added the last two open tasks, T018 and T020, so this closes the spec at 43 of 43.

The behaviour was already right and nothing held it there. Extracting the choice into trackingEndpoint makes both halves of TOT-021 testable: reads land on a peer when one is configured, and fall back to the load node when it is the only one, saying so out loud rather than competing with the workload in silence. TOT-022 then follows from the shape, since the tracker takes one endpoint and derives the head stream from it.

Both mutation-checked: reads landing on the load node despite a configured peer, and a single-node run not announcing that its reads compete.

make lint clean after the merge-forward.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The InclusionTracker→OutcomeTracker rename is complete and consistent across types, methods, filenames, the metrics registry, and docs, and the sender/doc.go conservation-identity correction matches the real non-blocking Submit. The newly added trackingEndpoint helper is behavior-preserving; two non-blocking notes concern an undocumented precondition and a test that re-derives the invariant it claims to pin.

Findings: 0 blocking | 2 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • None at the file/PR level.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

Inline comments (could not post inline; listed here)

  • main_test.go:109 (RIGHT) -- [suggestion] This test re-derives the composition in test code rather than asserting the production wiring. trackingEndpoint() and utils.GetWSEndpoint() are both production, but nothing here observes that OutcomeTracker.Run is actually called with the tracking endpoint — the WS derivation happens inside Run (wsEndpoint := utils.GetWSEndpoint(endpoint)), while the test performs it itself.

So the doc comment's claim ("the caller cannot split them ... This pins that") overstates what is pinned: if main.go:320 were changed to inclusionTracker.Run(ctx, cfg.Endpoints[0]), this test would still pass green. What it currently proves is that trackingEndpoint prefers ReceiptEndpoint (already covered by TestStatusReadsLandOnTheTrackingNode) plus that GetWSEndpoint is deterministic.

To actually pin TOT-022, the assertion needs to observe the endpoint Run receives — e.g. have the head server in stats/inclusion_run_test.go record the endpoint it was dialed on, or restructure so both the status source and the head subscription are built from one value the test can assert against.

  • main.go:475 (RIGHT) -- [suggestion] cfg.Endpoints[0] panics if Endpoints is empty and ReceiptEndpoint is unset. The sole caller is guarded by len(cfg.Endpoints) > 0 at line 294, so this is safe today, but now that the logic is a package-level function the precondition no longer travels with the code — and TestOneNodeStillWorks pins the single-node path without covering the zero-endpoint one.

Worth either stating the precondition in the doc comment ("requires at least one endpoint when ReceiptEndpoint is empty") or returning "", false for the empty case, so a future caller gets a misconfiguration signal rather than an index panic.

bdchatham and others added 4 commits September 1, 2026 12:04
Added on plt-1078, where the type was still InclusionTracker and the
constructor took no tps. The rename and the hand-off parameter both land
between there and here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…81-polish

# Conflicts:
#	stats/export_test.go
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