fix(componentized): honor USE_DDTRACE in the gateway and backend deployments - #35490
Conversation
|
|
Greptile SummaryThis PR enables Datadog bootstrap tracing for componentized gateway and backend deployments while retaining compatibility with images that predate the new entrypoint script.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the deployment overrides now honor tracing without requiring older or custom images to contain the newly added entrypoint script.
|
| Filename | Overview |
|---|---|
| docker/component_entrypoint.sh | Adds a transparent command wrapper that invokes ddtrace-run only when USE_DDTRACE=true and otherwise preserves the original command. |
| gateway/Dockerfile | Routes gateway startup through the shared wrapper while preserving worker expansion and CMD forwarding. |
| backend/Dockerfile | Routes backend startup through the shared wrapper without changing its single-process invocation. |
| terraform/litellm/aws/ecs.tf | Inlines tracing-aware launch commands in ECS overrides, avoiding a dependency on the new script in older or custom images. |
| terraform/litellm/gcp/cloudrun.tf | Adds the tracing-aware launch conditional as the final stage of the existing Cloud Run initialization chain. |
| tests/test_litellm/test_component_entrypoint.py | Exercises wrapper behavior and verifies that Docker and Terraform launch paths remain aligned. |
Reviews (4): Last reviewed commit: "fix(docker): honor USE_DDTRACE in the co..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a80e4f5 to
c00462c
Compare
|
@greptileai please re-review at c00462c Verified the entrypoint-override finding and fixed it. The Terraform modules did override the image ENTRYPOINT, so The ECS Tests extended in |
c00462c to
d27dc73
Compare
|
@greptileai please re-review at d27dc73 Verified this and fixed it. Checked the modules own defaults, Both modules now build their command from a Confirmed against real images with the command resolved out of Tests now resolve each launch command from the |
…nd images The componentized images exec uvicorn directly, so ddtrace-run never wraps the interpreter. USE_DDTRACE is not inert there; the proxy lifespan still runs patch_all and litellm's own manual spans still emit. What never gets installed is ddtrace's ASGI TraceMiddleware: starlette builds its middleware stack lazily on the first __call__, which is the lifespan scope, so patching from inside the lifespan body is already too late and no root request span is ever created. Route both entrypoints through a shared docker/component_entrypoint.sh that mirrors the monolith's prod_entrypoint.sh contract, including the DD_TRACE_OPENAI_ENABLED=False export that keeps ddtrace's openai integration from double-reporting calls litellm instruments itself.
d27dc73 to
203844d
Compare
|
@greptileai please re-review at 203844d Verified the compatibility concern and removed the file dependency entirely rather than guarding it. Checked the modules own defaults: Terraform no longer references Confirmed against the published default tag itself. That image contains no entrypoint script ( The decision now lives in two places, the script for image-default startup and the Terraform strings for override startup. Tests pin them to the same contract: each Terraform command is resolved from the |
TLDR
Problem this solves:
ddtrace-runHow it solves it:
USE_DDTRACE=trueDD_TRACE_OPENAI_ENABLED=Falselike the monolithRelevant issues
Fixes #34251
Linear ticket
Resolves LIT-4727
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
The end-user surface here is the container, so the proof runs the real images against a trace agent. The agent is a small HTTP server standing in for the Datadog agent; it msgpack-decodes the v0.5 payloads and prints the span names in each one, which is what makes the difference legible. No mocks inside the image; the gateway boots its real lifespan and serves real traffic
The headline is the span names. Before, the agent receives orphaned
fastapi.serialize_responsechildren and afastapi.requestcount of 0. After, that count is 6, one per served request. That is what a Datadog user actually sees: today the split deployment produces traces with no root span, so APM has no route, method, status or end-to-end latency to showBefore is commit
0a42f288and after is commit203844dfChild spans arrive, no root request span ever does. A trace with no root span carries no route, method, status or end-to-end latency, so the split deployment shows nothing usable in APM
Same commands against the fixed image:
Six root spans for six served requests, counting the readiness probe
ddtrace-runreaches every uvicorn worker, read straight out of the running container rather than inferred from the payload batching:The bootstrap directory is prepended to the image's own
PYTHONPATH=/apprather than replacing it, and it survives uvicorn's spawn into both workers. This is the same arrangement the monolith already ships, whereddtrace-run litellmfronts a multi-worker uvicornThe backend image, which has no worker knob:
With
USE_DDTRACEunset the fixed image is unchanged from today; the wrapper execs uvicorn with the same argv and touches no environment:The Terraform change was checked against real images rather than only in the unit tests, using the command string resolved straight out of
cloudrun.tf. The image under test isghcr.io/berriai/litellm-gateway:v1.86.0-dev, which is what both modules default to and which predates this PR, so it does not containdocker/component_entrypoint.shat all:That is the point of inlining. An image published long before this PR, with no entrypoint script anywhere in it, starts emitting root request spans the moment the module is applied. Had Terraform called the script instead, this same deployment would either have failed to start or, with an existence check in front of it, come up silently untraced
Type
🐛 Bug Fix
Changes
USE_DDTRACEwas not being wholly ignored in the componentized images, which is worth stating precisely because it explains why the symptom looks partial rather than absent.gateway/main.pyandbackend/main.pywrap proxy_server's lifespan, soProxyStartupEvent._init_dd_tracerstill runsddtrace.patch_all(logging=True, openai=False), andlitellm/litellm_core_utils/dd_tracing.pystill binds the real tracer at import time, so litellm's own manual spans emit normallyWhat never gets installed is ddtrace's ASGI
TraceMiddleware. The fastapi integration works by wrappingFastAPI.build_middleware_stack, and starlette builds that stack lazily on the first__call__, which is the lifespan scope. By the timepatch_allruns inside the lifespan body the stack already exists, and adding middleware after startup raises. The monolith does not hit this becauseddtrace-runinstalls itssitecustomizebootstrap at interpreter start, beforeimport fastapi. The same timing argument covers the httpx, redis and aiohttp references litellm binds at module importThe componentized images bypassed
docker/prod_entrypoint.shentirely and exec'd uvicorn directly, so nothing ever wrapped the interpreter. Both now route through a newdocker/component_entrypoint.shthat prefixes the command withddtrace-runwhen the flag is on and execs it untouched otherwise. It takes the whole command rather than an app target on purpose: the gateway honorsNUM_WORKERSand the backend deliberately does not, so leaving argv construction in each Dockerfile keeps that asymmetry intact instead of quietly granting the backend workers it never hadThe
DD_TRACE_OPENAI_ENABLED=Falseexport carries over fromdocker/prod_entrypoint.sh, and it matters more underddtrace-runthan it looks. The bootstrap patches the openai integration before any litellm code runs, so the in-processpatch_all(..., openai=False)cannot suppress it afterwards. Without the export every LLM call gets instrumented twice, once by ddtrace and once by litellm's own Datadog and LLM Observability loggers, and ddtrace additionally captures prompt and completion payloads that litellm's redaction settings are supposed to governThe wrapping stays behind the
USE_DDTRACEbranch rather than becoming unconditional, sinceddtrace-runalways starts the tracer and a writer thread that tries to reach an agent whether or not one existsFixing the images alone would have left this half done, because this repo's own Terraform modules override the image entrypoint and would have kept exec'ing uvicorn directly. Those overrides are already shell strings that spell out the uvicorn invocation, so they now spell out the tracing decision too:
Inlining rather than calling the script is deliberate. Both modules take a caller-supplied image tag and both default to
v1.86.0-dev, published long before this PR createsdocker/component_entrypoint.sh. Terraform therefore cannot reference that file: exec'ing a path the image does not contain fails at container start, and testing for it first would only downgrade those deployments to silently untraced, which is the exact failure this change exists to remove.ddtrace-runon the other hand is already present in every componentized image ever published, becauseddtraceships in theproxy-runtimeextra that both Dockerfiles have always installed. Writing the two-line decision inline means old images start tracing as soon as the module is applied, new images do the same, and nothing has to be released in a particular orderOn ECS the gateway and backend prepend an S3 config fetch to that command when
proxy_configis set; the gateway's other branch previously overrodeentryPointas an exec-form array, which cannot express a conditional, so it now uses the samesh -cshape as its sibling with the uvicorn args folded into the string. On Cloud Run both services join the command onto their existing startup fragments. The ECS backend has one branch that overrides nothing and inherits the imageENTRYPOINT, so it needed no change and got none. One comment went away with the branch it described, since it explained appending--workersthroughcommand, which that branch no longer doesThe decision now lives in two places,
docker/component_entrypoint.shfor image-default startup and the Terraform strings for override startup, and that duplication is the price of not depending on a file the caller's image may lack. The tests pin the two to the same contract rather than trusting them to stay in stepTests live in
tests/test_litellm/test_component_entrypoint.pyand run in the misc unit shard. They drive the script with stubddtrace-runanduvicornexecutables onPATHand assert which one got exec'd, that the openai integration was disabled on that branch only, and thatPYTHONPATHpasses through untouched soddtrace-runstill has something to prepend to. A parametrized case pins the componentized gating todocker/prod_entrypoint.shacrossunset, empty,false,True,TRUE,1andyes, so the two entrypoints cannot drift apart. The rest assert the wiring itself: both images invoke uvicorn through the entrypoint, both make it executable, the gateway keepsNUM_WORKERSand the backend stays single-processThe Terraform side gets the same treatment, and it is the only guard those files have, since no CI job lints or validates
terraform/litellm/. Rather than matching strings, the tests resolve each launch command out of the.tffile, expand its interpolations, and run it against the same stub binaries underUSE_DDTRACEunset,true,falseandTrue, asserting it reaches the same verdict asdocker/component_entrypoint.shrun under that same value: the same binary exec'd, and the openai integration disabled on the traced branch only. That is what keeps the two copies of the decision honest. Alongside it, every line naming a component ASGI target must honor the knob, the launch-site count is pinned so a deleted site cannot quietly shrink the scan, and the modules are asserted to contain no reference to the entrypoint script path at allReverting the Dockerfile wiring while leaving the script in place, which is exactly the shipped bug, fails four of these tests; removing only the
DD_TRACE_OPENAI_ENABLEDexport fails one; overwritingPYTHONPATHin the wrapper fails one. Collapsing either module's conditional to a bare uvicorn exec fails three tests in that module, dropping only itsDD_TRACE_OPENAI_ENABLEDexport fails two, and bypassing any one of the five individual launch sites fails that module's wiring assertionThe
PYTHONPATHcheck is worth a note, because its first version was worthless and mutation testing is what exposed that. It originally used/appas the fixture value, the same thing the images themselves set. Mutating the wrapper toexport PYTHONPATH="/app", which is precisely the bug the assertion exists to catch, left all tests green: the assertion was satisfied by the defect. The sentinel is now a value the wrapper could not plausibly invent, and the identical mutation failsOne thing this PR does not touch:
docker/build_from_pip/Dockerfile.build_from_pippins ddtrace yet has a bareENTRYPOINT ["litellm"], so it carries the same defect. Keeping the scope to the two componentized imagesFinal Attestation