-
Notifications
You must be signed in to change notification settings - Fork 7.9k
feat(telemetry): Add content length to tool calls and log tool output truncation event #8014
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
Conversation
Size Change: +3.76 kB (+0.03%) Total Size: 13.2 MB
ℹ️ View Unchanged
|
Code Coverage Summary
CLI Package - Full Text Report
Core Package - Full Text Report
For detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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.
had a chance to test this out in opentelemetry, like producing it to a gcp project?
Yes, I ran 2 evals and in gcp I was able to see the newly added contentLength field and the new tool output truncation event. |
/gemini review |
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.
Code Review
This pull request introduces valuable telemetry enhancements by adding contentLength
to tool call responses and logging a new ToolOutputTruncatedEvent
. The implementation is solid and well-tested across the codebase. I've found one high-severity issue related to inconsistent data serialization in the new Clearcut logger event, which could affect telemetry data analysis. My review includes a specific suggestion to fix this inconsistency.
); | ||
}); | ||
|
||
it('should have undefined contentLength for array llmContent with no string parts', async () => { |
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.
note that you will currently return undefined for llmContent with some string parts and some other parts. is that intended?
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, it's hard to compute length and truncate mixed-type llmContent. Setting it to undefined for now.
TLDR
This pull request enhances our telemetry by adding a
contentLength
field to all tool call responses. It also introduces a new logging event that fires whenever a tool's output is truncated, capturing details about the truncation. This provides better visibility into tool usage, especially for debugging issues related to large outputs.Dive Deeper
The core changes in this PR are:
contentLength
in Tool Responses: TheToolCallResponseInfo
interface now includes an optionalcontentLength
property. This is populated inCoreToolScheduler
for successful, errored, and cancelled tool calls, giving us insight into the size of the data being handled.ToolOutputTruncatedEvent
was created to specifically capture when a tool's output is shortened.logToolOutputTruncated
function has been added to our telemetry loggers, which reports this event to both Clearcut and OTel.CoreToolScheduler
right after thetruncateAndSaveToFile
function successfully truncates and saves the output.Reviewer Test Plan
Run evals with telemetry enabled, logs of tool call should include content length and there will be events for tool output truncation.
Testing Matrix