Skip to content

feat: make the OpenTelemetry sampler configurable, and compress the ADRs - #222

Merged
lesnik512 merged 7 commits into
mainfrom
feat/otel-sampler
Sep 19, 2026
Merged

lesnik512 merged 7 commits into
mainfrom
feat/otel-sampler

Conversation

@lesnik512

@lesnik512 lesnik512 commented Sep 19, 2026

Copy link
Copy Markdown
Member

Closes #184.

opentelemetry_sampler on OpenTelemetryConfig, handed to TracerProvider(sampler=...). Three
follow-on commits reshape docs/adr/, which is where this PR's own rationale lands.

Sampler, not a sample-rate float

The issue leaned towards opentelemetry_traces_sample_rate: float | None, to keep an
opentelemetry.sdk type out of a config whose OTel extra is optional. That argument does not hold
here: 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.

What decided it is that a float closes only part of the gap. This instrument owns provider
construction and has no opentelemetry_additional_params escape hatch, so after a float shipped,
ALWAYS_OFF, a bare TraceIdRatioBased without the ParentBased wrapper, and vendor samplers
would all stay unreachable, and a second field would follow. A float can still be layered on top of
this one later; the reverse ordering could not. ADR-0008 records it.

Two premises in the issue that did not survive checking

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. No conditional, no sentinel default; the field is passed unconditionally.

Sampling was already configurable, through OTEL_TRACES_SAMPLER / OTEL_TRACES_SAMPLER_ARG.
Precisely because the old code omitted sampler, the SDK read the environment. The gap was the
absence of a Python surface, not the absence of any surface — which makes the env path something the
change has to preserve rather than a non-feature. Two tests pin it: unset still honours the env
vars, and an explicit sampler wins over them.

Both of those also mean the default is unchanged, as the issue asked.

Not covered: FastStream (#223)

The issue expects this to reach every bootstrapper "since they share OpenTelemetryInstrument".
FastAPI, Litestar and Free do. FastStream does not, for a pre-existing reason:
FastStreamOpenTelemetryInstrument.bootstrap() overrides the base without calling
super().bootstrap() and never constructs a provider — it reads get_tracer_provider() and hands
that to the middleware. opentelemetry_endpoint, the resource attributes and
opentelemetry_log_traces are equally inert there today. Filed as #223 with the two options, since
fixing it means lite-bootstrap starting to build and globally install a provider for services that
currently supply their own.

The ADR commits

docs/adr/ had grown a **Decision:** / ## Context / ## Rejected alternatives / **Revisit trigger:**
scaffolding, 3,421 words across ten files. They now follow the /domain-modeling format — a title
and a short body — at 1,423 words across eight, with revisit conditions folded into the prose.

Two merges, no drops. The exclusion-policy ADR joins the per-instrument-axis one: both record a
rejected re-organization refused for the same reason, that it moves complexity rather than
concentrating it. The OTLP-HTTP extra joins the orjson one: both are the same decision applied twice,
a native dependency with no free-threaded wheels becoming an opt-in extra rather than a
combinatorial matrix of variants. The set is then renumbered 0001-0008, so the sampler ADR above is
0008 rather than 0010.

Renumbering breaks citations, and the invariant that was supposed to catch that only read full
docs/adr/NNNN-slug.md paths in Python — of which the repo has exactly one. The other eight
citations are bare ADR-NNNN numbers in comments and prose, plus a path in pyproject.toml; none
were checked, and scripts/floor_smoke.py was left pointing at the wrong ADR until a manual grep
found it. The last commit closes that: the scanner reads .py, .md and .toml and resolves both
forms. Verified by breaking each form in turn.

Tests

Four against the instrument (sampler reaches the provider; unset keeps DEFAULT_ON; unset honours
the env vars; explicit sampler beats them) and one through FreeBootstrapper, so the pass-through is
pinned at the bootstrapper level too. Two more cover the widened citation scanner. 290 pass.

`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.
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.
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.
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.
@lesnik512 lesnik512 changed the title feat: make the OpenTelemetry sampler configurable feat: make the OpenTelemetry sampler configurable, and compress the ADRs Sep 19, 2026
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.
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.
`## 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.
@lesnik512
lesnik512 merged commit 7b32953 into main Sep 19, 2026
13 checks passed
@lesnik512
lesnik512 deleted the feat/otel-sampler branch September 19, 2026 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenTelemetry sampler is not configurable

1 participant