feat(obs): wire sgp-obs from the SDK for traces, metrics and logs - #523
Draft
stephen-wang24 wants to merge 5 commits into
Draft
stephen-wang24 wants to merge 5 commits into
stephen-wang24 wants to merge 5 commits into
Conversation
This reverts commit 687ebfb.
…agentex's The hand-over added in #518 had two halves, and only one of them covered an agent's own modules. The latch in `make_logger` never looked at the logger's name, so anything created after init was fine. The sweep matched the `agentex` prefix, so a logger created BEFORE init under any other name kept its handler and went on printing a second, ungoverned copy of every record. That is not a corner case: agents call `make_logger(__name__)` from their own modules, and `project.acp` -- the module that builds the ACP server, in every scaffold -- logs at import, which is necessarily before `init_sgp_obs` runs. Measured on dbt-assistant running 0.27.0b1: 123 of 3361 log lines were the second copy, each 80 microseconds after its governed twin, carrying `name`/`request_id` but no `trace_id`, `span_id`, `source` or `agent_id`. Since it is emitted before the pipeline's filters, it also escapes the allowlist and the truncation. The SDK cannot know an agent's package name, so `make_logger` now marks each handler it attaches and the sweep takes back exactly those, on a logger of any name. Prefix matching stays for `agentex.*` itself, where every handler is ours by definition. A handler this module did not attach is still left alone -- litellm's three loggers and anything else keep what their owner set up, which is why sgp-obs warns about them rather than stripping them. Renamed `route_agentex_loggers_to_root` to `route_loggers_to_root`, since "agentex loggers" is what the bug was. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`request_id` had exactly one writer: `CustomJSONFormatter`, on the handler
`make_logger` attaches to each module's own logger. Removing that handler is
what stops the duplicate line -- and it takes the field's only writer with it,
so without this the id would not move to the governed copy, it would disappear.
Measured on dbt-assistant: `request_id` was on 5.2% of lines, which were
exactly the ungoverned copies, against `trace_id` on 90.6%.
The ACP middleware now binds its id into sgp-obs' shared correlation context,
which the logs pipeline enriches every record from. sgp-obs can also fill that
context from its own `RequestIdMiddleware`; binding the SDK's id instead keeps
ONE generator for the value, so the id in the logs is the id
`ctx_var_request_id` gives application code and the id `x-request-id` carried
in.
Deliberately not written onto the record here. The pipeline's enrich stage runs
on a copy of the record at handler time and treats a hand-set value as
authoritative, which cannot collide with a caller's own field. Setting the
attribute up front does collide: with `request_id` already on the record, the
stdlib raises `KeyError: Attempt to overwrite 'request_id' in LogRecord` from
`logger.info(..., extra={"request_id": ...})` -- measured, not theoretical, and
an unacceptable way for telemetry to reach an agent.
Fail-open throughout, and the optional import is resolved once: Python does not
cache a failed import, so attempting one per request would re-walk sys.path for
the majority of agents that never install sgp-obs.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Python does not cache a FAILED import, so `from sgp_obs.metrics import genai`
inside inference_call re-walked sys.path on every single litellm call — for the
majority of agents, which are the ones that do not have sgp-obs installed.
Measured on the published 0.27.0b2 wheel in a clean venv with no sgp-obs, best
of 5x500 calls against a mocked transport:
0.26.0 11.6 us/call
0.27.0b2 as published 84.0 us/call
0.27.0b2 + this fix 11.8 us/call
The bare failed import is 62us of that, with only five sys.path entries; a
container image has more. Negligible beside a real model call, but it is pure
waste on the hot path, and the module docstring claimed the fallback "costs
nothing", which was the one part of it that was not true.
base_acp_server.py already solved exactly this for sgp_obs.context with an
_OBS_CONTEXT_UNRESOLVED sentinel, and said why in a comment. This applies the
same shape so the two files agree. The one-time debug line moves into the
resolver, whose body now runs exactly once — which retires the separate
_warned latch rather than leaving two latches for one fact.
Adds _reset_for_tests(), matching sgp_obs_setup.py and utils/logging.py. The
handle is process-wide state, so without it the first test to run with sgp-obs
absent would cache None for the rest of the session and every later test that
injects a fake sgp_obs.metrics would silently exercise the null path instead of
the one it means to.
Tests: 22 pass, up from 19. The new ones count import attempts under a counting
__import__ hook — 50 calls give 50 attempts on the old body and 1 on this one —
and pin that a present sgp-obs is still used on calls two and three, so the
cache cannot degrade a working install to the null path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+89
to
+92
| if _status is not None: | ||
| # init() is not meant to run twice, and a Temporal worker plus an ACP | ||
| # server can both reach this in one process. | ||
| return _status |
There was a problem hiding this comment.
_status returns before a later app reaches sgp_obs.init(app=...). If AgentexWorker initializes first in a combined process, or code creates a second BaseACPServer, that app never gets the middleware which this function says provides HTTP spans and incoming trace context. Keep process-wide provider setup idempotent, but track and instrument each app separately.
Knowledge Base Used: Observability
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/core/observability/sgp_obs_setup.py
Line: 89-92
Comment:
The process-wide `_status` returns before a later app reaches `sgp_obs.init(app=...)`. If `AgentexWorker` initializes first in a combined process, or code creates a second `BaseACPServer`, that app never gets the middleware which this function says provides HTTP spans and incoming trace context. Keep process-wide provider setup idempotent, but track and instrument each app separately.
**Knowledge Base Used:** [Observability](https://app.greptile.com/scale-ai/-/custom-context/knowledge-base/scaleapi/scale-agentex-python/-/docs/observability.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
…obs flush Three review findings from #523, all confirmed against the code before fixing. **The worker dropped its own business spans (P1).** Its `finally` called the sync and sgp-obs drains but not `shutdown_default_span_queue()`. Standard Temporal activities trace through `AsyncTracer` (core/temporal/activities/__init__.py:66), and `AsyncTrace.__init__` takes `get_default_span_queue()` when no queue is passed (core/tracing/trace.py:406) — so a worker's spans sit in exactly the queue nothing drained. The async drain now runs first, matching the ACP lifespan's order, and a test asserts both paths carry all three drains so they cannot drift apart again. **The sgp-obs flush had no deadline (P1).** `asyncio.to_thread(shutdown)` was awaited unbounded, so a hung exporter held both callers until the pod was killed. Worse, `asyncio.run` joins the default executor on the way out, so even adding a `wait_for` would not have helped — the process still blocks on the export the deadline was meant to escape. Measured, 20s stalled flush under a 0.25s budget: asyncio.to_thread + wait_for process exits at 20.04s daemon thread + wait_for process exits at 0.31s Now a daemon thread under a 5s budget, the same shape and the same reasoning as `shutdown_sync_tracing_processors`, whose docstring already spelled this out. The overrun is warned about rather than silent. Covered by a subprocess test, because interpreter shutdown cannot be observed from inside the test process. **A second app was silently uninstrumented (P2).** `init()` is process-wide and must not run twice, but the ASGI instrumentation it installs is per-app, so a later `BaseACPServer` — or an ACP server built after `AgentexWorker.run()`, which inits with no app — got no `http.server.*` and no ingress trace continuation, with nothing saying so. This warns instead, naming the ordering that causes it. That last one is deliberately a diagnostic rather than a repair: instrumenting the second app would mean calling into sgp-obs for a per-app entry point I cannot verify from here (it is not a dependency of this package and not on public PyPI), and guessing at an API is worse than making the silent case audible. Flagged for follow-up. Tests: 63 in the two obs suites, 1714 in the repo suite, ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
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
1 participant
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.
Wires
sgp-obsfrom the SDK so an agent adopts traces, metrics and logs by installing apackage and setting environment, instead of carrying the wiring itself across ~147 agent
repos.
This is #518 re-landed on
next(it was reverted in #522 while the behaviour was beingmeasured on sgp-dev), plus three follow-up fixes found during that measurement. First
commit is a revert-of-the-revert so the add → revert → re-add history stays legible.
No version change here. The
0.27.0b2bump that exists on the beta branch (#521) isdeliberately left behind —
.release-please-manifest.json, bothpyproject.tomls and_version.pyare untouched at0.26.0, so release-please keeps ownership. #520 alreadyproposes
0.27.0, and these commits just addfeat(obs)/fix(obs)entries to it.Safety for agents that will never have
sgp-obssgp-obsis served from Scale's curated CodeArtifact mirror, not public PyPI, so mostagents will not have it. This change is inert for them, and that is measured rather
than argued.
It is not a dependency, and not an extra
There is deliberately no
obsextra —adk/pyproject.tomldocuments why (declaring itmakes this repo's own uv workspace unresolvable: locking must resolve every declared
optional dependency and neither
--no-extranoroverride-dependenciesexempts one).The dependency is the agent's to declare; the SDK wires it only if it is importable.
Comparing the published
0.26.0and0.27.0b2wheels:Requires-Dist)sgp-obsanywhere in package metadatadiffemptypip install agentex-sdkwith no CodeArtifact accessNo new transitive dependency either. (
scale-gp-beta>=0.5.0is sometimes attributed tothis work — it was raised in 0.23.0 and is already in current stable
0.26.0.)Behaviour is unchanged
A 28-check A/B probe was run against both published wheels in clean venvs with no
sgp_obs, covering: module imports; logger handler count, formatter, propagation andone-line-per-
info();BaseACPServer()construction, lifespan enter/exit,/healthz,JSON-RPC dispatch and
x-request-idhandling; and the litellm gateway across happy path,positional
model(args[0]), exception propagation,CancelledError, streaming, andmid-stream failure.
The two reports differ by one line: the version string.
Every import of
sgp_obssits inside atry. The logging rewrite is latched behindroute_loggers_to_root(), which only runs once the logs signal actually wires, somake_loggeris byte-for-byte equivalent otherwise.The half-configured case is loud, not silent
SGP_OBS_ENABLED=truewith the package absent still serves200, reportsnot_installed, and says so:Independently confirmed by building and running a real agent image with no
sgp-obsandno obs environment (
ilana_digital_twinon sgp-dev): image builds,sgp_obsabsent,init_sgp_obs()returns'not_installed'and never raises, agent imports and serves, CIgreen.
Builds are unaffected
All 38 scaffold Dockerfiles mount the CodeArtifact secret with
required=falseand carry# syntax=docker/dockerfile:1.3, so a build with no secret proceeds on the plain installpath. These templates only affect newly scaffolded agents; an existing agent keeps its own
Dockerfile.
Two deltas that DO apply to everyone
Being straight about what is not a no-op. Neither is gated on
sgp-obs.shutdown_sync_tracing_processors()drains a queue nothing drained beforeLiteLLMGatewayShutdown is a bug fix: the ACP lifespan only ever drained the async span queue, so a
sync agent dropped whatever business spans were still queued when the pod stopped —
including the ones an obs span's
agentex.business_trace_idresolves to. The drain isbounded (5s), runs the processors concurrently on daemon threads, and is fail-open.
Verified: a 30s stalled flush under the 5s budget returns at 5.00s and the process exits
at 5.53s, not 30s.
asyncio.wait_forcan stop awaiting a thread but cannot stop thethread, and
asyncio.runjoins the default executor — hence daemon threads rather thanto_thread. Comfortably inside a default 30sterminationGracePeriodSeconds, but worthknowing for SGP-tracing agents.
litellm: every completion now flows through a recorder. With
sgp-obsabsent that isa stateless shared null object whose
observe()is a pass-through and whose__aexit__returns
False, so it can never swallow a caller's exception. Measured safe under 50concurrent calls.
The last commit is what makes this free.
inference_calloriginally importedsgp_obsper call, and Python does not cache a failed import, so it re-walked
sys.pathonevery model call — 62µs each with only five path entries, and a container has more. Now
resolved once, the same way
base_acp_serveralready handlessgp_obs.context:0.26.00.27.0b2as publishedTiming note
uvexcludes pre-releases from a bareagentex-sdkspecifier, so0.27.0b2is invisibleto a fresh resolve today. Agents pinned loosely first pick this up when 0.27.0 ships
stable — which is what merging this and then #520 does. That is the intended rollout, but
it is the moment the above stops being theoretical, which is why it was measured first.
Testing
sgp-obsinstalledruff check src testsclean__import__hook: 50 calls give 50 attempts on the old body and 1 on this oneThe changes since the last review appear safe to merge. No new issue was confirmed.
Fix with agent prompt
Summary
This PR wires optional
sgp-obssupport into the SDK so agents can enable traces, metrics, and logs through installation and environment settings. It also adds LiteLLM metrics, request ID correlation, bounded telemetry shutdown, and private-index support for generated agent images.Diagram
sequenceDiagram participant App as Agent app participant SDK as AgentEx SDK participant Obs as sgp_obs participant Root as Root logger App->>SDK: Build ACP server or start worker SDK->>SDK: init_sgp_obs(app?) alt sgp_obs is unavailable SDK-->>App: not_installed else sgp_obs is available SDK->>Obs: "init(app=app, source="agentex")" Obs-->>SDK: wired signal handles opt logs are wired SDK->>Root: route_loggers_to_root() end SDK-->>App: wired status end App->>SDK: Run requests and model calls SDK->>Obs: Bind request ID and observe inference App->>SDK: Stop SDK->>SDK: Drain async span queue SDK->>SDK: Drain sync processors within 5s SDK->>Obs: Run shutdown() on daemon thread SDK-->>App: Continue after completion or 5sReviews (2) · Last reviewed commit: "fix(obs): drain the async span queue in ..."