-
Notifications
You must be signed in to change notification settings - Fork 223
perf: move serialization to background threads #994
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.
Disclaimer: Experimental PR review
PR Summary
This PR optimizes performance by moving heavy operations like serialization, sampling and masking to background threads in the Langfuse Python SDK.
- Moves Pydantic model serialization from main thread to Consumer threads in
langfuse/task_manager.pyto reduce synchronous processing overhead - Removes detailed kwargs logging from debug events in
langfuse/callback/langchain.pyto minimize logging overhead - Adds filtering of sensitive input/output data from debug logs in
langfuse/client.pyfor better security - Introduces performance test
test_langfuse_overheadto verify tracing overhead remains under 10ms - Defers sampling and masking operations to background threads by moving them from TaskManager to Consumer class
4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
This PR adds proper queue task completion handling in the task manager's _next method. Here's a focused summary of the key changes:
- Added
queue.task_done()call after sampling out events to properly mark filtered tasks as complete - Added
queue.task_done()call after serialization errors to ensure failed tasks don't remain in queue - Added debug logging for item sizes and batch processing in Consumer class
- Added truncation of large items that exceed size limits with proper logging
- Added error handling for serialization failures with detailed logging
The changes focus on improving queue management reliability and preventing potential memory leaks by ensuring all tasks are properly marked as complete, whether they are filtered out, fail serialization, or exceed size limits.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
This PR adds performance testing and improves error handling in the task manager's background processing.
- Added
test_concurrency()intests/test_core_sdk.pyto verify concurrent processing of 100 generations - Added
test_multiple_tasks_without_predecessor()intests/test_task_manager.pyto validate batch processing reliability - Added detailed error handling for serialization failures in
Consumer._next()with proper error logging - Added size limit validation and truncation for oversized items with
MAX_MSG_SIZEandBATCH_SIZE_LIMITconstants
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Based on the latest changes and previous reviews, here's a focused summary of the new updates:
Adds filtering of metadata from debug logs and updates sampling rate access in tests.
- Modified
_filter_io_from_event_bodyinlangfuse/client.pyto also exclude 'metadata' field from debug logs for better verbosity control - Updated sampling rate access pattern in
test_sdk_setup.pyto use direct_sample_rateproperty instead of_sampler.sample_rate - Added test assertions to verify sampling rate configuration through task manager property
- Improved test coverage for debug log filtering with metadata exclusion
The changes are focused on logging improvements and test maintenance, building on the previous performance optimizations without introducing major new functionality.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Based on the latest changes and previous reviews, here's a focused summary of the new updates:
The PR adjusts performance test thresholds and adds validation in test_langchain.py:
- Increased performance test threshold from 10ms to 50ms in
test_langfuse_overhead()to account for background thread processing overhead - Added assertion to verify full execution takes >1 second to validate meaningful overhead measurement
- Test generates large random dictionary input to stress test serialization performance
- Test compares execution times with and without Langfuse tracing to measure actual overhead impact
The changes focus on making the performance tests more realistic and reliable by adjusting thresholds and adding validation, without changing the core test functionality.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Important
Move serialization to background threads, improve logging, and add performance overhead test for Langfuse tracing.
langfuse/task_manager.py.Consumer._next()inlangfuse/task_manager.py._filter_io_from_event_body()to filterinputandoutputfrom logs inlangfuse/client.py.inputandoutputinlangfuse/client.pyandlangfuse/task_manager.py.Consumerclass inlangfuse/task_manager.py.test_langfuse_overhead()intests/test_langchain.pyto measure performance overhead of Langfuse tracing.kwargs_login_log_debug_event()inlangfuse/callback/langchain.py.This description was created by
for 3a22aee. It will automatically update as commits are pushed.