-
Notifications
You must be signed in to change notification settings - Fork 173
Fix/tui aggregated token cost #751
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
c3a0214 to
a15dc5c
Compare
|
@krissetto — kindly take a look at this PR when you have a moment. I’d really appreciate your feedback! I closed the previous PR since the approach there turned out to be overly complex and didn’t rebase cleanly with the updated main branch. In this version, I’ve simplified the implementation by introducing dedicated fields for each session to track separate self and child token counts, which are then aggregated for display. I’ve tested it with: A root agent having multiple subagents. Subagents that themselves have nested subagents. There’s also a temporary logging system included to help with verification — I’ll remove it once the changes are approved for merge. |
|
For logging you can simply use |
pkg/tui/page/chat/chat.go
Outdated
| return p, cmd | ||
| case *runtime.TokenUsageEvent: | ||
| p.sidebar.SetTokenUsage(msg.Usage) | ||
| p.sidebar.SetTokenUsage(msg) // forward full event so sidebar can track per-session usage |
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.
Please remove this log
Hi @rumpl @krissetto , removed the logging. Can you please verify the solution and if some changes need to be done? |
0378171 to
1d5ad81
Compare
krissetto
left a comment
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.
Thanks for working on this @riturajFi! I left a bunch of little comments, mostly nitpicks though.
One thing, i think we could streamline the sidebar UI when no subagents are configured in a team.
Also, have a look here for best practices when it comes to documenting go code, for the most part we follow those recommendations when leaving comments etc
| } | ||
| builder.WriteString(fmt.Sprintf("\n Tokens: %s | Cost: $%.2f\n", formatTokenCount(totalTokens), totals.Cost)) // display totals line | ||
| builder.WriteString("--------------------------------\n") // visual separator | ||
| builder.WriteString(styles.SubtleStyle.Render("SESSION BREAKDOWN")) // heading for per-session details |
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.
nit: Let's use Agent breakdown (not all caps is a bit nicer imho) instead as its more representative. each time an agent delegates to a subagent, a new session is created. This can happen more than once
| // var usagePercent float64 | ||
| // if totals.ContextLimit > 0 { | ||
| // usagePercent = (float64(totals.ContextLength) / float64(totals.ContextLimit)) * 100 | ||
| // } | ||
| // percentageText := styles.MutedStyle.Render(fmt.Sprintf("%.0f%%", usagePercent)) |
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.
nit: this commented out code can be removed when dealing with multiple agents, but if there's only one it might be good to keep the percentage displayed
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.
on it @krissetto
d21981f to
68c2716
Compare
|
@krissetto I fixed the issues raised by you. Kindly review |
|
Hi @rumpl @krissetto @dgageot Would love your review of this PR! Exited to move to the next issue in the core codebase |
|
Hey @riturajFi i'm testing this out again. Could you please fix the linter errors so the CI is green? There is some unused code and some formatting issues. Also needs a rebase, sorry for that. |
|
Hi @krissetto i shall fix everything in the next 2 hours or so. Currently dont have access to my computer 🙃🙃 |
|
@riturajFi not a problem, i'll leave some comments as i test things out in the meantime. Also, could you squash the commits down when you get a chance? |
|
@riturajFi First of all, I love the enthusiasm you have in wanting to contribute to this project ❤️ To try and guide you in the right direction to get this feature merged, i made a quick example of what i was thinking about that you can find here: https://github.com/krissetto/cagent/tree/tui-usage It would be ideal if we could get this feature in with minimal changes to the actual runtime, focusing instead on the TUI side of things. Smaller changes are easier to review for us and will be faster to merge, then if there are followup issues we can deal with those separately 😄 Please have a look at the ideas in my branch (compare them directly to main), give it a try locally, and feel free to steal the code and bring it in here or open a separate PR if it makes things easier for you Let me know what you think! |
|
Oh...i thought you guys wanted to solve it in the backend too as any future consumer would directly have token count access.😅 if just handling things in the frontend is fine then i shall do it right away 🫡 |
|
@riturajFi go for it! frontend only is more than enough right now, a consumer can always access the data from the sessions |
|
Thanks @krissetto for the greenlight. I shall make the changes in some hours. Kindly find time to take a look. |
acf035f to
759f561
Compare
|
Hi @krissetto changed the entire logic to the frontend. The changes - basically went through your solution and implemented it here. Thanks for your help! |
759f561 to
1645e6c
Compare
Multi-session token tracking in runtime + sidebar
Overview
fixes #504
Expands runtime token-usage events to include session IDs and both self vs. inclusive snapshots, allowing downstream consumers to correlate root and child sessions.
The runtime now:
The sidebar is modernized to:
Implementation Highlights
pkg/runtime/event.go, pkg/session/session.go
Introduce richer
TokenUsageEventpayloads and per-sessionselffields, distinguishing the emitting agent’s contribution from inherited child totals.pkg/runtime/runtime.go
Calculates self vs. inclusive snapshots, merges child session usage, logs each provider chunk to
token_usage_chunks.log, and re-emitsTokenUsageevents whenever compaction or transfers change totals.pkg/tui/components/sidebar/sidebar.go
Replaces the single
Usagepointer with ausageStatemap keyed by session ID. Formats totals usinghumanize.Comma, and renders a “SESSION BREAKDOWN” section showing root-exclusive usage plus every child session (sorted, with the active session highlighted).pkg/tui/page/chat/chat.go
Passes the full
TokenUsageEventto the sidebar.pkg/runtime/runtime_test.go
Adds snapshot helpers so expectations include the richer event payloads.
Testing
task testScreenshots / Recordings