trace the output tool call but not count it #3398
Conversation
DouweM
left a comment
DouweM
left a comment
There was a problem hiding this comment.
@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 |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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}', |
There was a problem hiding this comment.
This is where I think we should say validating output: {tool_name}
|
This PR is stale, and will be closed in 3 days if no reply is received. |
|
@DouweM Ok I will do all of this. I just have been very busy last few days. |
|
This PR is stale, and will be closed in 3 days if no reply is received. |
|
@alibeyram Hey Ali, are you still interested in working on this? |
|
Yes I am. Sorry has been very busy days for me. I might start a fresh PR given how much changes is there. |
|
@alibeyram If you start from scratch, I'd prefer you work on the same branch and just force-0push into this PR instead of creating a new PR! But I don't think starting from scratch will be needed, as this branch still has no conflicts with |
|
This PR is stale, and will be closed in 3 days if no reply is received. |
|
Closing this PR as it has been inactive for 10 days. |
2 participants
Related to issue #3395
@DouweM am doing them one by one.