fix(runtime-sdk): fix top-level import for opentelemetry package - #248
ryanking13 wants to merge 2 commits into
Conversation
ryanking13
changed the title
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
I'm Bonk, and I've done a quick review of your PR. PR #248 adds an entropy allowance for OpenTelemetry context imports.
|
|
@ask-bonk try again |
| @contextmanager | ||
| def opentelemetry_context(module): | ||
| # OpenTelemetry creates three UUID-backed context keys at import time. | ||
| with allow_bad_entropy_calls(3): |
There was a problem hiding this comment.
allow_bad_entropy_calls() requires the exact count to be consumed. opentelemetry-api 1.28.0 through 1.39.1 create only two context keys; the third key was added in 1.40.0, while MCP and FastMCP accept opentelemetry-api>=1.28.0. Those valid resolutions therefore raise 1 unexpected leftover getentropy calls here. Make the allowance version-compatible (or constrain the supported version) and cover both the minimum and current API versions.
There was a problem hiding this comment.
That sounds like a good catch by bonk. Perhaps we should just explicitly throw an error on older package versions?
Maybe we should actually do that for every package we add in here?
There was a problem hiding this comment.
Yeah, we should definitely add some version-based dynamic patches.
| @@ -0,0 +1,8 @@ | |||
| # ruff: noqa: F401 | |||
| import opentelemetry.context | |||
| import opentelemetry.trace | |||
There was a problem hiding this comment.
Adding opentelemetry.trace covers the previously reported span key, but MCP and FastMCP also import opentelemetry.propagate. Its default baggage propagator imports opentelemetry.baggage, where _BAGGAGE_KEY = create_key("baggage") runs after both new patch contexts have exited, so import mcp still performs an unallowed entropy call. Patch opentelemetry.baggage and exercise opentelemetry.propagate or the actual MCP entry point here.
| # "langchain-openai", | ||
| # "openai", | ||
| "litestar", | ||
| "opentelemetry-sdk", |
There was a problem hiding this comment.
This dependency only tests OpenTelemetry itself, so it misses another top-level call in the stated FastMCP consumer. FastMCP 4.0.3 executes _SUPPRESS_KEY = otel_context.create_key("fastmcp_suppress_telemetry") in fastmcp.telemetry after the OpenTelemetry imports complete. Even after covering baggage, from fastmcp import FastMCP remains blocked; add a patch and an import test for FastMCP, or use a general snapshot-safe create_key solution.
|
Posted 3 new inline findings. |
dom96
left a comment
dom96
left a comment
There was a problem hiding this comment.
Bonk comments look important. Accepting to unblock.
| @contextmanager | ||
| def opentelemetry_context(module): | ||
| # OpenTelemetry creates three UUID-backed context keys at import time. | ||
| with allow_bad_entropy_calls(3): |
There was a problem hiding this comment.
That sounds like a good catch by bonk. Perhaps we should just explicitly throw an error on older package versions?
Maybe we should actually do that for every package we add in here?
2 participants
opentelementry is used inside the official
mcpand fastMCP package, and hasos.urandom()calls during the initialization