Skip to content

Enable Phoenix tracing for Prompt Agent (hosted) targets - #63

Merged
tangym merged 1 commit into
mainfrom
yemingtang/hosted-session-tracing
May 21, 2026
Merged

Enable Phoenix tracing for Prompt Agent (hosted) targets#63
tangym merged 1 commit into
mainfrom
yemingtang/hosted-session-tracing

Conversation

@tangym

@tangym tangym commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

target.trace: { backend: phoenix } was silently ignored for Prompt Agent (model-only) targets. The trace config was only consumed inside the if target.is_callable: branch in rollout.py, so HostedSession never set up instrumentation.

Fix

When target.trace is set for a hosted target, call phoenix.otel.register(auto_instrument=True) before building the session. This detects the installed openinference-instrumentation-litellm package and monkey-patches litellm so every acompletion call emits spans to Phoenix automatically.

  • Idempotent: module-level _hosted_trace_registered flag ensures it runs once per process
  • Graceful degradation: logs a warning if arize-phoenix-otel is not installed
  • No new dependencies required (uses existing optional deps)

Testing

Ran a smoke config with azure/gpt-5.4-nano + trace: { backend: phoenix }. Confirmed:

  • Log: Enabled Phoenix auto-instrumentation for hosted session tracing
  • 33 traces landed in Phoenix default project
  • All rollouts completed successfully

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks directionally good to me. I ran a local compile plus targeted runtime/import tests: 140 passed, 1 skipped.

I’d wait to approve/merge until #23 lands and this is rebased, since this currently touches p2m/stages/rollout.py and #23 moves that surface to inference.py / test-case terminology. After the rebase, the only thing I’d want to sanity-check is that the hosted-target trace path still registers Phoenix instrumentation in the renamed inference stage. A tiny unit test around “hosted target + target.trace registers once” would be nice, but I don’t think it needs to block if the smoke result holds.

@jakepresent

Copy link
Copy Markdown
Collaborator

Found one issue while doing a partial review before the rebase.

The existing targeted tests still pass locally:

  • uv run python -m compileall -q p2m
  • uv run pytest tests/test_runtime_modes.py tests/test_framework_agnostic.py tests/test_stage_runner_smoke.py -q
  • 137 passed, 1 skipped

But directly exercising _ensure_hosted_trace_instrumentation() crashes in my env. phoenix.otel.register(auto_instrument=True) attempts to auto-instrument all installed OpenInference instrumentors, not just LiteLLM. In this checkout it imported the CrewAI/ChromaDB stack and failed with a Pydantic config error before the hosted session was built.

That means target.trace on a hosted target can currently turn into a hard runtime crash, even though the intended behavior is graceful degradation. Also, _hosted_trace_registered = True is set before registration succeeds, so a failed registration attempt can leave the process in a “registered” state without instrumentation.

I think the fix is either:

  1. instrument only LiteLLM explicitly, rather than auto_instrument=True across every installed OpenInference integration, or
  2. catch broader registration failures, log a warning, and only mark the registration as successful after it actually succeeds.

After that plus the post-#23 rebase from rollout.py to inference.py, I’m good with the direction.

@tangym
tangym force-pushed the yemingtang/hosted-session-tracing branch from 0b9fb4a to 3cc91b4 Compare May 21, 2026 18:12
Add _ensure_hosted_trace_instrumentation() that registers Phoenix OTEL
and instruments only litellm (via LiteLLMInstrumentor) to capture spans
for HostedSession LLM calls.

- Use explicit LiteLLMInstrumentor().instrument() instead of
  auto_instrument=True to avoid side-effects from other providers
- Set _hosted_trace_registered flag only after successful registration
- Guard with ImportError (missing deps) and broad Exception handling
- Called from _build_target_session when target.trace is configured
@tangym
tangym force-pushed the yemingtang/hosted-session-tracing branch from 3cc91b4 to 6a0d0cd Compare May 21, 2026 18:26
@tangym

tangym commented May 21, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review! Updated with all the fixes:

  1. auto_instrument=True switched to explicit register() + LiteLLMInstrumentor().instrument() so we control exactly what gets instrumented and avoid the crash you flagged.

  2. Flag timing fixed: _hosted_trace_registered is now set after both register() and instrument() succeed, so a failure mid-setup correctly leaves the flag False for retry on next call.

  3. Broader exception handling: added a general except Exception with exc_info=True alongside the ImportError catch, so unexpected failures from Phoenix/OTel config issues get logged instead of bubbling up.

  4. Rebased onto main: resolved the renaming from refactor(p2m): terminology migration to maximize broad-developer recognition #23.

Also added unit tests (tests/test_hosted_trace_registration.py) covering the happy path (register-once idempotency), ImportError (missing deps), and RuntimeError (config failure).

@tangym
tangym merged commit 661f9d3 into main May 21, 2026
3 checks passed
tangym added a commit that referenced this pull request May 22, 2026
Add _ensure_hosted_trace_instrumentation() that registers Phoenix OTEL
and instruments only litellm (via LiteLLMInstrumentor) to capture spans
for HostedSession LLM calls.

- Use explicit LiteLLMInstrumentor().instrument() instead of
  auto_instrument=True to avoid side-effects from other providers
- Set _hosted_trace_registered flag only after successful registration
- Guard with ImportError (missing deps) and broad Exception handling
- Called from _build_target_session when target.trace is configured
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants