Repository navigation
Fix: Bake the shim on the app's own OpenTelemetry version - #1311
Conversation
The shim installed one fixed contrib release (0.65b0), and each contrib release requires one exact OpenTelemetry core (0.65b0 requires opentelemetry-api==1.44.0). On an image that carries another core the install replaced it, and the `uv pip check` that follows failed the build on every package pinning the old one: weather_tool (core 1.43.0, one conflict), git_issue_agent (1.42.1, six), and any app built on google-adk or crewai. The install is now pinned to the opentelemetry-api the app already carries, and the nine packages are requested without a version, so the resolver lands on the one release paired with that core (1.42.1 -> 0.63b1, 1.43.0 -> 0.64b0, 1.45.0 -> 0.66b0). The api/sdk under the app do not move. An app that carries no OpenTelemetry, or a core older than 1.34.0, is pinned to OTEL_CONTRIB_VERSION: the same install as before, judged by the same check. The releases paired with older cores instrument only starlette<0.15, and before 0.52b0 they lack the initialize() the hook calls. When no release is paired with the app's core the build stops with a REFUSING line under uv's error. build-otel-shim.sh and the check are unchanged. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: YehoshuaSagron <ysagron@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe shim now selects its OpenTelemetry install pin based on the app’s ChangesOpenTelemetry shim dependency resolution
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reviewed change has no supported unresolved merge-blocking issue; normal checks remain appropriate. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The Dockerfile pins installs to the existing
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
The fix holds up. I checked the pairing claims against PyPI:
- each semconv release pins one exact
opentelemetry-api; - every core release from 1.34.0 through today's 1.45.1 has a contrib release at the matching version, for all nine packages;
- starlette's upper bound is dropped at 0.55b0,
initialize()first appears in 0.52b0, andwrapt<2holds through 0.61b0.
I also ran the resolve with --dry-run against cores 1.34.0, 1.43.0 and 1.45.1. Each landed on the matching contrib (0.55b0, 0.64b0, 0.66b1) and moved no api or sdk.
Nothing blocking. The one real gap is that the 1.34 floor is an assert, which PYTHONOPTIMIZE removes (inline, with a tested replacement). The other two comments are about the new refusal's wording and what the reader should do next.
| RUN pin="$("${VENV_PYTHON}" -c 'from importlib.metadata import version; v = version("opentelemetry-api"); \ | ||
| assert tuple(map(int, v.split(".")[:2])) >= (1, 34); print("opentelemetry-api==" + v)' 2>/dev/null)" \ |
There was a problem hiding this comment.
suggestion: the floor disappears when the image sets PYTHONOPTIMIZE. RUN inherits the base image's ENV, and with PYTHONOPTIMIZE set Python skips the assert, so a core from 1.31 to 1.33 is pinned to itself instead of falling back to OTEL_CONTRIB_VERSION.
I ran this RUN's shell (lines joined the way the Dockerfile joins them) against a venv holding opentelemetry-api==1.32.1:
- without the variable:
the bake is pinned to opentelemetry-distro==0.65b0 - with
PYTHONOPTIMIZE=1:the bake is pinned to opentelemetry-api==1.32.1, anduv pip install --dry-runresolvesopentelemetry-instrumentation-starlette==0.53b1. That is thestarlette<0.15release this floor exists to keep out.
Nothing after this catches it. With that set installed next to starlette 1.7.0:
uv pip checkpasses;verify_propagates's snippet passes, because 0.53b1 already hasinitialize();StarletteInstrumentor().is_instrumented_by_opentelemetryisFalse. The only sign is aDependencyConflict: requested: "starlette >= 0.13, <0.15" but found: "starlette 1.7.0"log line.
So a pure-Starlette app bakes and attests clean, then splits into separate traces at runtime. (verify_propagates also uses assert, so under the same env it would pass even for a core below 1.31, where initialize() is missing. That's older code outside this diff.)
The fix can't be an if: the Dockerfile joins the continued lines into one, and a compound statement can't follow ;. So the check has to stay an expression. I tested this with PYTHONOPTIMIZE unset, 1 and 2, on cores 1.32.1, 1.43.0 and none:
| RUN pin="$("${VENV_PYTHON}" -c 'from importlib.metadata import version; v = version("opentelemetry-api"); \ | |
| assert tuple(map(int, v.split(".")[:2])) >= (1, 34); print("opentelemetry-api==" + v)' 2>/dev/null)" \ | |
| RUN pin="$("${VENV_PYTHON}" -c 'from importlib.metadata import version; import sys; v = version("opentelemetry-api"); \ | |
| tuple(map(int, v.split(".")[:2])) >= (1, 34) or sys.exit(1); print("opentelemetry-api==" + v)' 2>/dev/null)" \ |
| Fail `REFUSING to bake … already instruments …` (exit 3): the app instruments itself — go to step 3 **without** `APP_CONTAINER`/`APP_IMAGE` (capture only). | ||
| Fail `REFUSING to bake … no runnable Python found` (exit 3): outside the shim's envelope (DESIGN "The envelope") — same, capture only; or pass the interpreter as arg 3 if you know it. | ||
| Fail `REFUSING to bake … is not present locally` (exit 3): wrong `IMAGE` — see Inputs; nothing was built. | ||
| Fail `REFUSING: the shim could not be installed alongside …` (the build fails): no contrib release is paired with the app's `opentelemetry-api` (a core newer than its contrib release, or a package index that lags) — uv's own error is above the line. |
There was a problem hiding this comment.
suggestion: every other Fail line here tells the reader what to do next, and this one only explains the cause. Maybe end it with a step, e.g. "— capture only (step 3 without APP_CONTAINER/APP_IMAGE) until the paired contrib release reaches the index, then re-bake."
This line can also be reached with no paired-release problem at all, on the fallback pin. A Python 3.9 app with no OpenTelemetry lands here because uv reports opentelemetry-distro==0.65b0 depends on Python>=3.10. One clause for that case would keep the reader from looking for a pairing that doesn't matter.
| opentelemetry-instrumentation-aiohttp-client \ | ||
| opentelemetry-instrumentation-urllib3 \ | ||
| opentelemetry-instrumentation-threading \ | ||
| || { echo "REFUSING: the shim could not be installed alongside ${pin} (uv's error is above; if it found no solution, no contrib release is paired with that version)" >&2; exit 1; } |
There was a problem hiding this comment.
nit: when pin is the fallback, this hint blames the wrong thing. Against a Python 3.9 venv with no OpenTelemetry:
╰─▶ Because the current Python version (3.9.23) does not satisfy
Python>=3.10 and opentelemetry-distro==0.65b0 depends on Python>=3.10, ...
REFUSING: the shim could not be installed alongside opentelemetry-distro==0.65b0 (uv's error is above; if it found no solution, no contrib release is paired with that version)
"That version" is opentelemetry-distro==0.65b0 there. Either drop the hint and leave the explanation in RECIPE, or print it only when pin starts with opentelemetry-api==.
A RUN inherits the base image's ENV, and PYTHONOPTIMIZE strips asserts. With it set, the 1.34 floor that #1311 added vanished: a core from 1.31 to 1.33 was pinned to itself, uv landed on a starlette instrumentor for starlette<0.15, and the bake and attestation passed with instrumentation off at runtime. The floor is now `… or sys.exit(1)`; the attestation's three asserts go the same way, since they run under the image's ENV too. The install refusal no longer blames a missing contrib pairing when the pin is the default distro, where no pairing is involved; the RECIPE row says what each case means and what to do next. Follow-up to #1311, from its review. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: YehoshuaSagron <ysagron@gmail.com>
3 participants
Fixes #1310.
The problem
The lineage attach kit installs a fixed OpenTelemetry contrib release (
0.65b0), which brings afixed core with it (1.44.0). The bake fails when the app image cannot take that core (#1310):
The change
The kit now keeps the core the app image already has, and installs the contrib release that
matches it. That covers both cases:
weather_tool: itsOTLP exporter stays with core 1.43.0);
google-adkandcrewaikeep core 1.42.1).It is one
RUNindeploy/lineage-attach/Dockerfile.otel-shim: the install is pinned to theimage's
opentelemetry-apiversion, and the shim packages are requested without a version. Eachcontrib release works with exactly one core, so the resolver has one possible answer (core 1.43.0
gets
0.64b0, core 1.42.1 gets0.63b1).Two cases still get the fixed
0.65b0, exactly as before:current Starlette.
build-otel-shim.shand theuv pip checkstep are unchanged. The docs are updated.Result
mainghcr.io/rossoctl/examples/weather_tool:latest0.64b0ghcr.io/rossoctl/examples/git_issue_agent:latest0.63b1google-adkandcrewai0.63b1ghcr.io/rossoctl/examples/slack_researcher:latest0.65b0mainTesting
Starlette app and a FastAPI app, each calling out through
httpxandrequests. The trace idis carried with
LINEAGE_PROPAGATE=1, and no header is sent without it.gave the same end-to-end lineage test results as before the change.
build-otel-shim.shwith docker as the container tool.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Documentation