Pin the C++ span names to the ladder, and stop writing them twice - #1909
Merged
ChaoWao merged 1 commit intoAug 20, 2026
Merged
Conversation
Reviewing hw-native-sys#1875 turned up how a rename can go silently wrong. That PR adds EXPECT_FALSE(captured_host_span("host.dispatch")); on a base that predates hw-native-sys#1893, where the word became `node`. Rebasing produces no conflict — the line is new, not edited — so the assertion looks for a name nothing emits, `captured_host_span` answers false, and `EXPECT_FALSE(false)` **passes**. The test that exists to prove a disabled gate emits nothing would then pass whether the gate works or not. Ranked by when you find out, that is the worst of the three outcomes a rename can have: a conflict is immediate, a red test is one CI run, and a vacuously-true assertion is never. What makes it silent is the negated form — the same staleness under `EXPECT_TRUE` merely goes red. Two changes, and the first is not a new guard but the removal of what made the guard necessary. **The names come from the table the emitter uses.** Eight literals in `test_scheduler.cpp` are now `host_span_name(HostSpan::Dispatch)` and friends. These tests assert that a decision point emitted at all; the name is not their subject, so writing it out was a second copy of it. Taking it from the single source means a rename cannot leave them behind — verified by renaming the level word to `network9` and re-running: the suite stays green because the assertions follow. Under the old literals the same rename would have reddened the `EXPECT_TRUE` ones and silently passed an `EXPECT_FALSE` one. **The C++ pre-bind default is pinned to the ladder.** `host_span_names.h`'s `level_word()` defaults to `"node"`, which was a third hand-written copy of the L3 word: `WorkerLevel` is the source of truth, `strace_timing.py`'s `_NODE_WORDS` is pinned to it by its own test, and this one was pinned to nothing. A level renamed in Python would leave C++ emitting the old word for every span before a Worker binds the prefix, with no test noticing. The pin reads that default through the existing binding rather than adding a symbol: `set_level_prefix("")` returns early without binding, while the binding still reports what is in effect. It runs in a child process because the prefix freezes on first bind, so a Worker constructed anywhere in this process would leave the *bound* word behind instead of the default. `test_graph_failure_still_emits_graph_build_span` also spelled out `node.graph_build`; it now builds the expected name from the worker's own `_host_span_prefix`, which is the contract that assertion is about. Verification: - `pytest tests/ut/py` — 1657 passed, 0 failed. - `ctest -LE requires_hardware` — 107/107. - Negative control for the pin: setting the C++ default back to `"host"` fails `test_the_cpp_pre_bind_level_word_is_the_ladder_word_for_l3` and nothing else. - Negative control for the double-write removal: renaming the level word to `network9` keeps `test_scheduler` green, where a literal would have gone red or, worse, vacuously true. Not every literal span name is a double-write, and the distinction is which side of the test it sits on. A literal that **constructs input** — the `SimplerHostSpan` fed to the logger in `test_host_log_off.cpp`, the fake log lines in `test_strace_timing.py` — is right to be written out: the test owns its input, and it is exercising encoding or parsing over an arbitrary name. A literal that **asserts the implementation emitted a particular name** is the second copy, and that is the class both changes above remove. The eight in `test_scheduler.cpp` were the only ones of that kind. What this does not cover, stated because "renames are guarded now" would be too broad a claim: the pin ties the C++ *default* word to the ladder, not every place a name could be spelled out. `git grep -n '"\(node\|network[123]\)\.'` before landing a rename, and judge each hit by the input-versus-assertion test above.
|
Warning Review limit reached
Next review available in: 39 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Reviewing #1875 turned up how a rename can go wrong silently. That PR adds
on a base predating #1893, where the word became
node. Rebasing produces no conflict — the line is new, not edited — so the assertion looks for a name nothing emits,captured_host_spananswers false, andEXPECT_FALSE(false)passes. The test that exists to prove a disabled gate emits nothing would pass whether the gate works or not.Ranked by when you find out, that is the worst of the three outcomes a rename can have:
What makes it silent is the negated form — the same staleness under
EXPECT_TRUEmerely goes red.What this changes
Two things, and the first is not a new guard but removal of what made a guard necessary.
1. The names come from the table the emitter uses. Eight literals in
test_scheduler.cppbecomehost_span_name(HostSpan::Dispatch)and friends. Those tests assert that a decision point emitted at all — the name is not their subject, so spelling it out was a second copy of it.2. The C++ pre-bind default is pinned to the ladder.
host_span_names.h'slevel_word()defaults to"node", which was a third hand-written copy of the L3 word:WorkerLevel(source of truth)strace_timing.py::_NODE_WORDShost_span_names.h::level_word()defaultA level renamed in Python would leave C++ emitting the old word for every span emitted before a Worker binds the prefix, and no test would notice.
The pin reads that default through the existing binding rather than adding a symbol:
set_level_prefix("")returns early without binding, while the binding still reports what is in effect. It runs in a child process because the prefix freezes on first bind — a Worker constructed anywhere in this process would leave the bound word behind instead of the default.test_graph_failure_still_emits_graph_build_spanalso spelled outnode.graph_build; it now builds the expected name from the worker's own_host_span_prefix, which is the contract that assertion is actually about.Not every literal is a double-write
The distinction is which side of the test it sits on, and it is the reusable part of this change:
SimplerHostSpanfed to the logger intest_host_log_off.cpp, the fake log lines intest_strace_timing.py— is right to be written out. The test owns its input and is exercising encoding or parsing over an arbitrary name.test_scheduler.cppwere the only ones of that kind.Testing
pytest tests/ut/py— 1657 passed, 0 failedctest -LE requires_hardware— 107/107"host"failstest_the_cpp_pre_bind_level_word_is_the_ladder_word_for_l3and nothing elsenetwork9keepstest_schedulergreen, because the assertions follow. Under the old literals the same rename would have reddened theEXPECT_TRUEones and silently passed anEXPECT_FALSEone.Scope
Stated because "renames are guarded now" would be too broad: the pin ties the C++ default word to the ladder, not every place a name could be spelled out.
git grep -n '"\(node\|network[123]\)\.'before landing a rename, and judge each hit by the input-versus-assertion test above.Found while reviewing #1875 (#1792 items 3 and 7); that PR still needs a rebase, after which its own assertion should use the same accessor.