Repository navigation
fix(util-genai): call the completion hook for tool invocations - #950
akshat-lakhera wants to merge 4 commits into
Conversation
Signed-off-by: Akshat Lakhera <akshatlnct2024@gmail.com>
88c4514 to
fe41a16
Compare
Pull request dashboard statusWaiting on the author · refreshed 2026-10-31 21:50 UTC Move out of draft to request review. Status above doesn't look right?
|
There was a problem hiding this comment.
🟡 Changes recommended
The redaction regression test enables content capture after handler construction, leaving redaction of captured secrets untested.
1 open issue
What changed in this PR
This PR addresses #678 by routing tool invocation content through the shared completion hook, allowing existing hooks to process tool arguments and results.
Changes:
- Call the completion hook when tool invocations finish or fail, representing content as tool-call message parts.
- Add tests for hook payloads, missing content, failure, and span redaction.
- Document the fix in a changelog fragment.
| File | Description |
|---|---|
util/opentelemetry-util-genai/tests/test_handler_completion_hook.py |
Adds tool completion-hook and redaction tests. |
util/opentelemetry-util-genai/src/opentelemetry/util/genai/_tool_invocation.py |
Invokes the hook with tool arguments and results. |
util/opentelemetry-util-genai/.changelog/950.fixed |
Records the tool hook fix. |
Review effort: Balanced
|
The following test is failing We probably need to add 'tool' to the hook_calls_per_run in |
…tent Signed-off-by: Akshat Lakhera <akshatlnct2024@gmail.com>
Hey , |
There was a problem hiding this comment.
I think we should take a step back..
I dont think ToolInvocations should use the upload hook.. my understanding is the upload hook is only for inference spans, to capture input / output messages / system instructions.. and that the upload hook stuff only appears in the inference span doc in sem conv...
Currently the agent / workflow invocations and fetch response invocations are all wired to use the completion hook which seems mostly wrong, that will lead to a lot of duplicate uploads..
There is maybe an edge case for remote hosted agents (SpanKind.CLIENT, e.g. Bedrock Agents / OpenAI Assistants) where the client doesn't have visibility into the underlying LLM inference calls. But not sure if we can differentiate those calls from utils..
There was a problem hiding this comment.
I think this ties into what lmolkova said on #980, that the hook should run wherever we capture messages and only be skipped where there aren't any. Tool spans do capture content when capture is on, the arguments and the result both end up on the span, and that's really what #678 was about, since right now a redaction hook never gets to see them. Now that semantic-conventions-genai #649 is working out which operations should call the hook, and tool calls are one of the cases listed there, I'd rather leave this PR as it is until that's settled and then update it to match. PTAL @DylanRussell @lmolkova
This comment has been minimized.
This comment has been minimized.
lmolkova
left a comment
There was a problem hiding this comment.
Thanks for picking this up! Before we wire tools into the hook, I think we need to decide how the hook works for operations other than inference and agents. I opened open-telemetry/semantic-conventions-genai#649 to settle that in semconv.
The current hook parameters (system instructions, input/output messages, tool definitions) only fit inference and agents. Tool arguments and results, retrieval queries and documents, embedding inputs and memory records don't fit them. Squeezing tool content into ToolCallRequestPart/ToolCallResponsePart messages works, but it isn't what an inference hook expects. And whatever *_ref attribute the hook writes back wouldn't line up with gen_ai.tool.call.arguments/result.
We probably need a more granular hook API, for example per-operation methods or per-attribute upload. Users may also want to upload messages but not tool results. Could we hold this until #649 lands?
|
Sure, I'll hold this until semantic-conventions-genai #649 lands. A per-operation hook would also line up better with tool arguments and results living on gen_ai.tool.call.arguments and gen_ai.tool.call.result, so once #649 settles the API shape I'll rework this to match. I'll mark the PR as a draft in the meantime. |

Fixes #678
ToolInvocation now calls the completion hook when it finishes or fails, the same way inference and workflow invocations already do. The tool arguments are passed as an input message with a ToolCallRequestPart, and the result as an output message with a ToolCallResponsePart, so existing hooks like upload can handle tool content without a new hook method.
Known gaps: if a dedicated hook method for tools is preferred over reusing the message types, I can switch to that. When arguments or result are not set, the hook still runs with empty lists, matching how workflow handles missing content.