fix: trace FastStream without a broker telemetry middleware - #230
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 #226. Follow-up to #223 (merged as d6a7ad1), which made FastStream build a
TracerProviderbutleft this remainder of its headline claim, "all of these are accepted and silently do nothing".
FastStreamOpenTelemetryInstrumentgated itself on a second field:Both overrides are deleted. The middleware stays conditional on the guard already inside
bootstrap().That gate was sound before #223, when adding the broker middleware was the only thing the instrument
did: no middleware class meant genuinely nothing to do. Since #223 the instrument does three things
and still refused all three over the absence of one, so
FastStreamConfig(opentelemetry_endpoint=...)without a middleware class built no provider at all. Config-level skips are deliberately silent
(
base.py, "Config-level skip first: silent (no warning)"), so nothing was reported above INFO.Behaviour change
A FastStream service setting
opentelemetry_endpointoropentelemetry_log_traceswithoutopentelemetry_middleware_clspreviously got nothing. It now builds a provider, starts an exporterand claims the process-global
set_tracer_providerslot. That is what the config asked for, but it isa change in behaviour, not only a fix, which is why it is its own release rather than riding along
with #223.
What it buys, neither of which involves the broker:
FastStreamHealthChecksInstrumentwraps its handler intracer.start_as_current_span(...). That module-leveltraceris aProxyTracerresolving on spanentry, so with no provider installed the spans were
NonRecordingSpans. Verified by hand that theynow record:
"name": "GET /custom-health/"reaches the exporter withopentelemetry_middleware_cls=None.opentelemetry_instrumentors._apply_instrumentorslives in the basebootstrap()that neverran. An HTTP-client or SQLAlchemy instrumentor on a FastStream service has nothing to do with the
broker middleware, and was dropped.
Broker messages stay untraced without a middleware class. That is unavoidable, and the docs now say so
explicitly rather than implying the field gates all of OpenTelemetry.
Vocabulary
CONTEXT.mddefines an instrument as configured when the user's config asks for it. Anopentelemetry_endpointis the user asking. The deleted override made an asked-for instrumentdisappear into
skipped_instruments, which is the quiet skip "you have to go looking for". Thedeletion brings the code back in line with the definition.
Tests
The #223 instrumentor test is parametrized over
[RedisTelemetryMiddleware, None]rather than copied.without_middlewarefails onmain(assert len(instruments) == 1→0, the instrument is not eveninstantiated) and passes here;
with_middlewarepasses either way, since that is the already-fixed#223 regression.
RecordingInstrumentorstays defined inside the test function on purpose:BaseInstrumentoris asingleton per class, so a module-level one would find itself already instrumented on the second
parametrized run and silently skip.
Not tested, deliberately: an assertion that the health-check span records. It is the most
user-visible consequence, but the span resolves through the global provider, which is set-once and
owned by whichever test bootstraps first. An in-memory exporter attached to this instrument's provider
would see the span when the file runs alone and miss it in a full run. That is the same
order-dependence that got a test dropped from #228.
Type checking
Deleting this
is_configuredremoves one# ty: ignore[invalid-method-override].AGENTS.mdnamesthat suppression as recurring and correct, and it still is: two sites remain, in
litestar_bootstrapper.pyand inFastStreamPrometheusInstrumentjust below. NoAGENTS.mdeditneeded.
Out of scope
FastStreamPrometheusInstrumentkeeps the analogousprometheus_middleware_clsgate while itsbootstrap()mounts the metrics path unconditionally, so an injectedprometheus_collector_registrywith no middleware class yields no endpoint. The #226 fix does not transfer there, because
prometheus_metrics_pathdefaults to/metricsand deleting the override would change everyFastStream service's HTTP surface. Filed with options as #229.