-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat: add event logging api and more tracing #2441
Conversation
This partially addresses #2423. Let's add test coverage and fix the code format error. |
existing unit test is in test_logging.py, integration test in test_agent_logging.py. It would be nice to check the schema of the json_state instead of free format |
I went for free format of json to simplify calling site of logging, as long as it can be serialized to json I'm fine with it. In an ideal world we would have full normalized columns in this sqlite, but don't want to deal with that complexity and also don't want to restrict what types of keys and values they can log. Also, this allows same API to be used in multiple places - which is helpful for this 'event' state scenario and less for more formalized concepts like a 'new agent instance'. |
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.
We can log more events in the future PRs.
@henry-zhang-bohan This PR partially address your issue regarding observability #2423 . There are separate PRs like #2329 to use different logging destination. |
Thanks. There is a test requiring openai CI. Could you remake a PR from the upstream repo to run that CI please? Thank you. |
moved to #2478 |
PR moved to: #2478
Related:
#2415
Why are these changes needed?
Related issue number
Checks