refactor: replace InstrumentNotReadyWarning with is_configured classmethod + summary log#107
Merged
Merged
Conversation
…ethod + summary log PR #86 introduced InstrumentNotReadyWarning, escalating the not-ready skip path from a (silently dropped) logger.info call to a real warnings.warn. The escalation surfaced expected-path events as warnings: every service using a subset of available instruments emits N warnings on bootstrap, and lite-bootstrap's own tests can't suppress without breaking the warning assertions. Replace is_ready (instance method, ran after instantiation) with is_configured (classmethod taking config, runs before instantiation). The bootstrapper now: 1. is_configured(config) False → silent skip; append to new bootstrapper.skipped_instruments: list[tuple[type, str]]. 2. check_dependencies() False → InstrumentDependencyMissingWarning (kept; this is the genuine "configured but dep missing" deployment surprise). 3. Otherwise instantiate and register. After the loop, emit one INFO-level summary log listing configured + skipped instruments. Default Python logging suppresses INFO, so silent by default with an opt-in path via logging.basicConfig. PR #88's constraint (check_dependencies before instantiation, because some instruments have default_factory that NameErrors when optional extras aren't installed) is preserved: is_configured is a classmethod, so it runs without instantiation. Production-code subtlety: bootstrappers/base.py uses a fresh logger per call via _get_logger() instead of a module-level cached one. The module-level structlog.getLogger(__name__) interacts badly with LoggingInstrument's cache_logger_on_first_use=True — the BoundLogger gets cached with whatever processors were active at first use, and structlog.testing.capture_logs() can't override the cache. The fresh-per-call approach keeps the structlog pipeline reactive. Removed: - InstrumentNotReadyWarning class (hard removal; 4 weeks old; exports go too) - BaseInstrument.is_ready instance method (replaced by is_configured) - The _register_or_skip helper (flow inlined) - The dead `import_checker.is_prometheus_client_installed` conjunct in FastStreamPrometheusInstrument.is_configured (covered by check_dependencies()). Kept: - InstrumentSkippedWarning (base for forward-compat) - InstrumentDependencyMissingWarning (still useful for the genuine deployment surprise) - not_ready_message class attribute (used in skipped_instruments tuples and the summary log) - Bootstrapper-level is_ready methods (framework availability checks; unrelated) Tests migrated to call XInstrument.is_configured(config) instead of instrument.is_ready(). test_free_bootstrap_logging_disabled rewritten to assert on bootstrapper.skipped_instruments. New test_free_bootstrap_emits_summary_log pins the summary log behavior via capture_logs. Framework instrument overrides (FastStreamOpenTelemetryInstrument, FastStreamPrometheusInstrument, LitestarSwaggerInstrument) need `# ty: ignore[invalid-method-override]` because they narrow bootstrap_config to framework-specific types — LSP violation in static type but correct at runtime (each framework bootstrapper only calls them with its own config). 22 files modified (15 production + 6 tests + 2 docs). Closes design doc 2026-06-01-instrument-skip-rework-design.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
lesnik512
force-pushed
the
refactor/instrument-skip-rework
branch
from
36e91e0 to
f404d0a
Compare
Replace the _get_logger() structlog indirection with a module-level logging.getLogger(__name__). The two log sites (INFO summary in __init__, WARNING teardown error) describe bootstrapper lifecycle, not application events, so stdlib logging is appropriate. Removes a global-structlog-state landmine introduced by the previous fresh-per-call workaround. Add BaseBootstrapper.build_summary() returning a multi-line human-readable description of configured + skipped instruments. __init__ uses it for the INFO summary; users can call it post-construction for debugging. Migrate the two test sites that used structlog.testing.capture_logs() to pytest's caplog fixture.
Asserts that constructing a bootstrapper with instruments whose config indicates they should not run emits zero UserWarnings. Without this regression test, a future change that re-introduces warnings.warn to the is_configured=False branch would pass CI.
CLAUDE.md and configuration.md now describe the build_summary() method and the stdlib logging backend. The previous instrument-skip-rework plan gets a supersession note pointing at the new design.
Add a one-liner to build_summary()'s docstring noting that calling it before __init__ completes raises AttributeError — important context for anyone wiring it to a health endpoint that might be invoked during a bootstrapper construction failure path. Tighten test_free_bootstrap_emits_summary_log to assert on " configured:" and " skipped:" (with the two-space indent) so it matches the precision of test_build_summary_format.
…ning leak Add test_build_summary_renders_none_for_empty_sections to exercise the (none) fallback for empty instruments and skipped_instruments lists, closing the 1-line gap reported by coverage on bootstrappers/base.py:44. Wrap the opentelemetry-missing construction in test_faststream_bootstrap_without_opentelemetry with pytest.warns so the expected InstrumentDependencyMissingWarning is captured at the call site instead of leaking into the test session output.
…ocstring Wrap the __init__ summary log in `if logger.isEnabledFor(logging.INFO)` so build_summary() doesn't run when INFO is filtered. Bootstrap fires once per service so the savings are negligible, but the guard is the conventional stdlib pattern and matters more if a subclass overrides build_summary() with something expensive. Drop the "Raises AttributeError if called before __init__ completes" line from the docstring — it documented an implementation detail rather than a contract. The "post-construction debugging" phrasing in the prior sentence already conveys the constraint.
lesnik512
added a commit
that referenced
this pull request
Jun 2, 2026
PR #107 (instrument skip rework) shipped a few days after the original arc closed. Captured three new datapoints worth recording: - Mid-design pivot worked: user pushback on the is_configured arg forced the design through pre-#88 history and confirmed the classmethod-with-arg pattern was correct. - Defensive workaround was wrong design: my _get_logger() fresh-per- call structlog proxy made tests pass but the user's pivot to stdlib logging + public build_summary() was the actual right answer. caplog (which the original plan flagged but the subagent ignored) was the correct test mechanism. - LSP violations on classmethod parameter overrides are an emergent pattern: 3 framework instrument overrides needed # ty: ignore[invalid-method-override] because they narrow bootstrap_config covariantly. Existing field-narrowing pattern is accepted; method-param narrowing now needs the explicit ignore. Added action items #8 (workaround-vs-framework-choice heuristic), #9 (cap subagent dispatch scope; the ~60-minute connection drop orphaned work), and #10 (document the LSP override pattern in CLAUDE.md). Reaffirms the original retro's closing observation: subagent loop reliably produces a green-tests implementation of the spec, but the spec is rarely the right design. Design emerges during review iteration. PR #107 needed 5 user follow-up commits after the mechanical migration landed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lesnik512
added a commit
that referenced
this pull request
Jun 2, 2026
`just test` previously reported coverage as informational only; a regression to 99% (or any non-100% value) would still exit 0 and nothing flagged it. The PR #107 implementer's coverage drop to 99% slipped past `just test` and surfaced only when the user manually reviewed the totals. Match faststream-outbox's pytest config: append `--cov-fail-under=100` to addopts so any drop below 100% fails the test run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
PR #86 introduced
InstrumentNotReadyWarning, escalating the not-ready skip path from a (silently dropped)logger.infoto a realwarnings.warn. The escalation surfaced expected-path events as warnings: every service using a subset of available instruments emits N warnings on bootstrap, and lite-bootstrap's own tests couldn't suppress without breaking the warning assertions.This PR replaces the API:
is_readyinstance method →is_configuredclassmethod taking config, runs before instantiationInstrumentNotReadyWarningremoved (hard delete; 4 weeks old)bootstrapper.skipped_instruments: list[tuple[type, str]]introspection__init__(silent by default; opt-in vialogging.basicConfig)InstrumentDependencyMissingWarningkept for the genuine deployment surprise (configured but optional dep missing)is_configuredis a classmethod so it runs without instantiationProduction-code subtlety: fresh logger per call
bootstrappers/base.pynow uses_get_logger()(fresh per call) instead of a module-level cached logger. Reason:LoggingInstrument.bootstrap()setscache_logger_on_first_use=Trueon structlog. A module-level cachedBoundLoggerwould be memoized with whatever processors were active at first use, andstructlog.testing.capture_logs()couldn't override. The fresh-per-call approach keeps the structlog pipeline reactive — which is what makes the new summary-log test (and existing teardown-error tests) work across the full test suite.Scope
22 files modified (15 production + 6 tests + 2 docs). Bootstrapper-level
is_readymethods (framework availability checks like "fastapi is not installed") are unchanged — only instrument-level migrates.The framework instrument overrides (
FastStreamOpenTelemetryInstrument,FastStreamPrometheusInstrument,LitestarSwaggerInstrument) carry# ty: ignore[invalid-method-override]because they narrowbootstrap_configto framework-specific types — LSP violation in static type but correct at runtime (each framework bootstrapper only calls them with its own config). Matches the existing covariant-narrowing pattern already accepted forbootstrap_config:field overrides.Also opportunistically drops the dead
import_checker.is_prometheus_client_installedconjunct inFastStreamPrometheusInstrument(covered bycheck_dependencies()— per audit finding DES-5).Closes design
docs/superpowers/specs/2026-06-01-instrument-skip-rework-design.md.Test plan
just test— 150/150 passing.just lint— clean (ruff, ty).test_*_bootstrapper_with_missing_instrument_dependencytests still pass —InstrumentDependencyMissingWarningcontinues to fire for the configured + dep-missing case.test_free_bootstrap_emits_summary_logpins the summary log behavior viacapture_logs.test_free_bootstrap_logging_disabledrewritten to assert onbootstrapper.skipped_instrumentsinstead ofpytest.warns(InstrumentNotReadyWarning).Backward compatibility
InstrumentNotReadyWarningis publicly exported and hard-removed. User code importing it getsImportError. 4 weeks old; acceptable churn.is_readyinstance method on instruments removed. User code callinginstrument.is_ready()migrates totype(instrument).is_configured(config). Library-internal lifecycle; rare in user code.🤖 Generated with Claude Code