-
Notifications
You must be signed in to change notification settings - Fork 273
[gen-ai] expand invoke_agent span documentation beyond remote agents #2881
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
.chloggen/main.yaml
Outdated
component: gen-ai | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: "`invoke_agent` spans now include span kind guidance (CLIENT/INTERNAL/SERVER) and clarify when `server.*` attributes should be set." |
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.
How about following?
s/span kind guidance (CLIENT/INTERNAL/SERVER)/span kind (CLIENT/INTERNAL/SERVER) guidance
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.
Thanks for the suggestion! Updated to be "span kind (CLIENT/INTERNAL/SERVER) guidance"
invocation of agents running in the same process or `SERVER` when instrumenting | ||
the agent service itself. | ||
It's RECOMMENDED to use `CLIENT` kind when the agent being instrumented usually runs | ||
in a different process than its caller or when the agent invocation happens over |
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.
Can you please add more clarification when to use different span kind, like what you clarified
CLIENT: For remote agent services (OpenAI Assistants API, AWS Bedrock)
INTERNAL: For in-process agents (LangChain, CrewAI agents)
SERVER: For instrumenting the agent service itself
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.
Updated documentation note section with examples
docs/gen-ai/gen-ai-agent-spans.md
Outdated
invocation of agents running in the same process or `SERVER` when instrumenting | ||
the agent service itself. |
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.
Server kind seems to be a step too far and there is a clear pattern of splitting the definition given that the attributes change.
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.
Fair point - I've removed the SERVER span kind references from this PR for now. Seems like this case will need further discussion and can be addressed in a follow-up.
docs/gen-ai/gen-ai-agent-spans.md
Outdated
in a different process than its caller or when the agent invocation happens over | ||
instrumented protocol such as HTTP. | ||
|
||
`server.address` and `server.port` attributes SHOULD be set when span kind is `CLIENT`. |
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.
Should be defined via the yaml model ie note on the requirement level.
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.
Thanks - moved to the yaml model as conditionally required attributes.
model/gen-ai/spans.yaml
Outdated
conditionally_required: if applicable. | ||
- ref: server.address | ||
requirement_level: | ||
conditionally_required: when span kind is `CLIENT`. |
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.
It's recommended now, is there a reason to change it?
conditionally_required: when span kind is `CLIENT`. | |
recommended: when span kind is `CLIENT`. |
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.
My intention was to make it required only when the span kind is CLIENT
. I can change it back to recommended though if this is confusing.
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.
Other conventions don't (conditionally) require server.address on logical operations (check out DB or messaging). LLM calls are inherently logical - underlying HTTP / gRPC / protocol spans would represent physical call and contain details. So I don't see a strong reason to change existing requirement level for LLMs and would suggest reverting it back to recommended
.
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.
Makes sense, thanks for the clarification. Reverted it back to recommended
.
Co-authored-by: Liudmila Molkova <[email protected]>
Fixes #2837
Changes
Expands the
invoke_agent
span semantic conventions to cover all agent invocation scenarios, not just remote agents.Added guidance for span kind:
Also added explicit guidance that
server.address
andserver.port
attributes should only be set when the span kind is CLIENT.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]