-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Only start tracing worker thread on first span/trace #804
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
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
self._worker_thread = threading.Thread(target=self._run, daemon=True) | ||
self._worker_thread.start() | ||
# We lazily start the background worker thread the first time a span/trace is queued. | ||
self._worker_thread: threading.Thread | None = None |
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.
Why not start a task + asyncio q, then you don't have an extra thread to worry about and you can start the task unconditionally
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.
Hmm I was thinking:
- small overhead in cases where there are no traces (the unconditional task would still be scheduled every poll_interval seconds, even though it doesnt have work)
- It wouldn't work super well in totally sync contexts
- cleaning up the asyncio task is kinda annoying imo
lmk if you feel like I should change, happy to.
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.
No strong opinions
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.
Approving but an async queue might be cleaner
Closes openai#796. Shouldn't start a busy waiting thread if there aren't any traces. Test plan ``` import threading assert threading.active_count() == 1 import agents assert threading.active_count() == 1 ```
Closes #796. Shouldn't start a busy waiting thread if there aren't any traces.
Test plan