feat: add OpenAI Agents SDK support to OpenInference mapper - #366
feat: add OpenAI Agents SDK support to OpenInference mapper#366liramon2 wants to merge 4 commits into
Conversation
|
@strandly-the-agent Review this PR. Consolidate findings into a single comment. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@strandly-the-agent Here are the instrumentation:
Here are the current findings: For OpenAI agents SDK + traceloop instrumentation:
For OpenAI agents SDK + OpenInference instrumentation:
NOTE: In the current mappers' implementation strategy, I don't think we should use composition, and inheritance. And the mappers are grouped by instrumentation frameworks. I need you to:
|
|
Verified all seven claims against the instrumentation sources (
Design answer (Q2/Q3): agree — no inheritance, and no new classes. Traceloop verification (T1–T4)T1 — GenAI conventions ✅. Fixture chat spans carry T2 —
T3 — ✅ confirmed in both fixtures. Root T4 — ✅ confirmed. In the ADOT fixture,
OpenInference verification (O1–O3)O2 — mechanism ✅, paths and indices need precision. Verified against the v2.0.0 instrumentor source and both #366 fixtures:
O3 — ✅ verified, with one important limit.
Design recommendation (Q2/Q3): no new classes, no inheritance — three named phases inside the two family mappersThe rule that matches the repo you already have: one mapper per emitting-convention family ( Why I'd agree about inheritance specifically: #365's subclass works by overriding The shape I'd standardize (both mappers already approximate it):
Three guardrails, all from bugs already found in these two PRs: each hook is exception-isolated (one producer's quirk must degrade to a warned-and-skipped trace, not kill the session — the #366 normalizer currently runs outside every try/except); each hook checks scope on the spans it touches (the #366 walk doesn't); and each hook gets producer fixtures that pin it (mutation testing showed the Applied to the two PRs:
One thing I'd not do: don't introduce a producer-level abstraction (base class or protocol for "producers") until a third family needs it. Two call sites is a pattern; codifying it early is how the mapper layer gets rigid. Verification by |
|
@strandly-the-agent Review this PR. Consolidate findings into a single comment. For your questions:
|
|
Re-reviewed at Verdict: no blockers remain — both round-1 🔴s are fixed and verified. Three 🟡 should-fixes before un-drafting:
Suites: mapper 389 passed (was 385) · extractors/types/detectors 235 passed. Your three answers, closed out: coordinator exists / wrapper doesn't — implemented exactly that way and verified in both fixtures. Ordering with #365 — noted, no dependency. No integ tests yet — matches the #340 → #353 precedent; a tracking issue would close the loop but isn't blocking. Fix verification — round-1 repros re-run at
|
|
@strandly-the-agent Review this PR. Consolidate findings into a single comment. |
|
Re-reviewed at All three 🟡s from my last review are fixed and verified — nothing left that I'd block or should-fix. From my side this is ready to come out of draft.
Also cleaned up from the appendix, verified: cycle guard ( Regression spot-checks all green: multi-turn + reasoning still resolve correctly, ADOT fixture still yields 2 agents with per-agent tools, mapper suite 390 passed · downstream 235 passed. Still-open ⚪ (all contrived inputs, previously disclosed, fine to leave): empty-string final answer drops the span; an empty-string last message falls back to the previous message's text; a same-message Follow-up review by |
Description
Add OpenAI Agents SDK support to OpenInference mapper. This handles both Cloudwatch ADOT and in-memory exporter formats. No inheritance or composition is used because that is a one-way door decision that calls for a larger refactor.
This also fixes parent span ids in
OpenInferenceSessionMapperso that they point to converted spans. This preserves the agent-tool scopes when converting from OpenAI Agent traces (or other OpenInference traces) to evaluator inputs.Related Issues
#364
Documentation PR
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.