-
Notifications
You must be signed in to change notification settings - Fork 789
Implement OpenAI Agents span processing #3817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Outdated
Show resolved
Hide resolved
...ion-genai/opentelemetry-instrumentation-openai-agents/tests/stubs/agents/tracing/__init__.py
Outdated
Show resolved
Hide resolved
|
||
processor = _OpenAIAgentsSpanProcessor(tracer=tracer, system=system) | ||
|
||
tracing = _load_tracing_module() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we are loading the module during init of the instrumentor and not at the beginning of runtime/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid errors at import if the openai agents SDK is not installed. We can move it around if it makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think there is a need for that. All other instrumentations follow the convention that the underlying library is assumed to be installed. You can move the import statements to top level.
...ry-instrumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/__init__.py
Outdated
Show resolved
Hide resolved
...ry-instrumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/__init__.py
Outdated
Show resolved
Hide resolved
...umentation-genai/opentelemetry-instrumentation-openai-agents/examples/zero-code/.env.example
Show resolved
Hide resolved
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Outdated
Show resolved
Hide resolved
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Outdated
Show resolved
Hide resolved
start_time = _parse_iso8601(getattr(trace, "started_at", None)) | ||
|
||
with self._lock: | ||
span = self._tracer.start_span( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have a span created for trace start? It's not really clearly defined in the semantic conventions what this span should look like or where in the agent process does on_trace_start
get called. Might be something to discuss in the SIG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on_trace_start
fires in the OpenAI Agents SDK whenever it starts a new agent workflow Trace.start()
we translate that event into a root span so every downstream generation/response/tool span
has a common parent. We currently tag that root as SpanKind.SERVER
because, in the Agent runtime model, the trace represents the server side orchestrating a request; this mirrors how other server
frameworks build a tree rooted at a SERVER span. The GenAI spec doesn’t yet spell out guidance for this top-level agent span, so we can bring it to the SIG for confirmation, but today the root span
gives us the correct hierarchy and timing for the whole workflow.
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Show resolved
Hide resolved
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Show resolved
Hide resolved
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Show resolved
Hide resolved
...trumentation-openai-agents/src/opentelemetry/instrumentation/openai_agents/span_processor.py
Outdated
Show resolved
Hide resolved
|
||
def _resolve_system(value: str | None) -> str: | ||
if not value: | ||
return GenAI.GenAiSystemValues.OPENAI.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think gen_ai.provider.name
MUST be openai
for all openai spans. https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/openai.md#spans
} | ||
) | ||
|
||
_GEN_AI_PROVIDER_NAME = "gen_ai.provider.name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the sem conv symbol?
Description
Updates the previous barebones PR to include genai spans and traces.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Testing
cleanly with integer timestamps.
exporter errors.
timestamps and attribute mapping.
Environment
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.