-
Notifications
You must be signed in to change notification settings - Fork 2
Add sleep to OTEL exporters #51
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
src/humanloop/otel/exporter.py
Outdated
| block=False, | ||
| ) # type: ignore | ||
| except EmptyQueue: | ||
| # Wait for the another span to arrive |
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.
fix typo
src/humanloop/otel/exporter.py
Outdated
| ) # type: ignore | ||
| except EmptyQueue: | ||
| # Wait for the another span to arrive | ||
| time.sleep(0.1) |
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.
worth making it slightly longer? don't totally understand in what context this code is being used
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.
Yeah it's fine to make it longer, too. There's a task in the queue whenever a span should be exported which approximately means every generation/evaluation/call. In an evaluations.run setting with up to 20 workers that could happen quite a few times a second, but would probably be spiky given they're usually dependent on provider calls.
(exporter has 4 workers by default I think)
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.
claude seems to give a range of 100-500ms as suitable generally when there's no need for realtime
• Real-time systems: 10-50ms
• Interactive applications: 50-100ms
• Background processing: 100-500ms
• Batch processing: 500ms+
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.
mmk, leaving as is sounds reasonable in that case
|
Can you merge in the release branch? |
Infinite polling the export queue without a sleep was causing threads to take up a full core of CPU. Simple sleep fixes that.
99a6979 to
ee90d95
Compare
Infinite polling the export queue without a sleep was causing threads to take up a full core of CPU. Simple sleep fixes that.