From d88249cec35f8de2bcb2157cd602b07619826efe Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 12:56:12 +0300 Subject: [PATCH 1/7] feat: make the OpenTelemetry sampler configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `OpenTelemetryInstrument.bootstrap()` built the provider without a sampler, so `parentbased_always_on` applied unless the environment overrode it, and there was no way to express a sampler from Python. #184 measured ~55 µs/request between always-on and `ParentBased(TraceIdRatioBased(0.01))` on an endpoint whose bare cost is 16 µs. `opentelemetry_sampler` takes a `Sampler` rather than the sample-rate float the issue leaned towards: the config already carries `BaseInstrumentor` and Sentry's types as quoted `TYPE_CHECKING` annotations, so the "keep SDK types out of the config" argument for a float does not apply here, and a float would leave `ALWAYS_OFF`, a bare `TraceIdRatioBased` and vendor samplers unreachable behind an instrument that owns provider construction and offers no escape hatch. A float can still be layered on later. ADR-0010 records it. The field is passed unconditionally. Contrary to the issue, `TracerProvider(sampler=None)` is identical to omitting the argument — the SDK does `if not sampler: sampler = _get_from_env_or_default()`, at the declared 1.28 floor and at 1.44 alike — so the default is unchanged and `OTEL_TRACES_SAMPLER` keeps working when the field is unset. Both are pinned by tests. --- .../0010-otel-sampling-is-a-sampler-object.md | 42 ++++++++++++ docs/introduction/configuration.md | 14 ++++ .../instruments/opentelemetry_instrument.py | 6 +- .../test_opentelemetry_instrument.py | 68 +++++++++++++++++++ tests/test_free_bootstrap.py | 21 ++++++ 5 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 docs/adr/0010-otel-sampling-is-a-sampler-object.md diff --git a/docs/adr/0010-otel-sampling-is-a-sampler-object.md b/docs/adr/0010-otel-sampling-is-a-sampler-object.md new file mode 100644 index 0000000..ccf9b41 --- /dev/null +++ b/docs/adr/0010-otel-sampling-is-a-sampler-object.md @@ -0,0 +1,42 @@ +# OpenTelemetry sampling takes a `Sampler`, not a sample-rate float + +**Decision:** `OpenTelemetryConfig.opentelemetry_sampler` takes an `opentelemetry.sdk.trace.sampling.Sampler` +and is handed to `TracerProvider(sampler=...)` unconditionally. Left `None` — the default — the SDK +picks its own sampler, which honours `OTEL_TRACES_SAMPLER` / `OTEL_TRACES_SAMPLER_ARG`, so the +default behaviour and the existing environment-variable path are both unchanged. + +## Context + +`OpenTelemetryInstrument.bootstrap()` built the provider without a sampler, so `parentbased_always_on` +applied unless the environment said otherwise. #184 measured always-on tracing at ~55 µs/request over +`ParentBased(TraceIdRatioBased(0.01))` on an endpoint whose bare cost is 16 µs, and the instrument +owns provider construction, so there was no way to express a sampler from Python. + +`TracerProvider(sampler=None)` is identical to omitting the argument: the SDK does +`if not sampler: sampler = sampling._get_from_env_or_default()`, at the declared 1.28 floor and at +1.44 alike. The pass-through therefore needs no conditional and no sentinel default. + +## Rejected alternatives + +**`opentelemetry_traces_sample_rate: float | None`, mapped internally to `ParentBased(TraceIdRatioBased(rate))`.** +Mirrors `sentry_traces_sample_rate` and keeps `opentelemetry.sdk` out of the config's annotations. Two +problems. The annotation argument does not hold: `opentelemetry_instrumentors` already carries +`BaseInstrumentor`, and `sentry_integrations` / `sentry_before_send` carry Sentry types, all as quoted +annotations over `TYPE_CHECKING` imports that never run. And a float closes only part of the gap: +`ALWAYS_OFF`, a bare `TraceIdRatioBased` without the `ParentBased` wrapper, and vendor samplers all +stay unreachable, and this config has no `opentelemetry_additional_params` escape hatch to reach them +through. A float can still be layered on the `Sampler` field later; the reverse ordering would have +shipped a second field to finish the job. + +**Documenting `OTEL_TRACES_SAMPLER` and adding nothing.** It is a real surface and it keeps working, +but it is the only one, and it cannot express a custom sampler at all. Every other instrument takes +its configuration from the config object; tracing's sample rate should not be the exception that lives +in the environment. + +**Validating the sampler in `__post_init__`.** Nothing to validate: `TraceIdRatioBased` already +rejects a rate outside [0.0, 1.0] at construction, which is where a float field would have needed a +range check of our own. + +**Revisit trigger:** a request to drive the sampler from a settings object (pydantic-settings and +friends map an env var to a float, not to a `Sampler`), which is when the float becomes sugar worth +adding alongside — with this field winning where both are set. diff --git a/docs/introduction/configuration.md b/docs/introduction/configuration.md index f8fb32c..0e35807 100644 --- a/docs/introduction/configuration.md +++ b/docs/introduction/configuration.md @@ -78,9 +78,23 @@ Additional parameters: - `opentelemetry_insecure` - whether the gRPC OTLP connection is insecure (gRPC only; for `http` the endpoint URL scheme carries security). - `opentelemetry_instrumentors` - a list of extra instrumentors. - `opentelemetry_log_traces` - traces will be logged to stdout. +- `opentelemetry_sampler` - an `opentelemetry.sdk.trace.sampling.Sampler` deciding which traces are recorded. Unset, the SDK's own default applies: `parentbased_always_on`, which records every trace that is not the child of a non-recording remote parent, unless `OTEL_TRACES_SAMPLER` / `OTEL_TRACES_SAMPLER_ARG` say otherwise. - `opentelemetry_generate_health_check_spans` - generate spans for health check handlers if `True`. - `opentelemetry_excluded_urls` - extra URLs excluded from tracing; the metrics path and (unless health-check spans are enabled) the health-check path are excluded automatically. +Sampling is the cheapest way to cut what tracing costs: on a benchmark endpoint returning a constant, +`ParentBased(TraceIdRatioBased(0.01))` saved ~55 µs per request against the always-on default. + +```python +from opentelemetry.sdk.trace.sampling import ParentBased, TraceIdRatioBased + +config = FastAPIConfig( + service_name="my-service", + opentelemetry_endpoint="otl", + opentelemetry_sampler=ParentBased(TraceIdRatioBased(0.01)), +) +``` + For FastStream you must provide additionally: - `opentelemetry_middleware_cls` diff --git a/lite_bootstrap/instruments/opentelemetry_instrument.py b/lite_bootstrap/instruments/opentelemetry_instrument.py index d30d03f..edc03b7 100644 --- a/lite_bootstrap/instruments/opentelemetry_instrument.py +++ b/lite_bootstrap/instruments/opentelemetry_instrument.py @@ -12,6 +12,7 @@ if typing.TYPE_CHECKING: from opentelemetry.instrumentation.instrumentor import BaseInstrumentor + from opentelemetry.sdk.trace.sampling import Sampler if import_checker.is_opentelemetry_sdk_installed: from opentelemetry.context import Context @@ -69,6 +70,9 @@ class OpenTelemetryConfig(OpenTelemetryServiceFieldsConfig): default_factory=list ) opentelemetry_log_traces: bool = False + # Left None, the SDK picks its own default, which reads OTEL_TRACES_SAMPLER; see ADR-0010 + # (docs/adr/0010-otel-sampling-is-a-sampler-object.md). + opentelemetry_sampler: "Sampler | None" = None opentelemetry_generate_health_check_spans: bool = True opentelemetry_excluded_urls: list[str] = dataclasses.field(default_factory=list) @@ -228,7 +232,7 @@ def _apply_instrumentors(self, tracer_provider: "TracerProvider") -> None: def bootstrap(self) -> None: config = self.bootstrap_config self._silence_otel_loggers() - tracer_provider = TracerProvider(resource=self._build_resource()) + tracer_provider = TracerProvider(resource=self._build_resource(), sampler=config.opentelemetry_sampler) set_tracer_provider(tracer_provider) self._tracer_provider = tracer_provider if import_checker.is_pyroscope_installed and getattr(config, "pyroscope_endpoint", None): diff --git a/tests/instruments/test_opentelemetry_instrument.py b/tests/instruments/test_opentelemetry_instrument.py index 0ab0c78..1a37cd4 100644 --- a/tests/instruments/test_opentelemetry_instrument.py +++ b/tests/instruments/test_opentelemetry_instrument.py @@ -6,6 +6,7 @@ import pytest from opentelemetry.instrumentation.instrumentor import BaseInstrumentor +from opentelemetry.sdk.trace import sampling import lite_bootstrap.instruments.opentelemetry_instrument as otel_module from lite_bootstrap import import_checker @@ -46,6 +47,73 @@ def test_opentelemetry_instrument_empty_instruments() -> None: opentelemetry_instrument.teardown() +_SAMPLER_ENV_VARS: typing.Final = ("OTEL_TRACES_SAMPLER", "OTEL_TRACES_SAMPLER_ARG") + + +@pytest.fixture +def without_sampler_env(monkeypatch: pytest.MonkeyPatch) -> None: + for env_var in _SAMPLER_ENV_VARS: + monkeypatch.delenv(env_var, raising=False) + + +@pytest.mark.usefixtures("without_sampler_env") +def test_opentelemetry_sampler_reaches_tracer_provider() -> None: + sample_rate = 0.01 + sampler = sampling.ParentBased(sampling.TraceIdRatioBased(sample_rate)) + instrument = OpenTelemetryInstrument( + bootstrap_config=OpenTelemetryConfig(opentelemetry_log_traces=True, opentelemetry_sampler=sampler), + ) + try: + instrument.bootstrap() + assert instrument._tracer_provider is not None # noqa: SLF001 + assert instrument._tracer_provider.sampler is sampler # noqa: SLF001 + finally: + instrument.teardown() + + +@pytest.mark.usefixtures("without_sampler_env") +def test_opentelemetry_sampler_unset_keeps_sdk_default() -> None: + instrument = OpenTelemetryInstrument(bootstrap_config=OpenTelemetryConfig(opentelemetry_log_traces=True)) + try: + instrument.bootstrap() + assert instrument._tracer_provider is not None # noqa: SLF001 + assert instrument._tracer_provider.sampler is sampling.DEFAULT_ON # noqa: SLF001 + finally: + instrument.teardown() + + +def test_opentelemetry_sampler_unset_honours_sampler_env_vars(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("OTEL_TRACES_SAMPLER", "traceidratio") + monkeypatch.setenv("OTEL_TRACES_SAMPLER_ARG", "0.25") + instrument = OpenTelemetryInstrument(bootstrap_config=OpenTelemetryConfig(opentelemetry_log_traces=True)) + try: + instrument.bootstrap() + assert instrument._tracer_provider is not None # noqa: SLF001 + sampler = instrument._tracer_provider.sampler # noqa: SLF001 + assert isinstance(sampler, sampling.TraceIdRatioBased) + env_rate = 0.25 + assert sampler.rate == env_rate + finally: + instrument.teardown() + + +def test_opentelemetry_sampler_wins_over_sampler_env_vars(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("OTEL_TRACES_SAMPLER", "traceidratio") + monkeypatch.setenv("OTEL_TRACES_SAMPLER_ARG", "0.25") + instrument = OpenTelemetryInstrument( + bootstrap_config=OpenTelemetryConfig( + opentelemetry_log_traces=True, + opentelemetry_sampler=sampling.ALWAYS_OFF, + ), + ) + try: + instrument.bootstrap() + assert instrument._tracer_provider is not None # noqa: SLF001 + assert instrument._tracer_provider.sampler is sampling.ALWAYS_OFF # noqa: SLF001 + finally: + instrument.teardown() + + def test_opentelemetry_instrument_teardown_shuts_down_tracer_provider() -> None: instrument = OpenTelemetryInstrument( bootstrap_config=OpenTelemetryConfig(opentelemetry_log_traces=True), diff --git a/tests/test_free_bootstrap.py b/tests/test_free_bootstrap.py index 8b37243..d8d1732 100644 --- a/tests/test_free_bootstrap.py +++ b/tests/test_free_bootstrap.py @@ -5,6 +5,7 @@ import pytest import structlog +from opentelemetry.sdk.trace import sampling from lite_bootstrap import ( FreeBootstrapper, @@ -14,6 +15,7 @@ ) from lite_bootstrap.bootstrappers.base import BaseBootstrapper from lite_bootstrap.instruments.logging_instrument import LoggingInstrument +from lite_bootstrap.instruments.opentelemetry_instrument import OpenTelemetryInstrument from lite_bootstrap.instruments.pyroscope_instrument import PyroscopeInstrument from lite_bootstrap.instruments.sentry_instrument import SentryInstrument from tests.conftest import CustomInstrumentor, SentryTestTransport, emulate_package_missing @@ -42,6 +44,25 @@ def test_free_bootstrap(free_bootstrapper_config: FreeConfig) -> None: bootstrapper.teardown() +def test_free_bootstrap_passes_sampler_to_tracer_provider() -> None: + sampler = sampling.ParentBased(sampling.TraceIdRatioBased(0.01)) + bootstrapper = FreeBootstrapper( + bootstrap_config=FreeConfig( + opentelemetry_log_traces=True, + opentelemetry_sampler=sampler, + logging_buffer_capacity=0, + ), + ) + bootstrapper.bootstrap() + try: + instruments = [one for one in bootstrapper.instruments if isinstance(one, OpenTelemetryInstrument)] + assert len(instruments) == 1 + assert instruments[0]._tracer_provider is not None # noqa: SLF001 + assert instruments[0]._tracer_provider.sampler is sampler # noqa: SLF001 + finally: + bootstrapper.teardown() + + def test_free_bootstrap_logging_disabled() -> None: bootstrapper = FreeBootstrapper( bootstrap_config=FreeConfig( From fca2f4192c49e92c7686af6f2e7383fca5d0ec35 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 13:04:22 +0300 Subject: [PATCH 2/7] docs: compress the ADRs to the domain-modeling format The `**Decision:** / ## Context / ## Rejected alternatives / **Revisit trigger:**` shape had grown to 3,421 words across ten files, most of it scaffolding. The domain-modeling skill's format is a title plus a short body, with sections only where they earn their place; revisit conditions fold into the prose. Two merges, no drops. 0004 (exclusion policy stays in one method) joins 0002: both record a rejected re-organization refused for the same reason, that it moves complexity rather than concentrating it. 0006 (OTLP-HTTP as a sibling extra) joins 0005: both are the same decision applied twice - a native dependency with no free-threaded wheels becomes an opt-in extra, never a combinatorial matrix of per-framework variants. Numbers 0004 and 0006 are retired rather than reused, so every surviving citation still resolves. 3,421 words -> 1,423. The floor is set by the facts a reader opens these for: the `wrap_for_formatter` incompatibility, pydantic's `TypedDict` refusal below 3.12, the four framework binding styles. --- ...-fastmcp-teardown-via-provider-lifespan.md | 41 ++++------- .../adr/0002-cross-cutting-logic-stays-put.md | 23 ++++++ docs/adr/0002-keep-per-instrument-axis.md | 49 ------------- .../0003-teardown-marker-accepted-limits.md | 68 +++++------------- .../adr/0004-excluded-urls-stay-one-method.md | 25 ------- ...005-native-dependency-extras-are-opt-in.md | 20 ++++++ docs/adr/0005-orjson-is-opt-in.md | 36 ---------- docs/adr/0006-otlp-http-exporter-shape.md | 33 --------- .../0007-core-declares-typing-extensions.md | 45 +++--------- ...structlog-sentry-seam-stays-a-heuristic.md | 71 ++++++------------- docs/adr/0009-no-django-bootstrapper.md | 22 ++---- .../0010-otel-sampling-is-a-sampler-object.md | 53 ++++---------- 12 files changed, 126 insertions(+), 360 deletions(-) create mode 100644 docs/adr/0002-cross-cutting-logic-stays-put.md delete mode 100644 docs/adr/0002-keep-per-instrument-axis.md delete mode 100644 docs/adr/0004-excluded-urls-stay-one-method.md create mode 100644 docs/adr/0005-native-dependency-extras-are-opt-in.md delete mode 100644 docs/adr/0005-orjson-is-opt-in.md delete mode 100644 docs/adr/0006-otlp-http-exporter-shape.md diff --git a/docs/adr/0001-fastmcp-teardown-via-provider-lifespan.md b/docs/adr/0001-fastmcp-teardown-via-provider-lifespan.md index 051b001..a889a83 100644 --- a/docs/adr/0001-fastmcp-teardown-via-provider-lifespan.md +++ b/docs/adr/0001-fastmcp-teardown-via-provider-lifespan.md @@ -1,30 +1,15 @@ # FastMCP teardown attaches through a Provider lifespan -**Decision:** `FastMcpBootstrapper` wires its `teardown` into FastMCP's shutdown by registering a -`_TeardownProvider` via the public `FastMCP.add_provider()`, whose `async def lifespan(self)` runs -teardown on the exit branch. We will not reach into `FastMCP._lifespan`, rebuild the user's -`FastMCP`, or leave teardown manual. - -FastMCP is the one supported framework with no `on_shutdown`-shaped hook. `FastMCP.lifespan` is a -bound method on the `AggregateProvider` mixin, not a settable attribute: assigning `app.lifespan` -succeeds and has zero runtime effect, because the transport runners read the private `_lifespan` -attribute that is only set at construction time. `add_provider()` is the sole public, -post-construction hook whose callback is invoked by the server's ASGI lifespan. - -Rejected, with the reasoning that would otherwise be re-litigated: - -- **Mutate `app._lifespan` directly.** Works today, but it is private API on a fast-moving - dependency, and a rename ships as a silent no-teardown rather than an error. -- **Rebuild the user's `FastMCP` with a composed `lifespan=`.** Breaks the contract every other - bootstrapper keeps — the user owns the application object they passed in, and gets the same - object back. -- **No automatic wiring; document a manual `teardown()` call.** Adopted briefly and reverted once - `add_provider` was found. It makes FastMCP the only framework where shutdown is the user's job. - -The accepted cost is semantic: `Provider` is FastMCP's general extension abstraction for tools, -resources and prompts, and using one purely for a shutdown callback is thin. A one-line comment at -the registration site says so. - -**Revisit trigger:** FastMCP grows a first-class shutdown hook (an `on_shutdown` API, or a -documented public way to compose a lifespan post-construction). At that point the provider is the -indirect route and should be replaced by the direct one. +FastMCP is the one supported framework with no `on_shutdown`-shaped hook: `FastMCP.lifespan` is a +bound method on the `AggregateProvider` mixin, so assigning to it succeeds and does nothing — the +transport runners read the private `_lifespan`, set only at construction — which leaves +`add_provider()` as the single public post-construction hook the server's ASGI lifespan actually +invokes. `FastMcpBootstrapper` therefore registers a `_TeardownProvider` whose `lifespan` runs +teardown on the exit branch, accepting that `Provider` is FastMCP's extension abstraction for tools, +resources and prompts and that using one for a shutdown callback is thin. + +Rejected: mutating `app._lifespan` (private API on a fast-moving dependency, where a rename ships as +silent no-teardown rather than an error); rebuilding the user's `FastMCP` with a composed `lifespan=` +(every other bootstrapper hands back the object it was given); and documenting a manual `teardown()` +call (adopted briefly, reverted once `add_provider` was found). If FastMCP grows a real shutdown +hook, the provider becomes the indirect route and should be replaced. diff --git a/docs/adr/0002-cross-cutting-logic-stays-put.md b/docs/adr/0002-cross-cutting-logic-stays-put.md new file mode 100644 index 0000000..e92a502 --- /dev/null +++ b/docs/adr/0002-cross-cutting-logic-stays-put.md @@ -0,0 +1,23 @@ +# Cross-cutting logic stays where it is: per-instrument, and in one method + +Two re-organizations have been proposed and rejected for the same reason — each moves complexity +rather than concentrating it, and the matrix is inherently O(instruments × frameworks) whichever axis +it is cut along. + +The instrument × framework matrix keeps its **per-instrument axis**: base instruments own the hoisted +logic and each framework is a thin `bootstrap()` subclass, rather than per-framework adapters driving +generic instruments. The bindings genuinely differ — FastAPI imperative (`add_middleware`, +`include_router`), Litestar declarative before the app is built, FastStream attaching to a broker, +FastMCP via `custom_route` — so a uniform `add_route`/`add_middleware` interface would leak; and +framework-locality already exists, because all of a framework's subclasses live in its one +bootstrapper file. Reconsider if the per-cell bindings start converging, or if a new framework shares +an existing one's attach mechanism exactly. + +`_build_excluded_urls` likewise keeps the whole trace-exclusion policy in **one method** that reads +its siblings' paths through `getattr(..., None)` — those sibling configs are genuinely optional, so +the defensive read is correct rather than a smell. A per-instrument contribution mechanism would +spread the policy across `PrometheusConfig`, `HealthChecksConfig` and OpenTelemetry while still +needing `opentelemetry_generate_health_check_spans`, so the coupling would not even disappear. The +real risk — renaming `prometheus_metrics_path` and silently stopping the exclusion — is answered by a +test pinning each sibling path in the built set. Reconsider when a user-supplied instrument needs +paths the policy cannot know about. diff --git a/docs/adr/0002-keep-per-instrument-axis.md b/docs/adr/0002-keep-per-instrument-axis.md deleted file mode 100644 index 8813a4d..0000000 --- a/docs/adr/0002-keep-per-instrument-axis.md +++ /dev/null @@ -1,49 +0,0 @@ -# Keep the per-instrument axis for the instrument × framework matrix - -**Decision:** The framework-binding code stays organized around the *instrument* — base instrument -classes own the shared logic, and each framework is a thin subclass overriding only its `bootstrap()` -binding. We reject inverting to a per-framework adapter axis (a -`FastAPIAdapter`/`LitestarAdapter`/… that knows how to attach any instrument, driven by generic -instruments). - -## Context - -The codebase has an instrument × framework matrix: every filled cell is "how instrument *I* binds to -framework *F*" (e.g. `FastAPIHealthChecksInstrument`, `LitestarPrometheusInstrument`). The -2026-06-23 architecture review raised the matrix as candidate 3 — "no framework-locality; ~28 shallow -per-cell subclasses" — and proposed inverting the axis so a per-framework adapter owns the binding -and instruments become generic. - -## Decision & rationale - -The candidate's premise and payoff do not hold up: - -- **Framework-locality already exists at the file level.** All of a framework's instrument - subclasses live in its one bootstrapper file, so "what does lite-bootstrap do to my FastAPI app" - is already answered by reading one file. The review's "five scattered classes" are co-located, - not scattered. -- **The shared depth is already hoisted.** `render_health_check_data()` lives in the base - `HealthChecksInstrument`; provider setup in base `OpenTelemetryInstrument`; config validation in - the base configs. The per-framework subclasses contain *only* the genuinely-different binding, - which is what you want — the thinness is the result of correct hoisting, not shallowness to fix. -- **The N×M bindings differ genuinely.** FastAPI is imperative (`app.add_middleware`, - `include_router`), Litestar is declarative *before the app is built* (`application_config.cors_config = …`, - append to `middleware`/`route_handlers`), FastStream attaches middleware to a *broker* not the - app, FastMCP uses `custom_route`. A uniform adapter interface (`add_route`/`add_middleware`) would - have to paper over imperative-vs-declarative-vs-broker and normalize differing handler return - types — it would leak. -- **Deletion test fails for the inversion.** Inverting relocates the same N×M genuinely-different - bindings into framework-grouped adapters; the complexity *moves*, it does not *concentrate*. The - matrix is inherently O(instruments × frameworks); no axis choice removes a cell. Adding an - instrument touches every framework either way; adding a framework touches every instrument either - way. - -No friction worth a refactor was identified: navigation is satisfied by the file layout, the -cross-cutting change cost is inherent to the matrix, and the per-cell "shallowness" is hoisted-out -depth rather than duplication. - -**Revisit trigger:** the per-cell bindings start genuinely converging — the shared part outgrows the -base instrument and the same binding code appears across framework subclasses; then hoist the -convergent part and reconsider an adapter for that specific shared mechanism. Or a new framework -arrives that shares an existing framework's attach mechanism (another ASGI app driven exactly like -FastAPI), turning the hypothetical seam into a real one for that pair. diff --git a/docs/adr/0003-teardown-marker-accepted-limits.md b/docs/adr/0003-teardown-marker-accepted-limits.md index e3d374c..05cedf2 100644 --- a/docs/adr/0003-teardown-marker-accepted-limits.md +++ b/docs/adr/0003-teardown-marker-accepted-limits.md @@ -1,52 +1,20 @@ # The teardown-attach guard is an attribute marker, and its two limits are accepted -**Decision:** `BaseBootstrapper._attach_teardown_once` detects a second bootstrapper by tagging the -attach target with the `_lite_bootstrap_teardown_attached` attribute (#130). Two consequences of -that choice — FastMCP detecting via the attribute rather than a provider-list scan, and Litestar -tagging the shared `AppConfig` rather than a built app — are accepted rather than designed around. - -## Why an attribute on the target - -Rejected: **a class-level registry or `WeakSet` of already-attached applications.** It would -contradict the `_lite_bootstrap_*` app-tagging convention the codebase already follows, and it -introduces process-global mutable state with the test-isolation hazards that come with it. The -marker keeps the "attached" bit local to the application's own lifetime, which is exactly the -lifetime the fact is true for. - -Rejected: **a free-function helper module.** The guard is bootstrapper-lifecycle logic and belongs on -`BaseBootstrapper` next to `teardown()`, where `type(self).__name__` is available for the warning. - -## The two accepted limits - -1. **FastMCP detects via the marker, not structurally.** FastMCP previously detected double-attach - by scanning `any(isinstance(p, _TeardownProvider) for p in app.providers)`, which reads the actual - attach state. If the providers list is cleared after the first bootstrap while the marker - survives, the second bootstrapper is refused rather than re-attaching. Accepted: uniformity across - all four app-bearing bootstrappers is worth more than the sliver of state-accuracy the scan gave, - the only scenario the guard exists for is detected identically either way, and the regression - requires user or framework code to mutate `app.providers` after bootstrap, which no supported flow - does. Keeping FastMCP on a bespoke structural check would re-fragment detection and defeat the - seam's whole point. - -2. **Litestar tags the `AppConfig`.** The Litestar app does not exist at `__init__` time — it is - built later by `Litestar.from_config()` — so the attach, and therefore the marker, lands on - `application_config`. Two `LitestarConfig` instances that *share* one `AppConfig` but intend two - distinct apps will collide. Accepted: config-level is the only option while the app is built - lazily, and sharing one mutable `AppConfig` across two intended apps is already broken - independently of teardown — instrument bootstrap mutates the shared config's `cors_config`, - `route_handlers` and `openapi_config`. The marker collision is one symptom of an - already-unsupported pattern, not a new hazard. - -The genuinely actionable finding from the same review — the marker being set before a fallible -`attach()` — was *fixed*, not accepted: the target is tagged only after `attach()` returns. - -Since #167 the consequence of hitting the marker is louder in both cases: the losing bootstrapper's -`bootstrap()` raises `ConfigurationError` before any instrument is applied, rather than warning and -leaving a half-wired application whose teardown never runs. The marker and these two limits are -unchanged. - -**Revisit trigger:** for FastMCP, a supported flow starts mutating `FastMCP.providers` after -bootstrap (a documented hot-reload or provider-swap API), making the attribute diverge from real -attach state — then move back to structural detection or reconcile the two. For Litestar, sharing one -`AppConfig` across multiple apps becomes a supported pattern, or the attach is restructured to run at -`bootstrap()` time when the app exists — then tag the built `Litestar` app instead. +`BaseBootstrapper._attach_teardown_once` detects a second bootstrapper by tagging the attach target +with `_lite_bootstrap_teardown_attached`, so the "attached" bit lives exactly as long as the +application it describes — rather than in a class-level registry or `WeakSet`, which would mean +process-global mutable state and would contradict the `_lite_bootstrap_*` tagging convention the +codebase already follows. The target is tagged only after `attach()` returns, and since #167 hitting +the marker raises `ConfigurationError` from `bootstrap()` rather than warning and leaving a half-wired +application. + +Two consequences are accepted rather than designed around: + +- **FastMCP detects via the marker, not by scanning `app.providers`.** If that list is cleared after + the first bootstrap, the second bootstrapper is refused instead of re-attaching. No supported flow + mutates it, and uniformity across the four app-bearing bootstrappers is worth more than the sliver + of state-accuracy the structural check gave. +- **Litestar tags the shared `AppConfig`**, because the app does not exist until + `Litestar.from_config()`. Two configs sharing one `AppConfig` collide — but that pattern is already + broken independently, since instrument bootstrap mutates the shared config's `cors_config`, + `route_handlers` and `openapi_config`. diff --git a/docs/adr/0004-excluded-urls-stay-one-method.md b/docs/adr/0004-excluded-urls-stay-one-method.md deleted file mode 100644 index 282f917..0000000 --- a/docs/adr/0004-excluded-urls-stay-one-method.md +++ /dev/null @@ -1,25 +0,0 @@ -# Trace-exclusion policy stays in one method, not contributed per instrument - -**Decision:** `_build_excluded_urls` keeps the whole OpenTelemetry URL-exclusion policy in one -method that reads its sibling instruments' paths. We reject a contribution mechanism in which each -instrument or config declares the paths it wants excluded and OpenTelemetry unions them (#132). - -The method reads `prometheus_metrics_path` and `health_checks_path` off the config through defensive -`getattr(..., None)`, because a given framework config need not compose `PrometheusConfig` or -`HealthChecksConfig` at all — those are genuinely optional siblings, and the defensive read is the -correct expression of that, not a smell to refactor away. - -A contribution mechanism was the obvious alternative and fails the deletion test. Today the entire -exclusion policy is readable in one place; contributing would *spread* it across `PrometheusConfig`, -`HealthChecksConfig` and OpenTelemetry, and the health-check case would still need OpenTelemetry's -own `opentelemetry_generate_health_check_spans` flag, so the cross-coupling would not even -disappear. It moves complexity and worsens locality. - -The real risk in the sibling reads is silent breakage: rename `prometheus_metrics_path` and the -`getattr` returns `None`, the metrics endpoint quietly stops being excluded from traces, and nothing -fails. That is answered with a test that pins each sibling path in the built set, not with a -refactor. - -**Revisit trigger:** a third-party or user-supplied instrument needs its own paths excluded. The -policy would then have to name paths it cannot know about, which is precisely the case a contribution -mechanism exists for. diff --git a/docs/adr/0005-native-dependency-extras-are-opt-in.md b/docs/adr/0005-native-dependency-extras-are-opt-in.md new file mode 100644 index 0000000..66b40e1 --- /dev/null +++ b/docs/adr/0005-native-dependency-extras-are-opt-in.md @@ -0,0 +1,20 @@ +# Native-dependency extras are opt-in, and never per-framework variants + +Free-threaded CPython has wheels for neither `orjson` nor `grpcio`, and PEP 508 has no environment +marker for "GIL enabled", so neither can be required conditionally. Both are therefore opt-in: +`orjson` is its own extra (`logging` is `structlog` only) with the logging serializer falling back to +the stdlib `json` accelerator, and OTLP over HTTP is a sibling `otl-http` extra beside a gRPC-only +`otl`. The fallback is a documented performance change, not a correctness one — roughly 2-5x slower, +with non-JSON-native values in log `extra` rendering via `repr`; the GIL fast path is byte-for-byte +unchanged when `[orjson]` is installed. + +Both refuse the combinatorial alternative: `*-ft` twins of every logging-bearing and `*-all` extra, +or `fastapi-otl-http`-style framework variants. A free-threaded service composes `[fastapi, otl-http]` +itself. Replacing `orjson` with msgspec, ujson or rapidjson was also rejected — a permanent mandatory +dependency, with a different encoder API and output shape, to paper over a temporary gap +([ijl/orjson#530](https://github.com/ijl/orjson/issues/530) tracks ft wheels); if that resolves, +`orjson` could return to core and the fallback branch retire. + +The HTTP exporter deliberately carries no insecure-endpoint warning mirroring the gRPC one: it has no +`insecure` flag to inspect, only a full URL whose scheme is a stronger signal than anything +`__post_init__` could re-derive, and the user typed it explicitly. diff --git a/docs/adr/0005-orjson-is-opt-in.md b/docs/adr/0005-orjson-is-opt-in.md deleted file mode 100644 index cf06369..0000000 --- a/docs/adr/0005-orjson-is-opt-in.md +++ /dev/null @@ -1,36 +0,0 @@ -# `orjson` is an opt-in extra with a stdlib-`json` fallback - -**Decision:** `orjson` is its own extra (`lite-bootstrap[orjson]`), the `logging` extra is -`structlog` only, and the logging serializer falls back to the stdlib `json` accelerator when -`orjson` is absent. It is not bundled with `logging`/`*-all`, and it is not replaced by a -free-threading-ready native encoder. - -## Context - -`orjson` used to be a mandatory core dependency, used only by the logging serializer, and it -hard-blocks free-threaded installs: no ft wheels, and the build refuses to compile on ft (verified -on 3.14t). PEP 508 has **no environment marker for "GIL enabled"**, so `orjson` cannot be required -conditionally on GIL builds only. That missing marker is what forces the choice. - -## Rejected alternatives - -**Keep `orjson` reachable by default and add parallel `*-ft` extras that omit it.** Zero behaviour -change for GIL users, at the cost of an ft twin for every logging-bearing and `*-all` extra — -`fastapi-logging-ft`, `free-all-ft`, and so on. That is exactly the combinatorial extras sprawl this -project refuses; the same argument later rejected `*-otl-http` framework variants (ADR-0006). - -**Replace `orjson` with an ft-ready native encoder — msgspec, ujson, rapidjson.** A permanent new -mandatory dependency to paper over a *temporary* gap ([ijl/orjson#530](https://github.com/ijl/orjson/issues/530) -tracks ft wheels). msgspec's encoder API differs (`enc_hook`, not orjson's `default=`) and so does -its output shape, forcing a serializer rewrite and a test re-baseline — for a dependency that would -outlive the problem. - -The chosen shape keeps one coherent rule: `orjson` is a per-build opt-in speedup, and no extra drags -it in. It also fixed a standing hygiene defect — a JSON encoder had no business being a mandatory -core dependency. The GIL fast path is byte-for-byte unchanged when `[orjson]` is present; the cost -is a documented, opt-in performance change (stdlib `json`, roughly 2-5x slower, same correctness, -and non-JSON-native values in log `extra` render via `repr` rather than orjson's native encoding). - -**Revisit trigger:** `orjson` ships free-threaded wheels (#530 resolves). At that point it could -return to `logging`/core as a hard dependency and the fallback branch retire — reopen then to decide -whether that simplification is worth removing the opt-in extra. diff --git a/docs/adr/0006-otlp-http-exporter-shape.md b/docs/adr/0006-otlp-http-exporter-shape.md deleted file mode 100644 index fd62561..0000000 --- a/docs/adr/0006-otlp-http-exporter-shape.md +++ /dev/null @@ -1,33 +0,0 @@ -# OTLP over HTTP is a sibling extra, and HTTP carries no insecure warning - -**Decision:** `otl` points at `opentelemetry-exporter-otlp-proto-grpc`; a sibling `otl-http` carries -`opentelemetry-exporter-otlp-proto-http` (no `grpcio`). There are no framework `*-otl-http` variants. -The `__post_init__` insecure-endpoint warning stays tied to the gRPC `insecure` flag; for HTTP the -endpoint URL's scheme is the security signal, and it is documented rather than warned about. - -## Context - -`otl` used to pull `opentelemetry-exporter-otlp`, a meta package that drags in the gRPC exporter and -therefore `grpcio`. `grpcio` has no free-threaded wheels, so OTLP export on ft needs the HTTP -exporter to be installable on its own. - -## Rejected alternatives - -**Keep `otl` as the meta package and layer `otl-http` on top.** Leaves `otl` `grpcio`-bound and -redundant, since the meta package already ships the HTTP exporter. Repointing `otl` at the gRPC -exporter package is functionally identical for existing users — the exporter already defaulted to -gRPC and the HTTP package was never used — and drops only an unused transitive dependency. - -**Framework `*-otl-http` variants (`fastapi-otl-http`, `litestar-otl-http`, …).** The same -combinatorial sprawl rejected for `orjson` in ADR-0005. A free-threaded framework service composes -`[fastapi, otl-http]` and adds its own instrumentation package directly. - -**An `http://`-non-local warning mirroring the gRPC one.** The gRPC exporter has an `insecure` bool -that the config can inspect; the HTTP exporter has no such flag, only a full endpoint URL. Re-deriving -an "insecure" state would mean parsing `http://` vs `https://` in `__post_init__` to nudge the user -about a signal they already typed explicitly into the URL — more code for a weaker signal than the -URL itself gives. - -**Revisit trigger:** a user asks for a framework-specific free-threaded OTLP-HTTP convenience extra, -or for the HTTP endpoint to accept a bare `host:port` and auto-build the URL. Either reopens the -extras shape or the URL handling. diff --git a/docs/adr/0007-core-declares-typing-extensions.md b/docs/adr/0007-core-declares-typing-extensions.md index b917086..3429c66 100644 --- a/docs/adr/0007-core-declares-typing-extensions.md +++ b/docs/adr/0007-core-declares-typing-extensions.md @@ -1,37 +1,12 @@ -# Core declares `typing-extensions`; a genuinely zero-dependency core was tried and failed - -**Decision:** `lite-bootstrap`'s core declares exactly one runtime dependency, `typing-extensions`. -The alternative — removing the two runtime uses so a bare install has no dependencies at all — was -implemented, tested, and abandoned. - -`1.3.0` shipped claiming a zero-dependency core once `orjson` became opt-in (ADR-0005), but -`import lite_bootstrap` on a bare install raised `ModuleNotFoundError: No module named -'typing_extensions'`. Every install with any extra masked it, because every extra pulls +# Core declares `typing-extensions` + +A genuinely zero-dependency core was implemented, tested and abandoned. `HealthCheckTypedDict` is a +FastAPI response model, and pydantic refuses a stdlib `typing.TypedDict` model on Python < 3.12 +(`PydanticUserError`) — a constraint that lives in pydantic's behaviour on an old interpreter, not in +a `TypedDict` you can build in isolation, so the attempt passed locally on 3.12 and failed the CI +matrix on 3.10 and 3.11. `1.3.0` had already shipped claiming a zero-dependency core and raised +`ModuleNotFoundError` on a bare install, masked everywhere because every extra pulls `typing_extensions` transitively. -## Rejected alternative: remove the usage - -There are two runtime uses: `typing_extensions.Self` return annotations on `BaseConfig`'s -constructors, and `class HealthCheckTypedDict(typing_extensions.TypedDict, ...)` in the health-checks -instrument. The first is easy to drop (`from __future__ import annotations` plus `TYPE_CHECKING`). -The second is not, and that is what settles it: `HealthCheckTypedDict` is a FastAPI response model, -and pydantic refuses a stdlib `typing.TypedDict` model on Python < 3.12 — - -``` -PydanticUserError: Please use `typing_extensions.TypedDict` instead of -`typing.TypedDict` on Python < 3.12. -``` - -The attempt passed locally on 3.12 and against an isolated 3.10 `TypedDict` construction, and failed -the CI matrix on 3.10 and 3.11 — which is the shape of this whole class of mistake: the constraint -lives in pydantic's behaviour on an old interpreter, not in a `TypedDict` you can build in isolation. -So for as long as 3.10 and 3.11 are supported, core genuinely needs `typing_extensions` at runtime, -and declaring it is the honest fix. Dropping 3.10/3.11 to reclaim zero-dep was considered and is not -worth it on its own. - -`typing-extensions` is pure Python, so core stays free-threading-friendly; this is the leanest core -available rather than a compromise on the ft story. - -**Revisit trigger:** Python 3.11 goes out of support and the floor rises to 3.12, at which point -`typing.TypedDict` is acceptable to pydantic, `Self` is in `typing`, and the dependency can be -dropped for a genuinely zero-dependency core. +It is pure Python, so core stays free-threading-friendly, and the dependency can be dropped once the +floor reaches 3.12 — where `typing.TypedDict` is acceptable to pydantic and `Self` is in `typing`. diff --git a/docs/adr/0008-structlog-sentry-seam-stays-a-heuristic.md b/docs/adr/0008-structlog-sentry-seam-stays-a-heuristic.md index 97cf24f..91e8e41 100644 --- a/docs/adr/0008-structlog-sentry-seam-stays-a-heuristic.md +++ b/docs/adr/0008-structlog-sentry-seam-stays-a-heuristic.md @@ -1,49 +1,22 @@ -# The structlog→Sentry seam is a value object, not a marker key or a shared module - -**Decision:** `StructuredLogPayload` in `logging_factory.py` owns the parse of a rendered log line -and the meta-key vocabulary (`STRUCTLOG_META_KEYS`); `sentry_instrument.py` keeps only the -orchestration — drop on `skip_sentry`, lift `message`, attach `extra` under `contexts.structlog`. -The producer still emits a plain flat JSON object and the consumer still recognises one by -`startswith("{")`. - -The problem being solved was ownership, not the heuristic: the meta-key set was duplicated across -the producer's processor chain and the consumer's strip list and owned by neither, so renaming a -meta-key silently degraded Sentry enrichment with nothing failing. - -## Rejected alternatives - -**Stamp an explicit sentinel/marker key on every log line** instead of sniffing `startswith("{")`. -This pollutes the stdout JSON shape for every logging user in order to serve one consumer. The -heuristic is cheap and adequate, and the log shape is user-visible output. - -**A neutral third module both instruments import.** There is exactly one consumer. A -shared-for-sharing's-sake module abstracts a sharing that does not exist and pulls the vocabulary -away from the chain that generates it — the value object belongs next to the serializer that -produces what it parses. - -**A symmetric `serialize()` on the value object.** The producer never constructs a -`StructuredLogPayload`; it hands structlog's full `event_dict` to the existing serializer. A -`serialize()` nobody calls would be dead surface. - -**An absolute, compile-time drift fix.** Nesting user kwargs under a single `extra` key would make -drift impossible, and would change the emitted log shape for every user. The deliberate trade is -"unlikely and caught by a round-trip test" over "impossible and a breaking change": a custom -top-level meta-processor whose key is not added to `STRUCTLOG_META_KEYS` still leaks, and that is a -known, accepted limit. - -**Deferring the render to `ProcessorFormatter.wrap_for_formatter`.** This is structlog's own -idiomatic stdlib integration, it is already what `_configure_foreign_loggers` uses ten lines away, -and it was the obvious fix for #193's double-rendered traceback. It is incompatible with this seam. -`wrap_for_formatter` moves rendering to handler-flush time, so `record.msg` is the `EventDict` -rather than a rendered line; `sentry_sdk` builds `logentry.formatted` from `record.getMessage()`, -which for a dict `msg` is a Python repr. That repr opens with `{`, so `StructuredLogPayload.parse` -accepts it, fails to decode it, and returns `None` — `skip_sentry` stops being honoured and -`contexts.structlog` stops being attached, with nothing failing. The seam requires that the line -be rendered before it becomes a `LogRecord`; anything downstream of the chain is transport only. - -`IGNORED_STRUCTLOG_ATTRIBUTES` survives in `sentry_instrument.py` as a silent alias of -`STRUCTLOG_META_KEYS` for external importers of the old name. - -**Revisit trigger:** a second consumer honours `skip_sentry` or needs the parsed payload. At that -point the value object has a real audience, `skip_sentry` should be renamed to something reporter- -neutral, and a marker key stops being a cost paid for one consumer. +# The structlog→Sentry seam is a value object, not a marker key + +`StructuredLogPayload` in `logging_factory.py` owns both the parse of a rendered log line and the +meta-key vocabulary (`STRUCTLOG_META_KEYS`), leaving `sentry_instrument.py` only the orchestration. +The problem solved was ownership, not the heuristic: the meta-key set was duplicated across the +producer's processor chain and the consumer's strip list and owned by neither, so renaming a meta-key +silently degraded Sentry enrichment. The producer still emits flat JSON and the consumer still +recognises it by `startswith("{")` — an explicit marker key would pollute user-visible stdout for +every logging user to serve one consumer, and a neutral third module both instruments import would +abstract a sharing that does not exist. + +The trap worth knowing before "fixing" it: structlog's own idiomatic +`ProcessorFormatter.wrap_for_formatter` is incompatible with this seam. It defers rendering to +handler-flush time, so `record.msg` is an `EventDict`, `sentry_sdk` builds `logentry.formatted` from +`record.getMessage()`, and the resulting Python repr opens with `{` — which the parser accepts, fails +to decode, and returns `None` from, silently dropping both `skip_sentry` and `contexts.structlog`. +The seam requires the line to be rendered before it becomes a `LogRecord`. + +A known accepted limit: a custom top-level meta-processor whose key is not added to +`STRUCTLOG_META_KEYS` still leaks into the payload. Nesting user kwargs under one `extra` key would +make that impossible, at the cost of changing the emitted log shape for every user; a round-trip test +is the trade taken instead. diff --git a/docs/adr/0009-no-django-bootstrapper.md b/docs/adr/0009-no-django-bootstrapper.md index 25b3b28..e25c677 100644 --- a/docs/adr/0009-no-django-bootstrapper.md +++ b/docs/adr/0009-no-django-bootstrapper.md @@ -1,17 +1,9 @@ # No Django bootstrapper -**Decision:** `lite-bootstrap` will not ship a Django bootstrapper. Django's size makes it the -framework most likely to be proposed by someone who has not hit the contract below. - -Every bootstrapper keeps one contract: the user constructs the application, passes it in, and gets -the same object back. Django's observability is conventionally owned by `settings.py` — `MIDDLEWARE` -ordering, installed apps — which runs before any object a bootstrapper could be handed, so the -contract has no natural expression there. ADR-0001 is the calibration: FastMCP was the hardest -framework to fit and still had an application object to attach to. - -Rejected: **attach to a constructed `ASGIHandler` instead.** This is the shape that would fit, and -today it means putting middleware outside `MIDDLEWARE`, diverging from every Django deployment guide -and from what a Django user would debug against. - -**Revisit trigger:** a released, maintained path that attaches instrumentation to a constructed -`ASGIHandler` (or equivalent) without going through `settings.py`. +Every bootstrapper keeps one contract — the user constructs the application, passes it in, and gets +the same object back — and Django's observability is conventionally owned by `settings.py` +(`MIDDLEWARE` ordering, installed apps), which runs before any object a bootstrapper could be handed. +ADR-0001 is the calibration: FastMCP was the hardest framework to fit and still had an application +object to attach to. Attaching to a constructed `ASGIHandler` is the shape that would fit, and today +that means middleware outside `MIDDLEWARE`, diverging from every Django deployment guide and from +what a Django user would debug against. diff --git a/docs/adr/0010-otel-sampling-is-a-sampler-object.md b/docs/adr/0010-otel-sampling-is-a-sampler-object.md index ccf9b41..1ce2ef1 100644 --- a/docs/adr/0010-otel-sampling-is-a-sampler-object.md +++ b/docs/adr/0010-otel-sampling-is-a-sampler-object.md @@ -1,42 +1,15 @@ # OpenTelemetry sampling takes a `Sampler`, not a sample-rate float -**Decision:** `OpenTelemetryConfig.opentelemetry_sampler` takes an `opentelemetry.sdk.trace.sampling.Sampler` -and is handed to `TracerProvider(sampler=...)` unconditionally. Left `None` — the default — the SDK -picks its own sampler, which honours `OTEL_TRACES_SAMPLER` / `OTEL_TRACES_SAMPLER_ARG`, so the -default behaviour and the existing environment-variable path are both unchanged. - -## Context - -`OpenTelemetryInstrument.bootstrap()` built the provider without a sampler, so `parentbased_always_on` -applied unless the environment said otherwise. #184 measured always-on tracing at ~55 µs/request over -`ParentBased(TraceIdRatioBased(0.01))` on an endpoint whose bare cost is 16 µs, and the instrument -owns provider construction, so there was no way to express a sampler from Python. - -`TracerProvider(sampler=None)` is identical to omitting the argument: the SDK does -`if not sampler: sampler = sampling._get_from_env_or_default()`, at the declared 1.28 floor and at -1.44 alike. The pass-through therefore needs no conditional and no sentinel default. - -## Rejected alternatives - -**`opentelemetry_traces_sample_rate: float | None`, mapped internally to `ParentBased(TraceIdRatioBased(rate))`.** -Mirrors `sentry_traces_sample_rate` and keeps `opentelemetry.sdk` out of the config's annotations. Two -problems. The annotation argument does not hold: `opentelemetry_instrumentors` already carries -`BaseInstrumentor`, and `sentry_integrations` / `sentry_before_send` carry Sentry types, all as quoted -annotations over `TYPE_CHECKING` imports that never run. And a float closes only part of the gap: -`ALWAYS_OFF`, a bare `TraceIdRatioBased` without the `ParentBased` wrapper, and vendor samplers all -stay unreachable, and this config has no `opentelemetry_additional_params` escape hatch to reach them -through. A float can still be layered on the `Sampler` field later; the reverse ordering would have -shipped a second field to finish the job. - -**Documenting `OTEL_TRACES_SAMPLER` and adding nothing.** It is a real surface and it keeps working, -but it is the only one, and it cannot express a custom sampler at all. Every other instrument takes -its configuration from the config object; tracing's sample rate should not be the exception that lives -in the environment. - -**Validating the sampler in `__post_init__`.** Nothing to validate: `TraceIdRatioBased` already -rejects a rate outside [0.0, 1.0] at construction, which is where a float field would have needed a -range check of our own. - -**Revisit trigger:** a request to drive the sampler from a settings object (pydantic-settings and -friends map an env var to a float, not to a `Sampler`), which is when the float becomes sugar worth -adding alongside — with this field winning where both are set. +`opentelemetry_sampler` is handed straight to `TracerProvider(sampler=...)`. Left `None` — the +default — the SDK picks its own sampler, which honours `OTEL_TRACES_SAMPLER` / `OTEL_TRACES_SAMPLER_ARG`, +so both the default behaviour and the existing environment-variable path are unchanged; the +pass-through needs no conditional, because `TracerProvider(sampler=None)` is identical to omitting +the argument at the declared 1.28 floor and at 1.44 alike. + +A `float` sample rate mirroring `sentry_traces_sample_rate` was rejected. It buys no dependency +independence — `opentelemetry_instrumentors` already carries `BaseInstrumentor`, and +`sentry_integrations` / `sentry_before_send` carry Sentry types, all as quoted annotations over +`TYPE_CHECKING` imports that never run — and it closes only part of the gap, leaving `ALWAYS_OFF`, a +bare `TraceIdRatioBased` and vendor samplers unreachable behind an instrument that owns provider +construction and offers no `*_additional_params` escape hatch. A float can still be layered on top +later, which is what a settings object mapping one env var would want. From ce4fe42c8fde668e8c0c86b2ab24c2476f9c9af2 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 13:07:53 +0300 Subject: [PATCH 3/7] docs: renumber the ADRs to close the merge gaps The two merges retired 0004 and 0006, leaving holes. Closing them so the set runs 0001-0008 and the next ADR is 0009: extras 0005->0004, typing-extensions 0007->0005, structlog/Sentry seam 0008->0006, Django 0009->0007, sampler 0010->0008. Renumbering breaks references, so all nine in the repo were audited, not just the one shape the citation test enforces. That test reads full `docs/adr/NNNN-slug.md` paths in Python and there is exactly one (opentelemetry_instrument.py); the other eight are bare `ADR-NNNN` numbers in comments and prose, invisible to it. Four needed rewriting: logging_factory, test_core_import_surface, floor_smoke and the sampler comment's number alongside its path. pyproject's 0001 path, base.py's 0003, the ADR-0001 cross-reference in the Django ADR and domain.md's 0002 example all kept their numbers; domain.md's gloss was updated to the merged title. `docs/agents/domain.md` now records the format and the numbering rule, so the next ADR does not reintroduce the old scaffolding or reuse a retired number. --- ...pt-in.md => 0004-native-dependency-extras-are-opt-in.md} | 0 ...xtensions.md => 0005-core-declares-typing-extensions.md} | 0 ...c.md => 0006-structlog-sentry-seam-stays-a-heuristic.md} | 0 ...jango-bootstrapper.md => 0007-no-django-bootstrapper.md} | 0 ...-object.md => 0008-otel-sampling-is-a-sampler-object.md} | 0 docs/agents/domain.md | 6 ++++-- lite_bootstrap/instruments/logging_factory.py | 2 +- lite_bootstrap/instruments/opentelemetry_instrument.py | 4 ++-- scripts/floor_smoke.py | 2 +- tests/test_core_import_surface.py | 2 +- 10 files changed, 9 insertions(+), 7 deletions(-) rename docs/adr/{0005-native-dependency-extras-are-opt-in.md => 0004-native-dependency-extras-are-opt-in.md} (100%) rename docs/adr/{0007-core-declares-typing-extensions.md => 0005-core-declares-typing-extensions.md} (100%) rename docs/adr/{0008-structlog-sentry-seam-stays-a-heuristic.md => 0006-structlog-sentry-seam-stays-a-heuristic.md} (100%) rename docs/adr/{0009-no-django-bootstrapper.md => 0007-no-django-bootstrapper.md} (100%) rename docs/adr/{0010-otel-sampling-is-a-sampler-object.md => 0008-otel-sampling-is-a-sampler-object.md} (100%) diff --git a/docs/adr/0005-native-dependency-extras-are-opt-in.md b/docs/adr/0004-native-dependency-extras-are-opt-in.md similarity index 100% rename from docs/adr/0005-native-dependency-extras-are-opt-in.md rename to docs/adr/0004-native-dependency-extras-are-opt-in.md diff --git a/docs/adr/0007-core-declares-typing-extensions.md b/docs/adr/0005-core-declares-typing-extensions.md similarity index 100% rename from docs/adr/0007-core-declares-typing-extensions.md rename to docs/adr/0005-core-declares-typing-extensions.md diff --git a/docs/adr/0008-structlog-sentry-seam-stays-a-heuristic.md b/docs/adr/0006-structlog-sentry-seam-stays-a-heuristic.md similarity index 100% rename from docs/adr/0008-structlog-sentry-seam-stays-a-heuristic.md rename to docs/adr/0006-structlog-sentry-seam-stays-a-heuristic.md diff --git a/docs/adr/0009-no-django-bootstrapper.md b/docs/adr/0007-no-django-bootstrapper.md similarity index 100% rename from docs/adr/0009-no-django-bootstrapper.md rename to docs/adr/0007-no-django-bootstrapper.md diff --git a/docs/adr/0010-otel-sampling-is-a-sampler-object.md b/docs/adr/0008-otel-sampling-is-a-sampler-object.md similarity index 100% rename from docs/adr/0010-otel-sampling-is-a-sampler-object.md rename to docs/adr/0008-otel-sampling-is-a-sampler-object.md diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 77cb857..6de2232 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -25,7 +25,9 @@ either, the answer is that this repo does not have them. split and the two kinds of **skip** are defined there and are load-bearing throughout. - **`docs/adr/`**: read the ADRs that touch the area you are about to work in. They are an internal decision record, excluded from the published docs site, so an ADR is written for contributors - rather than users. + rather than users. They follow the `/domain-modeling` format — a title and a short body, with + sections only where they earn their place, and no `Decision:` / `Context` / `Revisit trigger` + scaffolding. A new one takes the next free number; retired numbers are never reused. ## Use the glossary's vocabulary @@ -37,4 +39,4 @@ If the concept you need isn't in the glossary yet, that's a signal: either you'r If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: -> _Contradicts ADR-0002 (keep the per-instrument axis), but worth reopening because…_ +> _Contradicts ADR-0002 (cross-cutting logic stays put), but worth reopening because…_ diff --git a/lite_bootstrap/instruments/logging_factory.py b/lite_bootstrap/instruments/logging_factory.py index b6c6e0e..ba293d6 100644 --- a/lite_bootstrap/instruments/logging_factory.py +++ b/lite_bootstrap/instruments/logging_factory.py @@ -46,7 +46,7 @@ def _dumps_stdlib(value: typing.Any, **kwargs: typing.Any) -> str: # noqa: ANN4 # orjson has no free-threaded wheels and refuses to build on ft; fall back to the -# stdlib json accelerator (always ft-native) when it is absent. See ADR-0005. +# stdlib json accelerator (always ft-native) when it is absent. See ADR-0004. _serialize_log_to_string = _dumps_orjson if import_checker.is_orjson_installed else _dumps_stdlib _json_loads = orjson.loads if import_checker.is_orjson_installed else json.loads diff --git a/lite_bootstrap/instruments/opentelemetry_instrument.py b/lite_bootstrap/instruments/opentelemetry_instrument.py index edc03b7..1f709c6 100644 --- a/lite_bootstrap/instruments/opentelemetry_instrument.py +++ b/lite_bootstrap/instruments/opentelemetry_instrument.py @@ -70,8 +70,8 @@ class OpenTelemetryConfig(OpenTelemetryServiceFieldsConfig): default_factory=list ) opentelemetry_log_traces: bool = False - # Left None, the SDK picks its own default, which reads OTEL_TRACES_SAMPLER; see ADR-0010 - # (docs/adr/0010-otel-sampling-is-a-sampler-object.md). + # Left None, the SDK picks its own default, which reads OTEL_TRACES_SAMPLER; see ADR-0008 + # (docs/adr/0008-otel-sampling-is-a-sampler-object.md). opentelemetry_sampler: "Sampler | None" = None opentelemetry_generate_health_check_spans: bool = True opentelemetry_excluded_urls: list[str] = dataclasses.field(default_factory=list) diff --git a/scripts/floor_smoke.py b/scripts/floor_smoke.py index 717fe68..b6f81de 100644 --- a/scripts/floor_smoke.py +++ b/scripts/floor_smoke.py @@ -132,7 +132,7 @@ def _fastapi() -> None: ) application = bootstrapper.bootstrap() # Generating the schema puts HealthCheckTypedDict through pydantic as a response model, - # which is the typing-extensions use ADR-0007 records. + # which is the typing-extensions use ADR-0005 records. assert application.openapi()["paths"] assert application.url_path_for("health_check_handler") == HEALTH_PATH _emit_span() diff --git a/tests/test_core_import_surface.py b/tests/test_core_import_surface.py index 183dbe6..f16b828 100644 --- a/tests/test_core_import_surface.py +++ b/tests/test_core_import_surface.py @@ -36,7 +36,7 @@ def test_importing_lite_bootstrap_needs_only_the_stdlib_and_typing_extensions() The rule is also what makes the free-threaded story work: nothing native is reachable from a bare import, so core installs and imports on any interpreter and each unavailable extra degrades to a skipped instrument rather than an ImportError. Adding a genuinely mandatory dependency is - allowed — declare it in `[project.dependencies]` and add it here; ADR-0007 records why + allowed — declare it in `[project.dependencies]` and add it here; ADR-0005 records why `typing-extensions` is the only one. """ offenders = { From b921cf0addac60706bc3c6b89d66476fd373ec2d Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 13:11:45 +0300 Subject: [PATCH 4/7] test: check bare `ADR-NNNN` citations, and citations outside Python MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The renumber exposed the gap: the invariant read full `docs/adr/NNNN-slug.md` paths in Python, and the repo has exactly one of those. The other eight citations are bare numbers in comments and prose, plus a path in `pyproject.toml`'s fastmcp floor comment — none of them checked by anything. `scripts/floor_smoke.py` was left pointing at the wrong ADR and only a manual grep caught it. So the scanner now reads `.py`, `.md` and `.toml`, and resolves both forms: a path must exist, a bare number must match some `docs/adr/NNNN-*.md`. The AST pass stays Python-only, since it exists for citations split across adjacent literals. `site/` is skipped as mkdocs output, a second copy of docs/ whose citations are the originals'. Verified by breaking each form in turn: a dangling number in test_core_import_surface and a dangling path in pyproject both fail the invariant, and both pass again once restored. The limit is in the docstring — a number renumbered onto a different live ADR still resolves, and nothing here can know it now names the wrong decision. --- tests/test_adr_citations.py | 81 ++++++++++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 19 deletions(-) diff --git a/tests/test_adr_citations.py b/tests/test_adr_citations.py index 760f6f1..292686b 100644 --- a/tests/test_adr_citations.py +++ b/tests/test_adr_citations.py @@ -8,43 +8,63 @@ _REPO_ROOT: typing.Final = pathlib.Path(__file__).resolve().parent.parent _ADR_DIR: typing.Final = "docs/adr/" _CITATION: typing.Final = re.compile(r"docs/adr/\d{4}-[a-z0-9-]+\.md") +_NUMBER_PREFIX: typing.Final = "ADR-" +_NUMBER_CITATION: typing.Final = re.compile(_NUMBER_PREFIX + r"\d{4}") _UNWALKED_DIR: typing.Final = "node_modules" +# mkdocs build output: a second copy of docs/, whose citations are the originals'. +_GENERATED_DIR: typing.Final = "site" +_SCANNED_SUFFIXES: typing.Final = (".py", ".md", ".toml") -def _python_files(root: pathlib.Path) -> list[pathlib.Path]: +def _scanned_files(root: pathlib.Path) -> list[pathlib.Path]: found: list[pathlib.Path] = [] for dirpath, dirnames, filenames in os.walk(root): - dirnames[:] = sorted(name for name in dirnames if not name.startswith(".") and name != _UNWALKED_DIR) - found.extend(pathlib.Path(dirpath, name) for name in sorted(filenames) if name.endswith(".py")) + dirnames[:] = sorted( + name for name in dirnames if not name.startswith(".") and name not in (_UNWALKED_DIR, _GENERATED_DIR) + ) + found.extend(pathlib.Path(dirpath, name) for name in sorted(filenames) if name.endswith(_SCANNED_SUFFIXES)) return found -def _citations(source: str) -> set[str]: +def _citations(file: pathlib.Path, source: str) -> set[str]: texts = [source] - texts.extend( - node.value - for node in ast.walk(ast.parse(source)) - if isinstance(node, ast.Constant) and isinstance(node.value, str) - ) - return {cited for text in texts for cited in _CITATION.findall(text)} + if file.suffix == ".py": + # Adjacent literals are joined at parse time, so a path split across them is one string + # in the AST and two fragments in the raw text. + texts.extend( + node.value + for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.Constant) and isinstance(node.value, str) + ) + return {cited for text in texts for pattern in (_CITATION, _NUMBER_CITATION) for cited in pattern.findall(text)} + + +def _resolves(root: pathlib.Path, cited: str) -> bool: + if cited.startswith(_ADR_DIR): + return (root / cited).is_file() + return any((root / _ADR_DIR).glob(f"{cited.removeprefix(_NUMBER_PREFIX)}-*.md")) def unresolved_citations(root: pathlib.Path) -> list[tuple[str, str]]: return sorted( (file.relative_to(root).as_posix(), cited) - for file in _python_files(root) - for cited in _citations(file.read_text(encoding="utf-8")) - if not (root / cited).is_file() + for file in _scanned_files(root) + for cited in _citations(file, file.read_text(encoding="utf-8")) + if not _resolves(root, cited) ) -def test_every_adr_path_cited_from_python_resolves() -> None: - """INVARIANT: a `docs/adr/NNNN-.md` path named anywhere in this repo's Python exists. +def test_every_adr_citation_in_the_repo_resolves() -> None: + """INVARIANT: every ADR named in this repo resolves, by full path or by bare `ADR-NNNN` number. + + Broken by renaming, renumbering or pruning an ADR without following its citations. The offline + link gate reads Markdown links only, so a path in a docstring, a comment, a guard message or a + `pyproject.toml` dependency rationale is otherwise checked by nothing, and neither is the bare + number form, which is how most of them are written. A user who trips a guard is handed a link + to follow. - Broken by renaming, renumbering or pruning an ADR without following its citations. The - offline link gate reads Markdown only, so a path in a docstring, a comment or a guard message - is otherwise checked by nothing, and an `INVARIANT:` docstring that names its ADR silently - loses the rationale the test depends on. A user who trips a guard is handed a link to follow. + The number form is checked for existence only: a citation renumbered onto a *different* live + ADR still resolves, and nothing here can know it now names the wrong decision. """ unresolved = unresolved_citations(_REPO_ROOT) @@ -81,6 +101,29 @@ def test_a_citation_inside_a_hash_comment_is_found(tmp_path: pathlib.Path) -> No assert unresolved_citations(tmp_path) == [("graph.py", f"{_ADR_DIR}0009-comment.md")] +def test_a_bare_adr_number_naming_no_file_is_reported(tmp_path: pathlib.Path) -> None: + """A renumber leaves `ADR-NNNN` prose behind; only the path form was ever checked.""" + (tmp_path / _ADR_DIR).mkdir(parents=True) + (tmp_path / _ADR_DIR / "0001-kept.md").write_text("# kept\n", encoding="utf-8") + (tmp_path / "smoke.py").write_text( + f"# the constraint {_NUMBER_PREFIX}0001 records, unlike {_NUMBER_PREFIX}9999\n", encoding="utf-8" + ) + + assert unresolved_citations(tmp_path) == [("smoke.py", f"{_NUMBER_PREFIX}9999")] + + +def test_a_citation_outside_python_is_found(tmp_path: pathlib.Path) -> None: + """`pyproject.toml` explains dependency floors by citing ADRs, and Markdown cites them in prose.""" + (tmp_path / _ADR_DIR).mkdir(parents=True) + (tmp_path / "pyproject.toml").write_text(f"# see {_ADR_DIR}0002-floor.md\ndeps = []\n", encoding="utf-8") + (tmp_path / "AGENTS.md").write_text(f"Read {_NUMBER_PREFIX}0004 before editing.\n", encoding="utf-8") + + assert unresolved_citations(tmp_path) == [ + ("AGENTS.md", f"{_NUMBER_PREFIX}0004"), + ("pyproject.toml", f"{_ADR_DIR}0002-floor.md"), + ] + + def test_a_tree_with_no_citations_and_no_adr_directory_reports_nothing(tmp_path: pathlib.Path) -> None: (tmp_path / "plain.py").write_text("X = 1\n", encoding="utf-8") From afc8a58b2bb011539e04ccc84168332b9ff3110c Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 13:16:24 +0300 Subject: [PATCH 5/7] docs: point at the skill's ADR format instead of restating it domain.md is the pointer file for the engineering skills; the format itself is the `/domain-modeling` skill's to define, and a paraphrase here is one more copy to drift. The numbering rule goes for the same reason - the skill already says to take the next free number, and with the gaps closed there are no retired numbers left to warn about. --- docs/agents/domain.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 6de2232..20e28e0 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -25,9 +25,7 @@ either, the answer is that this repo does not have them. split and the two kinds of **skip** are defined there and are load-bearing throughout. - **`docs/adr/`**: read the ADRs that touch the area you are about to work in. They are an internal decision record, excluded from the published docs site, so an ADR is written for contributors - rather than users. They follow the `/domain-modeling` format — a title and a short body, with - sections only where they earn their place, and no `Decision:` / `Context` / `Revisit trigger` - scaffolding. A new one takes the next free number; retired numbers are never reused. + rather than users. They follow the ADR format the `/domain-modeling` skill defines. ## Use the glossary's vocabulary From 841f1ca7cd34d3b1c12e8579c174f9f0cd6f65f6 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 13:19:19 +0300 Subject: [PATCH 6/7] docs: restore domain.md to the setup skill's template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file had drifted into repo prose — sections renamed and reordered, the `proceed silently` rule dropped, bullets rewritten to editorialise about CONTEXT.md's contents and the ADR format. All of that is the skills' to say. Back to the seed template verbatim, with only the branches this repo already answered resolved: single-context, so no CONTEXT-MAP bullet and no multi-context tree, and the trees and ADR example name real files. triage-labels.md and issue-tracker.md were checked against their templates and are byte-identical. --- docs/agents/domain.md | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 20e28e0..c5d8994 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -2,31 +2,26 @@ How the engineering skills should consume this repo's domain documentation when exploring the codebase. -## Layout +## Before exploring, read these + +- **`CONTEXT.md`** at the repo root. +- **`docs/adr/`**: read ADRs that touch the area you're about to work in. + +If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The `/domain-modeling` skill (reached via `/grill-with-docs` and `/improve-codebase-architecture`) creates them lazily when terms or decisions actually get resolved. + +## File structure -Single-context, and flat: +Single-context repo: ``` / ├── CONTEXT.md ├── docs/adr/ │ ├── 0001-fastmcp-teardown-via-provider-lifespan.md -│ └── ... +│ └── 0002-cross-cutting-logic-stays-put.md └── lite_bootstrap/ ``` -There is no `CONTEXT-MAP.md` and no per-package `CONTEXT.md` or `docs/adr/`. If you are looking for -either, the answer is that this repo does not have them. - -## Before exploring, read these - -- **`CONTEXT.md`** at the repo root. It owns the vocabulary, and `AGENTS.md` requires reading it - before naming a concept in code, a test name, or an issue title. The **configured** / **ready** - split and the two kinds of **skip** are defined there and are load-bearing throughout. -- **`docs/adr/`**: read the ADRs that touch the area you are about to work in. They are an internal - decision record, excluded from the published docs site, so an ADR is written for contributors - rather than users. They follow the ADR format the `/domain-modeling` skill defines. - ## Use the glossary's vocabulary When your output names a domain concept (in an issue title, a refactor proposal, a hypothesis, a test name), use the term as defined in `CONTEXT.md`. Don't drift to synonyms the glossary explicitly avoids. From 637677f4dab369187a22a8821b619b1d5f280d3a Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 13:20:29 +0300 Subject: [PATCH 7/7] docs: restore the AGENTS.md agent-skills block to its native shape `## Agent docs` with three bullets was a compressed variant of the block the setup skill writes. Back to `## Agent skills` with a sub-heading per artifact, carrying the same three answers. Nothing referenced the old heading. --- AGENTS.md | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8190a88..2e8a7db 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,11 +24,19 @@ Every link in `README.md` must be absolute: `https://github.com/modern-python/` for a directory. Never a relative path: `README.md` is also the PyPI long description, and PyPI does not rewrite relative links, so a relative one 404s on the package page. -## Agent docs +## Agent skills -- Issue tracker: GitHub issues on `modern-python/lite-bootstrap` via `gh`. `docs/agents/issue-tracker.md`. -- Triage labels: five canonical roles, each label string equal to its name. `docs/agents/triage-labels.md`. -- Domain docs: single-context, `CONTEXT.md` and `docs/adr/` at the repo root. `docs/agents/domain.md`. +### Issue tracker + +GitHub issues on `modern-python/lite-bootstrap`, via `gh`. See `docs/agents/issue-tracker.md`. + +### Triage labels + +The five canonical roles, each label string equal to its name. See `docs/agents/triage-labels.md`. + +### Domain docs + +Single-context: `CONTEXT.md` and `docs/adr/` at the repo root. See `docs/agents/domain.md`. ## Code style