-
Notifications
You must be signed in to change notification settings - Fork 548
feat(logging): improve event logging and add bot thinking display #1434
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: feat/bot-thinking-events
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Looks good, just a few small comments
bot_thinking_logs = [ | ||
record | ||
for record in caplog.records | ||
if "Event :: BotThinking" in record.message |
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: Should we define an enum with these strings if there are a static set of events? It would be more robust to typos
for event in next_events: | ||
if event["type"] != "EventHistoryUpdate": | ||
event_type = event["type"] | ||
log.info( |
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: Should this be a log.debug() rather than log.info()? How much noise would this make in logs?
if event_name == "BotThinking": | ||
title = f"[blue]{title}[/] [yellow bold]{event_name}[/]" | ||
elif event_name.startswith("Start") and event_name.endswith("Action"): | ||
title = f"[magenta][bold]Start[/]{event_name[5:]}[/]" |
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.
Where does the magic number 5 come from here?
elif event_name.startswith("Start") and event_name.endswith("Action"): | ||
title = f"[magenta][bold]Start[/]{event_name[5:]}[/]" | ||
elif event_name.endswith("ActionFinished"): | ||
title = f"[magenta]{event_name[:-8]}[bold]Finished[/][/]" |
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.
Another magic number here, is there a danger we add longer-named events and this breaks?
Stack Info
This PR is part of a stack:
#1427 → #1431 → #1432
Description
Enhanced event logging in the runtime and callbacks to better display bot thinking events with reasoning traces. Moved event logging to occur after event generation for proper ordering, added reasoning content extraction from LLM completions, and improved console output formatting for different event types.
Changes