feat: make the OpenTelemetry sampler configurable, and compress the ADRs - #222
Merged
Merged
Conversation
`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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #184.
opentelemetry_sampleronOpenTelemetryConfig, handed toTracerProvider(sampler=...). Threefollow-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 anopentelemetry.sdktype out of a config whose OTel extra is optional. That argument does not holdhere:
opentelemetry_instrumentorsalready carriesBaseInstrumentor, andsentry_integrations/sentry_before_sendcarry Sentry types, all as quoted annotations overTYPE_CHECKINGimports thatnever run.
What decided it is that a float closes only part of the gap. This instrument owns provider
construction and has no
opentelemetry_additional_paramsescape hatch, so after a float shipped,ALWAYS_OFF, a bareTraceIdRatioBasedwithout theParentBasedwrapper, and vendor samplerswould 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 doesif not sampler: sampler = sampling._get_from_env_or_default()— at the declared 1.28 floor and at1.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 theabsence 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 callingsuper().bootstrap()and never constructs a provider — it readsget_tracer_provider()and handsthat to the middleware.
opentelemetry_endpoint, the resource attributes andopentelemetry_log_tracesare equally inert there today. Filed as #223 with the two options, sincefixing 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-modelingformat — a titleand 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.mdpaths in Python — of which the repo has exactly one. The other eightcitations are bare
ADR-NNNNnumbers in comments and prose, plus a path inpyproject.toml; nonewere checked, and
scripts/floor_smoke.pywas left pointing at the wrong ADR until a manual grepfound it. The last commit closes that: the scanner reads
.py,.mdand.tomland resolves bothforms. Verified by breaking each form in turn.
Tests
Four against the instrument (sampler reaches the provider; unset keeps
DEFAULT_ON; unset honoursthe env vars; explicit sampler beats them) and one through
FreeBootstrapper, so the pass-through ispinned at the bootstrapper level too. Two more cover the widened citation scanner. 290 pass.