-
Notifications
You must be signed in to change notification settings - Fork 205
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
Adding span for invoke agent #1900
base: main
Are you sure you want to change the base?
Conversation
@gyliu513 this needs rebase after the experimental -> development rename |
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.
LGTM, just a few minor comments
1af7a29
to
b704514
Compare
@lmolkova I think I have addressed all comments here, can you help review again? Thanks! |
|---|---|---|---|---|---| | ||
| [`gen_ai.operation.name`](/docs/attributes-registry/gen-ai.md) | string | The name of the operation being performed. [1] | `chat`; `text_completion`; `embeddings` | `Required` |  | | ||
| [`gen_ai.system`](/docs/attributes-registry/gen-ai.md) | string | The Generative AI product as identified by the client or server instrumentation. [2] | `openai` | `Required` |  | | ||
| [`gen_ai.agent.description`](/docs/attributes-registry/gen-ai.md) | string | Free-form description of the GenAI agent provided by the application. | `Helps with math problems`; `Generates fiction stories` | `Conditionally Required` If provided by the application. |  | |
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 don't think it's common to have agent description of name around when invoking a service-side agent.
- bedrock only has agent id - https://docs.aws.amazon.com/bedrock/latest/APIReference/API_agent-runtime_InvokeAgent.html
- openai assistants only have id - https://platform.openai.com/docs/api-reference/runs/createThreadAndRun
- azure ai agents only have id - https://learn.microsoft.com/en-us/azure/ai-services/agents/overview#what-is-an-ai-agent
so I think we need to update requirement level on the name and description to something like recommended: when available
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.
sorry, this comment is about invoke_agent
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.
Done
docs/gen-ai/gen-ai-agent-spans.md
Outdated
|
||
The `gen_ai.operation.name` SHOULD be `invoke_agent`. | ||
|
||
The **span name** SHOULD be `invoke_agent {gen_ai.agent.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.
since gen_ai.agent.name
is rarely available for remote agents, I think we need to change it to
The **span name** SHOULD be `invoke_agent {gen_ai.agent.name}`. | |
The **span name** SHOULD be `invoke_agent {gen_ai.agent.name}` if `gen_ai.agent.name` is readily available. When `gen_ai.agent.name` is not available, it SHOULD be `invoke_agent`. |
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.
Done
|---|---|---|---|---|---| | ||
| [`gen_ai.operation.name`](/docs/attributes-registry/gen-ai.md) | string | The name of the operation being performed. [1] | `chat`; `text_completion`; `embeddings` | `Required` |  | | ||
| [`gen_ai.system`](/docs/attributes-registry/gen-ai.md) | string | The Generative AI product as identified by the client or server instrumentation. [2] | `openai` | `Required` |  | | ||
| [`gen_ai.agent.description`](/docs/attributes-registry/gen-ai.md) | string | Free-form description of the GenAI agent provided by the application. | `Helps with math problems`; `Generates fiction stories` | `Conditionally Required` If provided by the application. |  | |
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.
is there any reason we don't have gen_ai.agent.id
on create agent 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.
Usually, the agent id should be generated when the agent is invoked. When the agent was created, the end user can specified the agent name, but for agent id, it seems need to be a run time attribute, comments?
Signed-off-by: Guangya Liu <[email protected]>
Fixes #1842 #1924
Changes
Please provide a brief description of the changes here.
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]