Conversation
util/opentelemetry-util-genai/src/opentelemetry/util/genai/emitters/utils.py
Outdated
Show resolved
Hide resolved
util/opentelemetry-util-genai/src/opentelemetry/util/genai/handler.py
Outdated
Show resolved
Hide resolved
wrisa
left a comment
wrisa
left a comment
There was a problem hiding this comment.
LGTM. Sometime later we need to move agent_id out of GenAI invocation type.
| agent_name_value = context_agent.agent_name or context_agent.name | ||
| inv.agent_name = _safe_str(agent_name_value) | ||
| inv.agent_id = str(context_agent.run_id) | ||
| inv.agent_id = _agent_span_id(context_agent) |
There was a problem hiding this comment.
I think agent id should be coming from the framework and should not be span id, let's discuss with the team ?
| "run_id": str(invocation.run_id), | ||
| "parent_run_id": str(invocation.parent_run_id) | ||
| if invocation.parent_run_id | ||
| "trace_id": f"{invocation.trace_id:032x}" |
There was a problem hiding this comment.
worker.py still references run_id. check _process_evaluation line#182
| if isinstance(agent, AgentInvocation): | ||
| try: | ||
| if self._agent_context_stack: | ||
| if self._agent_context_stack and agent.agent_id is not None: |
There was a problem hiding this comment.
wouldn't the agent_id be None for outermost agent since no parent sets it? In that case its pushed on the stack ins start https://github.com/signalfx/splunk-otel-python-contrib/pull/228/changes#diff-ceaf683887146bf95f37a7ebb888621d695e881d6e69059e4c7b3a9bc0120f9bR972 with None value but never popped here cause of not None check.
| if isinstance(agent, AgentInvocation): | ||
| try: | ||
| if self._agent_context_stack: | ||
| if self._agent_context_stack and agent.span_id is not None: |
There was a problem hiding this comment.
fail_agent checks for agent.span_id instead of agent_id. In start agent_id is pushed onto the stack https://github.com/signalfx/splunk-otel-python-contrib/pull/228/changes#diff-ceaf683887146bf95f37a7ebb888621d695e881d6e69059e4c7b3a9bc0120f9bR972, so comparing against hex span_id will never match.
Both this and mismatch in stop_agent will cause stack accumulation.
There was a problem hiding this comment.
isn't _span_registry removed?
| if processor._workflow is not None: | ||
| # Parent run_id should be set (either to agent or workflow) | ||
| assert ( | ||
| getattr(tool_state.invocation, "parent_run_id", None) is not None |
There was a problem hiding this comment.
parent_run_id reference, can you please check the whole codebase for references to run_id, parent_run_id and _span_registry?
|
|
||
| with self._pending_lock: | ||
| self._pending[run_id] = invocation | ||
| self._pending[span_id] = invocation |
There was a problem hiding this comment.
will span_id ever be None. If it can then it will be overridden by multiple
* lint updates, test fixes * remove auto generation of agent_id * resolve rebase conflicts * restore context clarification and tests * clean up dead code in llamaindex, align agent_id references
keith-decker
force-pushed
the
hybim-447_remove-run-id
branch
from
1143763 to
82a9511
Compare
4 participants
Remove
run_idfrom GenAI UtilsSummary
This PR removes the
run_idandparent_run_idfields from the GenAI types and eliminates the associated span/entity registries fromTelemetryHandler. OpenTelemetry's native trace context propagation handles parent-child relationships, making these custom tracking mechanisms redundant.Changes
Core Types (
types.py)run_id: UUIDfield fromGenAIbase classparent_run_id: Optional[UUID]field fromGenAIbase classuuid4import (no longer needed)TelemetryHandler (
handler.py)_span_registry: dict[str, Span]- was used to lookup spans by run_id_entity_registry: dict[str, GenAI]- was used to lookup entities by run_idEmitters
SpanEmitter,MetricsEmitter,SplunkEmitter,TestEmitterto removerun_idfrom attribute dictionariesrun_idfrom debug output indebug.pyInstrumentation Packages
run_id/parent_run_idmapping logicExamples & Tests
run_idreferences from all example scriptsrun_idWhy
trace.Contextalready provides parent-child linking viatrace_idandspan_idrun_idis not part of OpenTelemetry semantic conventions for GenAI