feat: add tool usage recording to streaming responses interceptor #123
Conversation
|
|
||
| event: response.completed | ||
| data: {"type":"response.completed","response":{"id":"resp_0c3fb28cfcf463a500695fa2f0239481a095ec6ce3dfe4d458","object":"response","created_at":1767875312,"status":"completed","background":false,"completed_at":1767875312,"error":null,"incomplete_details":null,"instructions":null,"max_output_tokens":null,"max_tool_calls":null,"model":"gpt-4.1-2025-04-14","output":[{"id":"fc_0c3fb28cfcf463a500695fa2f0b0a881a0890103ba88b0628e","type":"function_call","status":"completed","arguments":"{\"a\":3,\"b\":5}","call_id":"call_7VaiUXZYuuuwWwviCrckxq6t","name":"add"}],"parallel_tool_calls":true,"previous_response_id":null,"prompt_cache_key":null,"prompt_cache_retention":null,"reasoning":{"effort":null,"summary":null},"safety_identifier":null,"service_tier":"default","store":true,"temperature":1.0,"text":{"format":{"type":"text"},"verbosity":"medium"},"tool_choice":"auto","tools":[{"type":"function","description":"Add two numbers together.","name":"add","parameters":{"type":"object","properties":{"a":{"type":"number"},"b":{"type":"number"}},"required":["a","b"],"additionalProperties":false},"strict":true}],"top_logprobs":0,"top_p":1.0,"truncation":"disabled","usage":{"input_tokens":58,"input_tokens_details":{"cached_tokens":0},"output_tokens":18,"output_tokens_details":{"reasoning_tokens":0},"total_tokens":76},"user":null,"metadata":{}},"sequence_number":14} | ||
|
|
There was a problem hiding this comment.
nit: expected or missing linter?
There was a problem hiding this comment.
Without this new line completed event is not read by openAI library (stream.Next()/stream.Current()) .
mdn docs mention "Messages in the event stream are separated by a pair of newline characters." so I think without trailing new lines last event is skipped. I can see 2 new lines in what OpenAI API returns.
intercept/responses/streaming.go
Outdated
| responseID = ev.Response.ID | ||
| } | ||
| // capture the final response from the response.completed event | ||
| if ev.Type == "response.completed" { |
|
|
||
| func (i *responsesInterceptionBase) recordToolUsage(ctx context.Context, response *responses.Response) { | ||
| if response == nil { | ||
| i.logger.Warn(ctx, "got empty response, skipping tool usage recording") |
There was a problem hiding this comment.
should we cover this case with tests?
intercept/responses/streaming.go
Outdated
| if responseID == "" && ev.Response.ID != "" { | ||
| responseID = ev.Response.ID | ||
| } | ||
| // capture the final response from the response.completed event |
There was a problem hiding this comment.
Please explain the why in comments, not the what.
There was a problem hiding this comment.
This is also relying on the fact that only the final response will contain tool calls, which is an implicit assumption we should either make very explicit or try to avoid.
| }, | ||
| }, | ||
| { | ||
| name: "streaming_custom_tool", |
There was a problem hiding this comment.
All of these tests should be run for both blocking and streaming.
Adding transport-specific tests is going to lead to holes in coverage.
All that should be different is the fixture and the streaming flag.
In the other implementations the fixtures return the same results to make this easy.
pawbana
force-pushed
the
pb/responses-record-tool-usage-streaming
branch
2 times, most recently
from
8e0756e to
8eff86e
Compare
pawbana
force-pushed
the
pb/responses-record-tool-usage
branch
from
8ecf6e4 to
7bdf03a
Compare
pawbana
force-pushed
the
pb/responses-record-tool-usage-streaming
branch
from
8eff86e to
86a14ee
Compare
pawbana
force-pushed
the
pb/responses-record-tool-usage
branch
from
7bdf03a to
11d3c46
Compare
pawbana
changed the base branch from
pb/responses-record-tool-usage
to
graphite-base/123
pawbana
force-pushed
the
pb/responses-record-tool-usage-streaming
branch
from
86a14ee to
0e6d1a3
Compare
3 participants
No description provided.