Skip to content

Fix the defects the agents page turned up - #16

Merged
AlexeyShalaev merged 3 commits into
masterfrom
fix/agents-page-findings
Sep 6, 2026
Merged

Fix the defects the agents page turned up#16
AlexeyShalaev merged 3 commits into
masterfrom
fix/agents-page-findings

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

Writing docs/agents.md against the source turned up five things. Four were real; one was
half real, and the half that was real ran the opposite way from the report. Every finding is
below with its reproduction, including the parts that did not hold up.

Nothing here is breaking. One name leaves a submodule's __all__ — see finding 5.

1. The health page said the factory does not forward options or compression

It forwards both, on purpose, with the reason in a comment right there in factory.py:
the probes have to negotiate HTTP/2 the way the application channels do. The page told the
reader the opposite and sent them off to construct HealthChecker by hand for something they
already had.

Reproduction — a settings object carrying channel options, with the checker class replaced by
a spy:

service = 'users.v1.Users'
options = [('grpc.max_receive_message_length', 1024)]
compression = None
insecure = True
check_interval = 7.0
timeout = 2.0

The doc drifted, not the code. Corrected, and the paragraph now lists what the factory does
pass, including the service the next finding is about. The "build it yourself" advice stays,
attached to the things settings genuinely cannot express — on_status_change, max_backoff,
fail_fast_callback.

While in that table: max_backoff was listed under "what the factory wires for you" without
saying that it is the checker's own default and is not read from settings. It says so now.

2. The same page on the probed service, and on where the error lives

Two claims, both wrong.

"Each probe asks for the overall server status ... rather than a per-service one."
HealthChecker.__init__ takes service: str = "", and the factory forwards
getattr(settings.health_checker, "service", "") — visible in the same reproduction above,
which came back with service = 'users.v1.Users'. The empty name is the default, not the only
option. The page now says so, and the settings table gained a service row.

"HealthCheckerNotRunningError lives in grpc_client_kit.health, which likewise needs the
extra." It lives in grpc_client_kit.errors, is exported from the package root, and its own
docstring explains that it is placed there so catching it does not require the [health]
extra: the caller who meets it is a caller of a balancer, and a balancer works on a bare
install. The page was telling people to install an extra to write an except clause.

3. Two pages said there is no settings block for their layer

resilience.md on wait-for-ready and deadlines.md on deadline budgets both said "there is
no settings block for this layer" and routed the reader to build_interceptors and a
hand-built chain. configuration.md documents both blocks correctly, and factory.py reads
both with getattr and builds the layers into the chain itself.

Reproduction — a settings object with a wait_for_ready block and a deadline_budget block,
handed to the factory, then the chain the factory built (duplicates are the four RPC-kind
adapters per logical interceptor):

AsyncTimeoutInterceptor x4
AsyncDeadlineBudgetInterceptor x4
AsyncWaitForReadyInterceptor x4
AsyncRetryInterceptor x4
AsyncCircuitBreakerInterceptor x4

Both layers are there, in the positions the pages themselves describe. Corrected. The part of
the advice that was right — per-method caps need a hand-built chain, and that chain goes to
GrpcClient(interceptors=...) and not to create_client(interceptors=...) — survives, now
attached to the thing that is actually true of it.

docs/agents.md carried the same claim in the sentence introducing its hand-built chain
example ("which is what you need for per-method budgets, request-budget propagation or
wait-for-ready"); that line is fixed in the same commit.

4. hasattr(grpc_client_kit, "HealthChecker") raises instead of returning False

Reproduces on an install without the [health] extra:

ImportError: Install grpc-client-kit[health] (grpcio-health-checking) to use gRPC health checking
  File "grpc_client_kit/__init__.py", line 164, in __getattr__

hasattr swallows AttributeError only, and the lazy __getattr__ raises ImportError.

Kept as it is. A missing extra is an install problem and should say so; downgrading to
AttributeError would make a broken install indistinguishable from a name that never existed,
and would break except ImportError around the attribute. The obvious third option does not
exist — an exception inheriting both raises TypeError: multiple bases have instance lay-out conflict, which I checked rather than assumed.

So the defect is that this was written down nowhere. It now appears in the health guide, in
rule 19 of the agents page and in the __getattr__ docstring, each naming the supported probe
(importlib.util.find_spec("grpc_health"), or catch the ImportError). The existing
bare-install subprocess probe in tests/unit/conftest.py gained an assertion for it, so a
future change to AttributeError fails a test instead of silently changing an unwritten
contract. Verified by making that change locally: the probe fails, as intended.

5. Exports that disagreed with each other

The report named the three extras protocols and MethodCircuitState. Checking the whole
surface found the same class of problem running the other way, and a third case that is not a
problem at all.

in protocols.__all__ but not at root: FullGrpcClientSettingsProtocol,
                                      GrpcChannelExtrasProtocol,
                                      GrpcObservabilityExtrasProtocol
root protocol exports missing from protocols.__all__: CircuitBreakerMetricsProtocol,
                                                      RetryMetricsProtocol
circuit_breaker.__all__ not at root: MethodCircuitState

The three extras protocols are now exported from the package root. Eleven of the module's
fourteen protocols already were; the three left out are exactly the ones describing the
optional settings blocks — which is to say, the ones a settings-object author reaches for
after the required surface. There is no principle separating them from the eleven, so the rule
is now simply "every protocol in protocols.__all__ is a top-level export". Purely additive.

RetryMetricsProtocol and CircuitBreakerMetricsProtocol are now in protocols.__all__.
They are top-level exports and documented as exported, but a star import of the module skipped
them.

MethodCircuitState leaves circuit_breaker.__all__. This is the one direction that is
not additive, so: it is the breaker's mutable per-method bookkeeping, it appears in no public
signature (get_states() returns CircuitBreakerStatus snapshots), and it is documented
nowhere. Declaring it public froze the breaker's implementation into the compatibility
contract for no caller's benefit — the same reason ChannelWrapper and chain_token are kept
out of the pool's surface, which the package comment already states. It stays importable by
name; only import * from that module changes, and the test suite imports it directly and
still passes.

Not a defect: the module-only names generally. A sweep found fifteen names in submodule
__all__s that the root does not export — Continuation, RpcType, DEFAULT_RETRYABLE_CODES,
DEFAULT_SENSITIVE_HEADERS, HAS_TRACING, the health module's constants, and so on. The
curated root is deliberate and the agents page maps the ones a caller reaches for. The three
protocols were the odd ones out, not the pattern.

Three tests in tests/unit/test_init.py pin all of this; all three fail on master:

declared in grpc_client_kit.protocols but not exported: ['FullGrpcClientSettingsProtocol', 'GrpcChannelExtrasProtocol', 'GrpcObservabilityExtrasProtocol']
exported by the package but absent from protocols.__all__: ['CircuitBreakerMetricsProtocol', 'RetryMetricsProtocol']
assert 'MethodCircuitState' not in [..., 'MethodCircuitState']

The agents page

Updated in this PR, as CONTRIBUTING.md requires: the protocols list gained the three new
exports, the module table lost their row, the internals paragraph gained
MethodCircuitState, rule 19 gained the hasattr caveat, and the hand-built-chain sentence
lost the claim from finding 3.

Verification

make check
  ruff check .            All checks passed!
  ruff format --check .   109 files already formatted
  mypy grpc_client_kit    Success: no issues found in 25 source files

make test-unit            418 passed, 92 deselected
make test-integration      90 passed, 418 deselected, 2 xfailed
make test                 508 passed, 2 xfailed — total coverage 96.02%

uv sync --frozen --all-extras --group dev; uv.lock unchanged.

Alex Shalaev added 3 commits September 6, 2026 21:15
`grpc_client_kit.protocols` declared fourteen protocols and the package
re-exported eleven of them, so a settings object that carries credentials,
channel options or a metrics registry had to import its protocol from the
submodule while every other protocol came from the package. Export the three
extras protocols too, so the rule is simply "every protocol in
`protocols.__all__` is a top-level export".

Two protocols went the other way: `RetryMetricsProtocol` and
`CircuitBreakerMetricsProtocol` are top-level exports and documented as such,
but were missing from `protocols.__all__`, so a star import of the module
skipped them.

`MethodCircuitState` leaves `circuit_breaker.__all__`. It is the breaker's
mutable bookkeeping and never crosses a public signature — `get_states()`
hands out `CircuitBreakerStatus` — so declaring it public froze the
implementation for no caller's benefit. It stays importable by name.
The health page said three things the code does not do.

It said each probe asks for the overall server status. `HealthChecker` takes a
`service` argument, defaulting to the empty name, and the factory forwards the
`health_checker` block's optional `service`, so a per-service probe has been
configurable all along.

It said the factory does not forward `options` or `compression`, and told the
reader to build the checker by hand when the probes need the application's
channel options. The factory forwards both, deliberately, so the probes
negotiate HTTP/2 the way real traffic does.

It said `HealthCheckerNotRunningError` lives in `grpc_client_kit.health` and
needs the [health] extra. It lives in `grpc_client_kit.errors`, is a top-level
export, and is placed there precisely so that catching it needs no extra — a
balancer caller meets it, and balancers work on a bare install.

Separately: `hasattr(grpc_client_kit, "HealthChecker")` raises the ImportError
rather than answering False, because the lazy `__getattr__` raises ImportError
and `hasattr` only swallows AttributeError. That stays as it is — a missing
extra is an install problem and must say so, and an error class inheriting both
is impossible (instance lay-out conflict) — but it was written down nowhere. It
is now in the guide, in rule 19 of the agents page and in the `__getattr__`
docstring, and the bare-install probe asserts it so it cannot drift.
Both pages told the reader "there is no settings block for this layer" and sent
them to a hand-built chain. The configuration page documents the two blocks
correctly, and the factory reads both with `getattr` and builds the layers into
the chain in their proper positions, so the two pages were sending readers to
`build_interceptors` for something a settings object already covers.

What settings genuinely cannot express is per-method timeouts, since the
`timeout` block carries only `default` — that part of the advice survives, and
is now what the paragraphs say. The agents page carried the same claim in the
sentence introducing its hand-built chain example.
@AlexeyShalaev
AlexeyShalaev merged commit e56cad0 into master Sep 6, 2026
6 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:45
AlexeyShalaev added a commit that referenced this pull request Sep 6, 2026
…behaviour (#17)

The work landed in #16. Its squash subject lost the Conventional Commit prefix --
my mistake on the merge, not the author's -- so release-please skipped the merge
and these fixes would never have reached a release. This commit carries the
record. It changes no code: #16 is already on master.

* FullGrpcClientSettingsProtocol, GrpcChannelExtrasProtocol and
  GrpcObservabilityExtrasProtocol are exported from the package root, where the
  other eleven protocols of that module already were. RetryMetricsProtocol and
  CircuitBreakerMetricsProtocol joined protocols.__all__, which a star import had
  been missing. MethodCircuitState left circuit_breaker.__all__: it is mutable
  internal bookkeeping in no public signature, and it stays importable by name.
* The health guide claimed the factory forwards neither options nor compression.
  It forwards both. It also placed HealthCheckerNotRunningError in the health
  module behind the health extra; the error is in grpc_client_kit.errors, is a
  root export, and catching it needs no extra. And probes are not always about
  the overall server: the checker takes a service name and the factory forwards
  it.
* The resilience and deadline guides both said there is no settings block for
  their layer. There is one, and the factory reads it.
* hasattr(grpc_client_kit, "HealthChecker") raising ImportError on an install
  without the extra is deliberate -- downgrading it would make a broken install
  look like a name that never existed -- and is now written down where a caller
  looks, rather than being folklore.
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