-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix token usage tracking for streaming providers #3909
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
base: main
Are you sure you want to change the base?
Conversation
29d5be6
to
0ac0658
Compare
Signed-off-by: jajimajp <soichi-yajima@c-fo.com>
0ac0658
to
a38ea0b
Compare
thanks @jajimajp - what is odd, I use anthropic/databricks and didn't see it show zero, can you confirm what provider you were using and model etc? |
I am using an OPENAI-compatible proxy with the model sonnet-4. Therefore, the provider is OPENAI. This change worked well in my environment, but let me verify again whether the same issue occurs with the regular OPENAI API. |
also if you can run cargo fmt and clippy etc. |
I hand tested this with databricks, openai direct, and anthropic, seems ok, but @katzdave sorry to hassle again - but you ok with this? I am just not sure how it works. I am wary that it works without with openai direct, but with a proxy, was different (sounds like 3rd party proxy may be returning incorrect values?) |
Can we just add a test that verifies it does the the same thing for the non-streaming case. Generally makes sense to me though on the streaming side. I can't remember my exact setup but I was seeing some kind of 0 issue a couple weeks ago; I think with openai on databricks. |
@jajimajp able to add any tests? |
Thanks for the feedback. I've added a test case to verify the non-streaming behavior as requested. Additionally, I manually verified that non-streaming works correctly with my OpenAI provider by temporarily making the following change: --- a/crates/goose/src/providers/openai.rs
+++ b/crates/goose/src/providers/openai.rs
@@ -206,7 +206,7 @@ impl Provider for OpenAiProvider {
}
fn supports_streaming(&self) -> bool {
- true
+ false
}
async fn stream( It seems to be working correctly. Let me know if there's anything else. |
0cb1c06
to
c5bb6bb
Compare
force pushed to sign previous commits |
c5bb6bb
to
8c8caac
Compare
Still in progress? |
No, I am waiting for comments on this: #3909 (comment) |
Problem
When using streaming providers with the goose CLI, the context token count remains at 0 and doesn't update properly:
Root Cause
The current implementation overwrites usage metrics with each streaming event instead of properly accumulating them. When streaming APIs send a final event with None values, it resets the usage count to 0.
Solution
Modified the update_session_metrics method to: