fix: build a TracerProvider on FastStream - #228
Merged
Merged
Conversation
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 #223.
FastStreamOpenTelemetryInstrument.bootstrap()overrode the base without callingsuper(), and it isthe only OpenTelemetry instrument in
FastStreamBootstrapper.instruments_types. So on FastStreamlite-bootstrap never constructed a
TracerProviderat all: the middleware was wired to whateverget_tracer_provider()returned, which in a process where nothing else installed one is aProxyTracerProviderhanding outNonRecordingSpans. Every OpenTelemetry field onFastStreamConfigwas accepted and did nothing.
One line fixes it:
Why option 1 of the two the issue offered
The issue left open whether the old behaviour was deliberate, a FastStream service consuming a provider
that something else owns. It cannot have been.
is_configuredisbool(opentelemetry_endpoint or opentelemetry_log_traces) and bool(opentelemetry_middleware_cls), sothe only way to turn FastStream tracing on at all was to set an endpoint the instrument then ignored.
The gate demanded an exporter config it proceeded to throw away. Documenting that (option 2) would
first have required changing
is_configuredto stop asking for the endpoint, a larger behaviour changethan just honouring it.
Why the middleware still reads
get_tracer_provider()Deliberately not
self._tracer_provider, matchingFastAPIOpenTelemetryInstrumentandLitestarOpenTelemetryInstrument. The SDK enforcesset_tracer_provideras set-once, so anapplication that installed its own provider before bootstrap still wins and the middleware still binds
to theirs. That is unchanged behaviour for exactly the users the issue worried about breaking. What
changes is the case the issue is about: nobody installed one, so lite-bootstrap's now exists.
This does mean a service that loses the set-once race is left with an orphaned provider carrying a live
exporter thread. That hazard is not new and not FastStream-specific, it is identical in FastAPI and
Litestar today, so this change makes FastStream share it rather than invent it. Filed separately as
#227.
Falls out for free:
_apply_instrumentorsnow runs, soopentelemetry_instrumentorsworks andteardown()'suninstrument()stops being unpaired;_silence_otel_loggers()now runs, so OTel's"Attempting to uninstrument while already uninstrumented" is suppressed as it is everywhere else. That
warning was visible on stderr before this change and is gone after.
No ADR
Against the three tests in the domain-modeling skill's
ADR-FORMAT.md, despite the issue text saying"either way it wants an ADR". That is right for option 2 and wrong for option 1. This change removes
a deviation: afterwards FastStream does what the other three bootstrappers do, so there is nothing
surprising left for a future reader to wonder about. Option 2 would have been a deliberate deviation
from the obvious path and would have needed one.
Tests
Two, both red on
mainand green with the one line (verified by stashing the source hunk andre-running, not only before writing it):
test_faststream_bootstrap_builds_its_own_tracer_provider— the instrument holds a real SDKTracerProvidercarryingservice.name. WasNone.test_faststream_bootstrap_applies_opentelemetry_instrumentors— an instrumentor inopentelemetry_instrumentorsis handed that provider. Was never called.Both assert on the instrument's own
_tracer_providerrather than the process global, followingtest_free_bootstrap_passes_sampler_to_tracer_provider, because the global is set-once and belongs towhichever test bootstraps first. Ran the FastStream file both first and last relative to the FastAPI
and Free files to confirm order independence.
A third test asserting the middleware receives
get_tracer_provider()was written and then dropped: itpassed identically with and without the fix, so it pinned nothing, and the obvious stronger assertion
(
get_tracer_provider() is instrument._tracer_provider) passes alone but fails once another filebootstraps first, which would have made the suite order-dependent.
Out of scope
is_configuredstill requiresopentelemetry_middleware_cls, so a config with only an endpoint isskipped entirely and stays inert, silently. That is the remainder of this issue's headline. FastStream skips the whole OpenTelemetry instrument when
opentelemetry_middleware_clsis unset #226._build_excluded_urls()has no FastStream call site, a broker middleware has no URL to exclude, yettwo existing tests exercise it. Untouched here.