Skip to content

Commit c08e70b

Browse files
refactor(obs): drop speculative review additions, keep the measured fixes
The review rounds added ~920 lines on top of a PR whose intent is "wire sgp-obs". Much of that defends cases that cannot occur or that I could not verify, which is cost without benefit: more surface to review, more to keep true later. Removing it takes the review-round churn to ~620 lines. Removed: - **The SGP_OBS_GENAI_ENABLED gate.** Added last commit against a variable name I could not verify — it appears nowhere in this repo, and sgp-obs is not installable here to check. A gate on a name that may not exist is either a no-op or wrong, and no one has asked for the switch in anger yet. - **The second-app ASGI warning.** A diagnostic for a process that builds two ACP servers, or a worker and an ACP server together. The worker runs in its own process by design, so this warns about nothing that happens. - **Standing down when litellm's routing table is unreadable.** litellm is a hard dependency of this package; `litellm.constants` cannot go missing at runtime. The tri-state `bool | None` it required leaked through `_split_model` into `inference_call` to buy nothing. Back to a plain bool. - **The drift test that parsed litellm's main.py.** A test reading another package's source to re-derive our constant is more likely to break on an unrelated refactor than to catch a real change. Replaced with direct assertions on the eight pre-empted providers, which is the fact that matters. Kept, because each is a measured defect rather than a hypothetical: - the async span queue drain in the worker — dropped business spans - litellm provider routing — double-counted azure/groq/deepseek, lost bare anthropic models - UV_DEFAULT_INDEX in the scaffold templates — public PyPI fallback; this one also deletes more than it adds - the bounded sgp-obs flush — 20.04s to exit vs 0.31s, measured - the resolve-once import cache — 84us to 12us per call, measured Also trimmed the shutdown docstring's atexit note from thirteen lines to five; the limitation is worth recording, at that length. Tests: 81 across the two obs suites (was 97, the difference being coverage of the removed code), 1714 repo-wide. Lint and pyright clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent ba0c406 commit c08e70b

4 files changed

Lines changed: 27 additions & 321 deletions

File tree

‎src/agentex/lib/core/adapters/llm/_genai_metrics.py‎

Lines changed: 10 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -79,45 +79,20 @@
7979
_openai_client_providers: Any = _OPENAI_CLIENT_PROVIDERS_UNRESOLVED
8080

8181

82-
def _openai_client_provider_set() -> frozenset[str] | None:
83-
"""Providers litellm dispatches over the ``openai`` client, or None if unknowable.
82+
def _over_openai_client(provider: str) -> bool:
83+
"""Would the OpenAI client instrumentor already have recorded this call?
8484
85-
Imported from ``litellm.constants``, which is where the list is defined, rather
86-
than from the ``litellm`` top level, which is an incidental re-export: litellm
87-
declares no ``__all__``, so a type checker treats the top-level name as private
88-
and it carries no stability promise even informally.
85+
Read from ``litellm.constants``, where the list is defined; the ``litellm``
86+
top-level name is an incidental re-export (litellm declares no ``__all__``).
8987
"""
9088
global _openai_client_providers
9189
if _openai_client_providers is _OPENAI_CLIENT_PROVIDERS_UNRESOLVED:
92-
try:
93-
from litellm.constants import openai_compatible_providers
94-
95-
_openai_client_providers = (
96-
frozenset(openai_compatible_providers)
97-
- _PREEMPTED_BY_OWN_HTTP_HANDLER
98-
) | _EXTRA_OPENAI_CLIENT_PROVIDERS
99-
except Exception: # pragma: no cover - litellm is a hard dependency
100-
logger.warning(
101-
"litellm.constants.openai_compatible_providers is unavailable, so "
102-
"GenAI metrics cannot tell which calls the OpenAI client instrumentor "
103-
"already records. Recording anyway would double-count every "
104-
"openai-compatible provider, so litellm gateway metrics are off for "
105-
"this process."
106-
)
107-
_openai_client_providers = None
108-
return _openai_client_providers
109-
90+
from litellm.constants import openai_compatible_providers
11091

111-
def _over_openai_client(provider: str) -> bool | None:
112-
"""Would the OpenAI client instrumentor already have recorded this call?
113-
114-
None means "cannot tell", which is NOT the same as False and must not collapse
115-
into it: treating an unknown provider as native is what double-counts it.
116-
"""
117-
known = _openai_client_provider_set()
118-
if known is None:
119-
return None
120-
return provider in known
92+
_openai_client_providers = (
93+
frozenset(openai_compatible_providers) - _PREEMPTED_BY_OWN_HTTP_HANDLER
94+
) | _EXTRA_OPENAI_CLIENT_PROVIDERS
95+
return provider in _openai_client_providers
12196

12297

12398
_GENAI_UNRESOLVED = object()
@@ -148,7 +123,7 @@ def _genai() -> Any | None:
148123
return _genai_module
149124

150125

151-
def _split_model(model: str) -> tuple[str, bool | None]:
126+
def _split_model(model: str) -> tuple[str, bool]:
152127
"""``(provider, goes_out_over_the_openai_client)`` for a litellm model string.
153128
154129
``"litellm_proxy/anthropic/claude-sonnet-4"`` -> ``("anthropic", True)``
@@ -176,9 +151,6 @@ def _split_model(model: str) -> tuple[str, bool | None]:
176151
one thing the OpenAI client instrumentor cannot report, since from inside that
177152
client the call is simply "openai".
178153
179-
A second element of None means the routing table itself could not be read, so
180-
whether this call is already recorded elsewhere is unknown. Callers must stand down
181-
rather than guess; see :func:`inference_call`.
182154
"""
183155
proxied = model.startswith(_PROXY_PREFIX)
184156
rest = model[len(_PROXY_PREFIX):] if proxied else model
@@ -272,13 +244,6 @@ def inference_call(kwargs: dict[str, Any], args: tuple[Any, ...] = ()) -> Any:
272244
try:
273245
model = resolve_model(args, kwargs)
274246
vendor, over_openai_client = _split_model(model)
275-
if over_openai_client is None:
276-
# The routing table could not be read, so we cannot tell whether the
277-
# OpenAI client instrumentor is already recording this call. Recording
278-
# would double-count every openai-compatible provider, and a doubled
279-
# token or cost figure is worse than a missing one: the gap is visible
280-
# and warned about, the doubling is silent and gets believed.
281-
return _NULL_CALL
282247
return genai.call(
283248
provider=vendor,
284249
operation=genai.CHAT,

‎src/agentex/lib/core/adapters/llm/tests/test_genai_metrics.py‎

Lines changed: 9 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -123,27 +123,20 @@ def test_the_proxy_vendor_survives_resolution(self):
123123
True,
124124
)
125125

126-
def test_the_provider_list_agrees_with_litellm(self):
127-
"""Pinned to litellm's own list rather than a copy of it, because the copy
128-
would go stale every release."""
129-
import litellm.constants
130-
126+
def test_the_preempted_providers_are_not_treated_as_openai_client(self):
127+
"""These eight are in litellm's `openai_compatible_providers` but litellm
128+
serves them with its own HTTP client, so the OpenAI instrumentor never sees
129+
them and this gateway must record them."""
131130
from agentex.lib.core.adapters.llm._genai_metrics import (
132131
_PREEMPTED_BY_OWN_HTTP_HANDLER,
133132
_over_openai_client,
134133
)
135134

136-
compatible = list(litellm.constants.openai_compatible_providers)
137-
checked = 0
138-
for provider in compatible:
139-
if provider in _PREEMPTED_BY_OWN_HTTP_HANDLER:
140-
# In the list, but litellm serves it with its own HTTP client.
141-
assert not _over_openai_client(provider), provider
142-
else:
143-
assert _over_openai_client(provider), provider
144-
checked += 1
145-
assert checked > 20, "expected litellm to list many compatible providers"
146-
135+
for provider in _PREEMPTED_BY_OWN_HTTP_HANDLER:
136+
assert not _over_openai_client(provider), provider
137+
# ...while the rest of the list is genuinely over the openai client.
138+
for provider in ("together_ai", "deepinfra", "openai", "azure"):
139+
assert _over_openai_client(provider), provider
147140
# Natively routed: no openai client anywhere in the path.
148141
for provider in ("anthropic", "bedrock", "vertex_ai", "gemini"):
149142
assert not _over_openai_client(provider), provider
@@ -157,118 +150,6 @@ def test_groq_and_deepseek_are_recorded_here(self):
157150
provider, over_openai_client = _split_model(model)
158151
assert over_openai_client is False, f"{provider} would go unrecorded"
159152

160-
def test_the_preempted_set_still_matches_litellm(self):
161-
"""Drift guard. `openai_compatible_providers` describes API-format
162-
compatibility, not which Python client litellm uses, so we subtract the
163-
providers litellm intercepts earlier. That set is derived from litellm 1.87.0's
164-
dispatch chain; if an upgrade moves a provider between branches this fails,
165-
rather than silently double-counting it or silently losing it.
166-
167-
Re-derives from the installed litellm's source. If the shape of main.py changes
168-
enough that the parse yields nothing, skip rather than fail: an unparseable
169-
source is not evidence that the set is wrong.
170-
"""
171-
import re
172-
173-
import litellm.main
174-
from litellm.constants import openai_compatible_providers
175-
176-
from agentex.lib.core.adapters.llm._genai_metrics import (
177-
_PREEMPTED_BY_OWN_HTTP_HANDLER,
178-
)
179-
180-
with open(litellm.main.__file__) as handle:
181-
src = handle.read().splitlines()
182-
hits = [i for i, line in enumerate(src) if "openai_chat_completions.completion(" in line]
183-
if not hits:
184-
pytest.skip("litellm/main.py no longer dispatches the way this parse assumes")
185-
guard = max(
186-
i for i, line in enumerate(src[: hits[0]]) if re.match(r"^ (elif|if) ", line)
187-
)
188-
189-
earlier: set[str] = set()
190-
for line in src[:guard]:
191-
if re.match(r"^ (elif|if) ", line) or re.match(
192-
r"^\s+(or|and)?\s*custom_llm_provider", line
193-
):
194-
earlier.update(re.findall(r'custom_llm_provider\s*==\s*"([a-z0-9_\-]+)"', line))
195-
if not earlier:
196-
pytest.skip("could not parse litellm's dispatch chain")
197-
198-
actually_preempted = set(openai_compatible_providers) & earlier
199-
assert actually_preempted == set(_PREEMPTED_BY_OWN_HTTP_HANDLER), (
200-
"litellm's dispatch chain changed. Providers newly intercepted by their own "
201-
f"HTTP handler: {sorted(actually_preempted - set(_PREEMPTED_BY_OWN_HTTP_HANDLER))}; "
202-
"no longer intercepted: "
203-
f"{sorted(set(_PREEMPTED_BY_OWN_HTTP_HANDLER) - actually_preempted)}"
204-
)
205-
206-
def test_an_unknown_routing_table_stands_down_rather_than_guessing(
207-
self, monkeypatch, caplog
208-
):
209-
"""If the routing table cannot be read we do not know whether the OpenAI client
210-
instrumentor is already recording a call. Recording anyway would double-count
211-
every openai-compatible provider, and a doubled token or cost figure is worse
212-
than a missing one: the gap is visible and warned about, the doubling is silent
213-
and gets believed. So the recorder stands down entirely."""
214-
import builtins
215-
216-
real_import = builtins.__import__
217-
218-
def no_constants(name, *args, **kwargs):
219-
if name == "litellm.constants":
220-
raise ImportError("litellm.constants is gone")
221-
return real_import(name, *args, **kwargs)
222-
223-
monkeypatch.setattr(builtins, "__import__", no_constants)
224-
_genai_metrics._reset_for_tests()
225-
226-
with caplog.at_level("WARNING", logger=_genai_metrics.logger.name):
227-
assert _genai_metrics._over_openai_client("groq") is None
228-
assert _genai_metrics._over_openai_client("anthropic") is None
229-
assert _split_model("groq/llama3-8b-8192") == ("groq", None)
230-
231-
assert any(
232-
"openai_compatible_providers is unavailable" in r.message
233-
for r in caplog.records
234-
), [r.message for r in caplog.records]
235-
236-
def test_a_real_sgp_obs_is_not_started_when_routing_is_unknown(self, monkeypatch):
237-
"""The property that actually protects the data: no record is started at all,
238-
rather than one started with a guessed transport."""
239-
import sys
240-
import builtins
241-
242-
started = []
243-
244-
class _Genai:
245-
CHAT = "chat"
246-
OPENAI_SPEC = "openai"
247-
OPENAI = "openai"
248-
249-
@staticmethod
250-
def call(**kwargs):
251-
started.append(kwargs)
252-
return object()
253-
254-
module = type(sys)("sgp_obs.metrics")
255-
module.genai = _Genai
256-
monkeypatch.setitem(sys.modules, "sgp_obs", type(sys)("sgp_obs"))
257-
monkeypatch.setitem(sys.modules, "sgp_obs.metrics", module)
258-
259-
real_import = builtins.__import__
260-
261-
def no_constants(name, *args, **kwargs):
262-
if name == "litellm.constants":
263-
raise ImportError("litellm.constants is gone")
264-
return real_import(name, *args, **kwargs)
265-
266-
monkeypatch.setattr(builtins, "__import__", no_constants)
267-
_genai_metrics._reset_for_tests()
268-
269-
assert inference_call({"model": "groq/llama3-8b-8192"}) is _genai_metrics._NULL_CALL
270-
assert started == [], "a record was started with an unknown routing table"
271-
272153
def test_an_unresolvable_model_does_not_spam_stdout(self):
273154
"""litellm prints a red "Provider List" banner to STDOUT (not logging, so it
274155
cannot be filtered) every time resolution fails. Uncached, an agent on a model

‎src/agentex/lib/core/observability/sgp_obs_setup.py‎

Lines changed: 8 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,6 @@
6161
logger = make_logger(__name__)
6262

6363
_status: str | None = None
64-
65-
# Which app, if any, was handed to ``sgp_obs.init()``. ``init()`` is process-wide and is
66-
# not meant to run twice, but the ASGI instrumentation it installs is per-APP — so a
67-
# second application arriving later silently gets none of it. Remembered so that case can
68-
# at least be named; see the warning in :func:`init_sgp_obs`.
69-
_wired_app: Any = None
70-
7164
# sgp-obs' own truthy set (sgp_obs.env._TRUTHY), so "is the master switch on?" is
7265
# answered the same way here as in the library deciding whether to wire.
7366
_TRUTHY = {"1", "true", "yes", "on"}
@@ -88,20 +81,6 @@ def _master_switch_on() -> bool:
8881
return (os.getenv("SGP_OBS_ENABLED") or "").strip().lower() in _TRUTHY
8982

9083

91-
# Lets a deployment keep traces on while turning GenAI model spans off. The bridge is
92-
# installed by THIS package (sgp_obs.init does not install it, see
93-
# _install_openai_agents_bridge), so without a gate here there is no way to switch it
94-
# off short of disabling the traces signal entirely.
95-
#
96-
# Default ON: an unset variable must not change what any agent does today.
97-
_GENAI_SWITCH = "SGP_OBS_GENAI_ENABLED"
98-
99-
100-
def _genai_tracing_on() -> bool:
101-
raw = (os.getenv(_GENAI_SWITCH) or "").strip().lower()
102-
return raw == "" or raw in _TRUTHY
103-
104-
10584
def init_sgp_obs(app: Any = None) -> str:
10685
"""Wire sgp-obs if it is installed and enabled. Returns a status; never raises.
10786
@@ -113,24 +92,10 @@ def init_sgp_obs(app: Any = None) -> str:
11392
It is also what installs the trace-context ingress middleware, so an incoming
11493
``traceparent`` continues into the agent's spans rather than starting a new trace.
11594
"""
116-
global _status, _wired_app
95+
global _status
11796
if _status is not None:
11897
# init() is not meant to run twice, and a Temporal worker plus an ACP
11998
# server can both reach this in one process.
120-
if _status.startswith("wired") and app is not None and app is not _wired_app:
121-
# Everything init() set up process-wide (providers, exporters, the egress
122-
# instrumentation) still applies to this app. What does NOT is the per-app
123-
# ASGI layer, and that is the half nothing else would report.
124-
logger.warning(
125-
"sgp-obs was already initialized %s, so this application does not get "
126-
"the ASGI instrumentation: no http.server.* for its own entry point, "
127-
"and an incoming traceparent starts a new trace instead of continuing "
128-
"one. Everything process-wide (model, egress, logs) is unaffected. "
129-
"init() cannot safely run twice, so construct whichever application "
130-
"serves agent traffic before anything else calls init_sgp_obs() — note "
131-
"AgentexWorker.run() initializes without an app.",
132-
"without an application" if _wired_app is None else "for a different application",
133-
)
13499
return _status
135100

136101
try:
@@ -190,18 +155,9 @@ def init_sgp_obs(app: Any = None) -> str:
190155
_hand_logging_to_the_pipeline()
191156

192157
if "traces" in handles:
193-
if _genai_tracing_on():
194-
_install_openai_agents_bridge()
195-
else:
196-
logger.info(
197-
"%s is set falsey, so the sgp-obs openai-agents bridge was not "
198-
"installed; Runner turns will produce no model-operation spans. Other "
199-
"traces are unaffected.",
200-
_GENAI_SWITCH,
201-
)
158+
_install_openai_agents_bridge()
202159
_warn_if_correlation_backend_mismatched()
203160

204-
_wired_app = app
205161
_status = "wired:" + ",".join(sorted(handles))
206162
logger.info("sgp-obs wired (%s)", _status)
207163
return _status
@@ -387,19 +343,11 @@ async def shutdown_sgp_obs(budget_s: float = SGP_OBS_SHUTDOWN_BUDGET_S) -> None:
387343
delays nothing else — but it can still burn the grace period the runtime needs to
388344
exit cleanly, which is what the deadline is for.
389345
390-
KNOWN LIMIT: the budget bounds how long we *await*, not how long the process takes
391-
to exit. If it expires, ``sgp_obs.shutdown()`` has not finished, so the providers it
392-
built have not been shut down — and OpenTelemetry's ``TracerProvider`` and
393-
``MeterProvider`` both default to ``shutdown_on_exit=True``, which does
394-
``atexit.register(self.shutdown)``. Those hooks still run at interpreter exit and
395-
can flush past this deadline.
396-
397-
Not fixable from here without reaching into providers this package does not own:
398-
unregistering their atexit hooks would also strip the runtime-owned providers that
399-
sgp-obs deliberately leaves alone (see the note above about operator injection).
400-
The real fix belongs in sgp-obs — build its own providers with
401-
``shutdown_on_exit=False`` now that it offers an explicit ``shutdown()``, or have
402-
that ``shutdown()`` take a deadline.
346+
KNOWN LIMIT: this bounds how long we await, not how long the process takes to exit.
347+
OTel providers default to ``shutdown_on_exit=True``, so on overrun their own atexit
348+
hooks still flush. Fixing that belongs in sgp-obs (own providers with
349+
``shutdown_on_exit=False``, or a deadline on its ``shutdown()``); unregistering them
350+
from here would also strip runtime-owned providers it deliberately leaves alone.
403351
"""
404352
if _status is None or not _status.startswith("wired"):
405353
return
@@ -447,9 +395,8 @@ def _flush() -> None:
447395

448396

449397
def _reset_for_tests() -> None:
450-
global _status, _wired_app
398+
global _status
451399
_status = None
452-
_wired_app = None
453400
# The logging hand-over is a process-wide latch too, and a test that wired the
454401
# logs signal would otherwise leave make_logger attaching nothing for the rest
455402
# of the session.

0 commit comments

Comments
 (0)