-
Notifications
You must be signed in to change notification settings - Fork 458
feat(agent): Add opt-in flag to include tool specs in traces for evaluation #1113
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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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 contribution.
After the discussion within the team, I believe we should:
-
Move the attributes to invoke_agent span > gen_ai.tool.definitions
(semantic conventions link). -
Move the flag (include_tool_definitions) into StrandsTelemetry class instead of in the Agent class as this is specifically for the trace.
Thanks for the feedback, will change it right away @poshinchen . |
|
Oh no, I messed up something by force pushing since I was having some rebase problems locally and now it closed the PR automatically. Sorry, could you reopen it or should I create a new PR @poshinchen . Nvm i was able to reopen it. |
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.
Hi, sorry
After a careful read from the otel documentation. We are expecting customers to set multiple values in OTEL_SEMCONV_STABILITY_OPT_IN env var
- As an example:
OTEL_SEMCONV_STABILITY_OPT_IN="gen_ai_latest_experimental,gen_ai_tool_definitions"allows latest semantic conventions, and opt-in gen_ai.tool.definitions.
That being said, could you do the following:
- In tracer.py, modify the parsing logic for use_latest_genai_conventions and include_tool_definitions (new).
- pass all_tools_config to
_start_agent_trace_spanand move tool_details = [...] into that method. - I wonder if it's possible to pass them as array?
- From the description:
It’s expected to be an array of objects where each object represents a tool definition. In case a serialized string is available to the instrumentation, the instrumentation SHOULD do the best effort to deserialize it to an array. When recorded on spans, it MAY be recorded as a JSON string if structured format is not supported and SHOULD be recorded in structured form otherwise.
No problem , will change it according your feedback. Thanks. Also, regarding your question about passing the data as an array: I looked into it, and you're right that the spec prefers a structured format. However, the OpenTelemetry Python SDK's set_attribute function doesn't have guaranteed support for complex nested objects like a list of dictionaries. What are your thoughts on this, open to being wrong about this. |
src/strands/agent/agent.py
Outdated
| self.trace_span = self._start_agent_trace_span(messages) | ||
| self.trace_span = self._start_agent_trace_span( | ||
| messages, | ||
| all_tools_config=self.tool_registry.get_all_tools_config() or {}, |
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.
or {} should not be required (source). And, what about just tools_config?
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.
Yes will change it.
src/strands/telemetry/config.py
Outdated
| Args: | ||
| tracer_provider: Optional pre-configured tracer provider. | ||
| If None, a new one will be created and set as global. | ||
| include_tool_definitions: Whether to include tool definitions in traces. |
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 removed
src/strands/agent/agent.py
Outdated
| if self.tracer.include_tool_definitions and all_tools_config: | ||
| try: | ||
| tool_details = [ | ||
| { | ||
| "name": name, | ||
| "description": spec.get("description"), | ||
| "inputSchema": spec.get("inputSchema"), | ||
| "outputSchema": spec.get("outputSchema"), | ||
| } | ||
| for name, spec in all_tools_config.items() | ||
| ] | ||
| serialized_tools = serialize(tool_details) | ||
| span.set_attribute("gen_ai.tool.definitions", serialized_tools) | ||
| except Exception: | ||
| # A failure in telemetry should not crash the agent | ||
| logger.exception("failed to attach tool metadata to 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.
This can be moved into tracer.py?
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.
Ok got it , I will move it to traceer.py
| messages=messages, | ||
| agent_name=self.name, | ||
| model_id=model_id, | ||
| tools=self.tool_names, |
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.
Let's keep it as customers might still track the tools?
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.
Yes makes sense, I shouldnt have removed this
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.
left some comments
src/strands/agent/agent.py
Outdated
| self.trace_span = self._start_agent_trace_span(messages) | ||
| self.trace_span = self._start_agent_trace_span( | ||
| messages, | ||
| tools_config=self.tool_registry.get_all_tools_config(), |
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 you don't need this line. Instead, at line 936, you can do:
span = self.tracer.start_agent_span(
messages=messages,
agent_name=self.name,
model_id=model_id,
tools=self.tool_names,
system_prompt=self.system_prompt,
custom_trace_attributes=self.trace_attributes,
tools_config=self.tool_registry.get_all_tools_config(),
)
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.
Ok got it
src/strands/agent/agent.py
Outdated
| self._append_message(assistant_msg) | ||
|
|
||
| def _start_agent_trace_span(self, messages: Messages) -> trace_api.Span: | ||
| def _start_agent_trace_span(self, messages: Messages, tools_config: Optional[dict] = None) -> trace_api.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.
Same from above, then this doesn't need to be changed.
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.
fixed
src/strands/agent/agent.py
Outdated
| if tools_config: | ||
| self.tracer.add_tool_definitions_to_span(span, tools_config) | ||
|
|
||
| return 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.
these are not needed too
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.
fixed
src/strands/telemetry/tracer.py
Outdated
|
|
||
| self._end_span(span, attributes, error) | ||
|
|
||
| def add_tool_definitions_to_span(self, span: Span, tools_config: dict) -> None: |
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.
make this as _construct_tool_definitions, then you can call it in start_agent_span with:
if self.include_tool_definitions:
tool_definitions= self._construct_tool_definitions(tools_config)
attributes["gen_ai.agent.tools"] = serialize(tool_definitions)
src/strands/telemetry/tracer.py
Outdated
| span.set_attribute("gen_ai.tool.definitions", serialized_tools) | ||
| except Exception: | ||
| # A failure in telemetry should not crash the agent | ||
| logger.exception("failed to attach tool metadata to 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.
I prefer it to be warning instead of exception.
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.
ok fixed it. thanks.
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.
Left the comments. There's conflict too: This branch has conflicts that must be resolved: tests/strands/agent/test_agent.py
In addition, can you test it locally to see whether set_attribute works without serialization?
Otherwise I can test it on my end
I fixed the resolve conflicts and made the refactoring changes according to your feedback. Thanks for your help.
For serialization: I initially removed the serialize() call. While the runtime tests passed, it caused a mypy error because the project's AttributeValue type hint does not allow complex objects (list[dict]). To satisfy the static type checker, I had to re-add the |
Description
This PR implements a feature to include the specifications of all available tools in the main invoke_agent trace span. This provides context for external evaluation frameworks to accurately assess the agent's tool selection.I went with the opt-in approach because:
Key Changes
Related Issues
Closes #1083
Documentation PR
N/AType of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
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.