fix: parse excluded_urls before handing them to the ASGI instrumentor - #249
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 #247.
Any Litestar service with OpenTelemetry enabled returns 500 on every request when
opentelemetry-instrumentationsits between the declared floor0.49b0and0.55b1:The middleware was handed a comma-joined string.
OpenTelemetryMiddlewareonly learned to parse oneitself in 0.56b0.
opentelemetry.util.http.parse_excluded_urlsexists at the floor and newer releasesaccept the
ExcludeListit returns, so parsing once covers the whole declared range and the floorstays at
>=0.49b0.Isolated to the OpenTelemetry floor rather than litestar's, and bisected: broken at 0.49b0, 0.50b0,
0.52b1 and 0.55b1; fixed from 0.56b0.
The test pins the type, deliberately
At current resolution both a string and an
ExcludeListwork, so a behavioural assertion would passon
mainand prove nothing. What is true at the floor and false before this change is that theinstrumentor receives something already parsed, so that is what the test asserts. It fails on
mainat current resolution, so CI guards the regression without needing the floors job.
A correction made during review
The first version of that test asserted
url_disabled("http://test/custom-health/")and passed. Itwas certifying a fiction: Litestar normalises the trailing slash out of
scope["path"], so themiddleware builds
http://host/custom-healthand never sees the URL the test used. Worse, thebehaviour it implied does not hold — a request to the excluded health path emits three spans.
That is a real, separate, pre-existing defect, now filed as #248 with the reproduction, the reason
FastAPI is unaffected, and the trap in the obvious fix (stripping the slash would start excluding
/custom-healthy, becauseExcludeListsearches unanchored). The test here now asserts only what isactually true, against a URL form the middleware really produces, and points at #248.
Not done here
#247 also suggests having
scripts/floor_smoke.pydrive one request per HTTP-serving target, which iswhat would have caught this. That belongs with #236 (floors never type-checked or unit-tested) and
#245 (the floors job never runs on pull requests) as one piece of work on what the floor job actually
exercises, rather than a quarter of it inside a three-line fix.
325 tests,
ruff,ty,mkdocs build --strictclean.