-
Notifications
You must be signed in to change notification settings - Fork 4
Enhance Langfuse Integration: Add /responses Support, Fix /threads Context, and Decouple Dependencies #248
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: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
session_id: Optional[str] = None, | ||
response_id: Optional[str] = None, | ||
): | ||
self.session_id = session_id or str(uuid.uuid4()) |
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.
isn't thread id used as session id, is it something that we can create as well?
if response_id: | ||
traces = self.langfuse.fetch_traces(tags=response_id).data | ||
if traces: | ||
self.session_id = traces[0].session_id |
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.
Just to clarify, is this because since all the traces will belong under one session so it wont matter if we retrieve session id from 1st trace or lets say from 11th trace?
|
||
def update_trace(self, tags: list[str], output: Dict[str, Any]): | ||
if self.trace: | ||
self.trace.update(tags=tags, output=output) |
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.
what are tags here
) | ||
|
||
def flush(self): | ||
self.langfuse.flush() |
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.
As stated here that langfuse.flush() is blocking so this holds a tiny possibility of causing a problem - check here
provider="langfuse", | ||
project_id=request.project_id, | ||
) | ||
tracer = LangfuseTracer( |
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.
for the case where it does not get langfuse credential, what happens. Same question for threads endpoint as well
Have you tested a case where you use threads and/or response endpoint for a user with no langfuse creds given, directly after you run these endpoint(s) for a user with langfuse creds given |
Summary
Closes #210: Removes hard dependency on Langfuse for /threads and /response, allowing use with only OpenAI credentials.
Fixes #249: Eliminates race conditions caused by shared langfuse_context across async requests and background jobs.
Closes #244: Brings tracing to /responses, ensuring consistent Langfuse session grouping.
Key Changes:
Grouping of traces in Response API
To group traces by conversation:
Checklist
fastapi run --reload app/main.py
ordocker compose up
in the repository root and test.Notes
Please add here if any other information is required for the reviewer.