Skip to content

Commit 7b32953

Browse files
authored
feat: make the OpenTelemetry sampler configurable, and compress the ADRs (#222)
* feat: make the OpenTelemetry sampler configurable `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. * 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. * 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. * test: check bare `ADR-NNNN` citations, and citations outside Python 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. * 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: restore domain.md to the setup skill's template 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: 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.
1 parent ffbd378 commit 7b32953

25 files changed

Lines changed: 328 additions & 367 deletions

‎AGENTS.md‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,19 @@ Every link in `README.md` must be absolute: `https://github.com/modern-python/<r
2424
or `.../tree/main/<path>` for a directory. Never a relative path: `README.md` is also the PyPI long
2525
description, and PyPI does not rewrite relative links, so a relative one 404s on the package page.
2626

27-
## Agent docs
27+
## Agent skills
2828

29-
- Issue tracker: GitHub issues on `modern-python/lite-bootstrap` via `gh`. `docs/agents/issue-tracker.md`.
30-
- Triage labels: five canonical roles, each label string equal to its name. `docs/agents/triage-labels.md`.
31-
- Domain docs: single-context, `CONTEXT.md` and `docs/adr/` at the repo root. `docs/agents/domain.md`.
29+
### Issue tracker
30+
31+
GitHub issues on `modern-python/lite-bootstrap`, via `gh`. See `docs/agents/issue-tracker.md`.
32+
33+
### Triage labels
34+
35+
The five canonical roles, each label string equal to its name. See `docs/agents/triage-labels.md`.
36+
37+
### Domain docs
38+
39+
Single-context: `CONTEXT.md` and `docs/adr/` at the repo root. See `docs/agents/domain.md`.
3240

3341
## Code style
3442

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,15 @@
11
# FastMCP teardown attaches through a Provider lifespan
22

3-
**Decision:** `FastMcpBootstrapper` wires its `teardown` into FastMCP's shutdown by registering a
4-
`_TeardownProvider` via the public `FastMCP.add_provider()`, whose `async def lifespan(self)` runs
5-
teardown on the exit branch. We will not reach into `FastMCP._lifespan`, rebuild the user's
6-
`FastMCP`, or leave teardown manual.
7-
8-
FastMCP is the one supported framework with no `on_shutdown`-shaped hook. `FastMCP.lifespan` is a
9-
bound method on the `AggregateProvider` mixin, not a settable attribute: assigning `app.lifespan`
10-
succeeds and has zero runtime effect, because the transport runners read the private `_lifespan`
11-
attribute that is only set at construction time. `add_provider()` is the sole public,
12-
post-construction hook whose callback is invoked by the server's ASGI lifespan.
13-
14-
Rejected, with the reasoning that would otherwise be re-litigated:
15-
16-
- **Mutate `app._lifespan` directly.** Works today, but it is private API on a fast-moving
17-
dependency, and a rename ships as a silent no-teardown rather than an error.
18-
- **Rebuild the user's `FastMCP` with a composed `lifespan=`.** Breaks the contract every other
19-
bootstrapper keeps — the user owns the application object they passed in, and gets the same
20-
object back.
21-
- **No automatic wiring; document a manual `teardown()` call.** Adopted briefly and reverted once
22-
`add_provider` was found. It makes FastMCP the only framework where shutdown is the user's job.
23-
24-
The accepted cost is semantic: `Provider` is FastMCP's general extension abstraction for tools,
25-
resources and prompts, and using one purely for a shutdown callback is thin. A one-line comment at
26-
the registration site says so.
27-
28-
**Revisit trigger:** FastMCP grows a first-class shutdown hook (an `on_shutdown` API, or a
29-
documented public way to compose a lifespan post-construction). At that point the provider is the
30-
indirect route and should be replaced by the direct one.
3+
FastMCP is the one supported framework with no `on_shutdown`-shaped hook: `FastMCP.lifespan` is a
4+
bound method on the `AggregateProvider` mixin, so assigning to it succeeds and does nothing — the
5+
transport runners read the private `_lifespan`, set only at construction — which leaves
6+
`add_provider()` as the single public post-construction hook the server's ASGI lifespan actually
7+
invokes. `FastMcpBootstrapper` therefore registers a `_TeardownProvider` whose `lifespan` runs
8+
teardown on the exit branch, accepting that `Provider` is FastMCP's extension abstraction for tools,
9+
resources and prompts and that using one for a shutdown callback is thin.
10+
11+
Rejected: mutating `app._lifespan` (private API on a fast-moving dependency, where a rename ships as
12+
silent no-teardown rather than an error); rebuilding the user's `FastMCP` with a composed `lifespan=`
13+
(every other bootstrapper hands back the object it was given); and documenting a manual `teardown()`
14+
call (adopted briefly, reverted once `add_provider` was found). If FastMCP grows a real shutdown
15+
hook, the provider becomes the indirect route and should be replaced.
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
# Cross-cutting logic stays where it is: per-instrument, and in one method
2+
3+
Two re-organizations have been proposed and rejected for the same reason — each moves complexity
4+
rather than concentrating it, and the matrix is inherently O(instruments × frameworks) whichever axis
5+
it is cut along.
6+
7+
The instrument × framework matrix keeps its **per-instrument axis**: base instruments own the hoisted
8+
logic and each framework is a thin `bootstrap()` subclass, rather than per-framework adapters driving
9+
generic instruments. The bindings genuinely differ — FastAPI imperative (`add_middleware`,
10+
`include_router`), Litestar declarative before the app is built, FastStream attaching to a broker,
11+
FastMCP via `custom_route` — so a uniform `add_route`/`add_middleware` interface would leak; and
12+
framework-locality already exists, because all of a framework's subclasses live in its one
13+
bootstrapper file. Reconsider if the per-cell bindings start converging, or if a new framework shares
14+
an existing one's attach mechanism exactly.
15+
16+
`_build_excluded_urls` likewise keeps the whole trace-exclusion policy in **one method** that reads
17+
its siblings' paths through `getattr(..., None)` — those sibling configs are genuinely optional, so
18+
the defensive read is correct rather than a smell. A per-instrument contribution mechanism would
19+
spread the policy across `PrometheusConfig`, `HealthChecksConfig` and OpenTelemetry while still
20+
needing `opentelemetry_generate_health_check_spans`, so the coupling would not even disappear. The
21+
real risk — renaming `prometheus_metrics_path` and silently stopping the exclusion — is answered by a
22+
test pinning each sibling path in the built set. Reconsider when a user-supplied instrument needs
23+
paths the policy cannot know about.

‎docs/adr/0002-keep-per-instrument-axis.md‎

Lines changed: 0 additions & 49 deletions
This file was deleted.
Lines changed: 18 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,20 @@
11
# The teardown-attach guard is an attribute marker, and its two limits are accepted
22

3-
**Decision:** `BaseBootstrapper._attach_teardown_once` detects a second bootstrapper by tagging the
4-
attach target with the `_lite_bootstrap_teardown_attached` attribute (#130). Two consequences of
5-
that choice — FastMCP detecting via the attribute rather than a provider-list scan, and Litestar
6-
tagging the shared `AppConfig` rather than a built app — are accepted rather than designed around.
7-
8-
## Why an attribute on the target
9-
10-
Rejected: **a class-level registry or `WeakSet` of already-attached applications.** It would
11-
contradict the `_lite_bootstrap_*` app-tagging convention the codebase already follows, and it
12-
introduces process-global mutable state with the test-isolation hazards that come with it. The
13-
marker keeps the "attached" bit local to the application's own lifetime, which is exactly the
14-
lifetime the fact is true for.
15-
16-
Rejected: **a free-function helper module.** The guard is bootstrapper-lifecycle logic and belongs on
17-
`BaseBootstrapper` next to `teardown()`, where `type(self).__name__` is available for the warning.
18-
19-
## The two accepted limits
20-
21-
1. **FastMCP detects via the marker, not structurally.** FastMCP previously detected double-attach
22-
by scanning `any(isinstance(p, _TeardownProvider) for p in app.providers)`, which reads the actual
23-
attach state. If the providers list is cleared after the first bootstrap while the marker
24-
survives, the second bootstrapper is refused rather than re-attaching. Accepted: uniformity across
25-
all four app-bearing bootstrappers is worth more than the sliver of state-accuracy the scan gave,
26-
the only scenario the guard exists for is detected identically either way, and the regression
27-
requires user or framework code to mutate `app.providers` after bootstrap, which no supported flow
28-
does. Keeping FastMCP on a bespoke structural check would re-fragment detection and defeat the
29-
seam's whole point.
30-
31-
2. **Litestar tags the `AppConfig`.** The Litestar app does not exist at `__init__` time — it is
32-
built later by `Litestar.from_config()` — so the attach, and therefore the marker, lands on
33-
`application_config`. Two `LitestarConfig` instances that *share* one `AppConfig` but intend two
34-
distinct apps will collide. Accepted: config-level is the only option while the app is built
35-
lazily, and sharing one mutable `AppConfig` across two intended apps is already broken
36-
independently of teardown — instrument bootstrap mutates the shared config's `cors_config`,
37-
`route_handlers` and `openapi_config`. The marker collision is one symptom of an
38-
already-unsupported pattern, not a new hazard.
39-
40-
The genuinely actionable finding from the same review — the marker being set before a fallible
41-
`attach()` — was *fixed*, not accepted: the target is tagged only after `attach()` returns.
42-
43-
Since #167 the consequence of hitting the marker is louder in both cases: the losing bootstrapper's
44-
`bootstrap()` raises `ConfigurationError` before any instrument is applied, rather than warning and
45-
leaving a half-wired application whose teardown never runs. The marker and these two limits are
46-
unchanged.
47-
48-
**Revisit trigger:** for FastMCP, a supported flow starts mutating `FastMCP.providers` after
49-
bootstrap (a documented hot-reload or provider-swap API), making the attribute diverge from real
50-
attach state — then move back to structural detection or reconcile the two. For Litestar, sharing one
51-
`AppConfig` across multiple apps becomes a supported pattern, or the attach is restructured to run at
52-
`bootstrap()` time when the app exists — then tag the built `Litestar` app instead.
3+
`BaseBootstrapper._attach_teardown_once` detects a second bootstrapper by tagging the attach target
4+
with `_lite_bootstrap_teardown_attached`, so the "attached" bit lives exactly as long as the
5+
application it describes — rather than in a class-level registry or `WeakSet`, which would mean
6+
process-global mutable state and would contradict the `_lite_bootstrap_*` tagging convention the
7+
codebase already follows. The target is tagged only after `attach()` returns, and since #167 hitting
8+
the marker raises `ConfigurationError` from `bootstrap()` rather than warning and leaving a half-wired
9+
application.
10+
11+
Two consequences are accepted rather than designed around:
12+
13+
- **FastMCP detects via the marker, not by scanning `app.providers`.** If that list is cleared after
14+
the first bootstrap, the second bootstrapper is refused instead of re-attaching. No supported flow
15+
mutates it, and uniformity across the four app-bearing bootstrappers is worth more than the sliver
16+
of state-accuracy the structural check gave.
17+
- **Litestar tags the shared `AppConfig`**, because the app does not exist until
18+
`Litestar.from_config()`. Two configs sharing one `AppConfig` collide — but that pattern is already
19+
broken independently, since instrument bootstrap mutates the shared config's `cors_config`,
20+
`route_handlers` and `openapi_config`.

‎docs/adr/0004-excluded-urls-stay-one-method.md‎

Lines changed: 0 additions & 25 deletions
This file was deleted.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Native-dependency extras are opt-in, and never per-framework variants
2+
3+
Free-threaded CPython has wheels for neither `orjson` nor `grpcio`, and PEP 508 has no environment
4+
marker for "GIL enabled", so neither can be required conditionally. Both are therefore opt-in:
5+
`orjson` is its own extra (`logging` is `structlog` only) with the logging serializer falling back to
6+
the stdlib `json` accelerator, and OTLP over HTTP is a sibling `otl-http` extra beside a gRPC-only
7+
`otl`. The fallback is a documented performance change, not a correctness one — roughly 2-5x slower,
8+
with non-JSON-native values in log `extra` rendering via `repr`; the GIL fast path is byte-for-byte
9+
unchanged when `[orjson]` is installed.
10+
11+
Both refuse the combinatorial alternative: `*-ft` twins of every logging-bearing and `*-all` extra,
12+
or `fastapi-otl-http`-style framework variants. A free-threaded service composes `[fastapi, otl-http]`
13+
itself. Replacing `orjson` with msgspec, ujson or rapidjson was also rejected — a permanent mandatory
14+
dependency, with a different encoder API and output shape, to paper over a temporary gap
15+
([ijl/orjson#530](https://github.com/ijl/orjson/issues/530) tracks ft wheels); if that resolves,
16+
`orjson` could return to core and the fallback branch retire.
17+
18+
The HTTP exporter deliberately carries no insecure-endpoint warning mirroring the gRPC one: it has no
19+
`insecure` flag to inspect, only a full URL whose scheme is a stronger signal than anything
20+
`__post_init__` could re-derive, and the user typed it explicitly.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
# Core declares `typing-extensions`
2+
3+
A genuinely zero-dependency core was implemented, tested and abandoned. `HealthCheckTypedDict` is a
4+
FastAPI response model, and pydantic refuses a stdlib `typing.TypedDict` model on Python < 3.12
5+
(`PydanticUserError`) — a constraint that lives in pydantic's behaviour on an old interpreter, not in
6+
a `TypedDict` you can build in isolation, so the attempt passed locally on 3.12 and failed the CI
7+
matrix on 3.10 and 3.11. `1.3.0` had already shipped claiming a zero-dependency core and raised
8+
`ModuleNotFoundError` on a bare install, masked everywhere because every extra pulls
9+
`typing_extensions` transitively.
10+
11+
It is pure Python, so core stays free-threading-friendly, and the dependency can be dropped once the
12+
floor reaches 3.12 — where `typing.TypedDict` is acceptable to pydantic and `Self` is in `typing`.

0 commit comments

Comments
 (0)