Skip to content

Conversation

@alibeyram
Copy link
Contributor

Related to issue #3395
@DouweM am doing them one by one.

Copy link
Collaborator

@DouweM DouweM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alibeyram Thanks Ali, a few notes. It makes sense to go one step at a time as you mentioned in the description, but I will only merge this once all the paths mentioned in #3395 (comment) are covered so that we can ensure the instrumentation is consistent and users won't be confused.

instrumentation_version=self.ctx.instrumentation_version,
usage=self.ctx.usage,
)
output_tool_flag = False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified as is_output_tool = ..., without the if/else, but I think we don't need this check here anymore since we can also do it inside the call method, now that we always use that. So please move this check into the method.

)
output_tool_flag = False

return await self._call_function_tool(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is no longer accurate, because we also use it for output tools now. Can we either merge the 2 methods (as there's no need for a separate _call_tool anymore), or call this method _call_tool_traced? The latter may be better to keep the concerns separated.


span_attributes = {
'gen_ai.tool.name': call.tool_name,
'gen_ai.tool.name': tool_name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This arg should always have the actual tool name, so please change this back

'gen_ai.tool.call.id': call.tool_call_id,
**({instrumentation_names.tool_arguments_attr: call.args_as_json_str()} if include_content else {}),
'logfire.msg': f'running tool: {call.tool_name}',
'logfire.msg': f'running tool: {tool_name}',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I think we should say validating output: {tool_name}

@github-actions
Copy link

This PR is stale, and will be closed in 3 days if no reply is received.

@github-actions github-actions bot added the Stale label Nov 20, 2025
@alibeyram
Copy link
Contributor Author

@DouweM Ok I will do all of this. I just have been very busy last few days.

@DouweM DouweM removed the Stale label Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants