feat(mappers): add OpenAI Agents OTel session mapper - #365
Conversation
|
@strandly-the-agent Review this PR. Consolidate findings into 1 comment. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@strandly-the-agent Review this PR. Consolidate findings into 1 comment. Accepting the risk of finding 3 because it is difficult to reach. |
This comment was marked as resolved.
This comment was marked as resolved.
e0a31b2 to
9fb48f5
Compare
|
@strandly-the-agent Review this PR. Consolidate any findings into a single comment. |
|
TL;DR — follow-up review of just the ✅ Previous findings — all closed
You also took the All three new findings are in 🟡 6. Duplicate agent names — only the last span with a given name is reparented
🟡 7. Handoff edges can build a cyclic or self-referential parent tree
Being straight about severity: I tried to make this hurt and couldn't. With that, 🟡 8. Two handoffs to the same target make the tree depend on raw span orderSame lines — last-write-wins. Given Since span order isn't guaranteed stable across exporters, the same trace can evaluate differently on two runs. Worth at least a documented tie-break (earliest handoff wins, say) so it's a decision rather than an accident. Appendix — non-blocking (3)⚪ The new logic has no edge-case tests. ⚪ Docstrings that explained non-obvious ordering were deleted in this commit. The class docstring lost its explanation of why the overrides exist, and ⚪ Consequence of a nit I raised last round. Review shape, and one thing I got wrongShape: a self-run follow-up scoped to the Something I got wrong, in case the diff misleads anyone else: reading the unified diff for the new test file, it looked like it contained two syntax errors (a stray Tree restored to PR head after every experiment; nothing left modified. Automated review — solid work for a human to approve, not a gate. |
|
@strandly-the-agent Review this PR. Consolidate any findings into a single comment. The 3 findings found in the last round are theoretical, not realistic. |
|
No new findings. Reviewed the ✅ Your call on 6–8 is a fair one, and my own evidence supports it. All three needed a multi-agent topology that appears in neither capture, and when I went looking for consequences I found none worth the name: no hang anywhere, tool ownership stayed correct, and the only reachable effect was One thing worth recording, since it's the useful outcome here: the new docstring on What I ranSelf-run follow-up on a 4-line docs delta — no fan-out, which would have been overkill here. Full history for anyone joining: findings 1, 2, 4 fixed at Automated review — solid work for a human to approve, not a gate. |
Description
Add session mapper for agents created with OpenAI Agent SDK and instrumented with OpenLLMetry Traceloop. The mapper inherits from
GenericGenAISessionMapperbecause OpenAI Agent + Traceloop traces mostly follow GenAI conventions. Divergences from the GenAI spec are handled within the new mapper.This also fixes parent span ids in
GenericGenAISessionMapperso that they point to converted spans. This preserves the agent-tool scopes when converting from OpenAI Agent traces (or other GenAI traces) to evaluator inputs.Related Issues
#319
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.