Skip to content
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

Do another update call when we're updating an ended trace in update_current_trace #1093

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yorickvP
Copy link

@yorickvP yorickvP commented Jan 29, 2025

It's possible to have observations that 'outlive' their trace, for example when returning a StreamingResponse in FastAPI.

Currently, calling update_current_trace in such cases only worked for the server_merged_attributes. We can work around this by sending the metadata in .trace() instead of storing it in the context.

Failing use case:

from fastapi import router, Response, StreamingResponse

@router.post("/test")
@observe()
async def test() -> Response:
    response_gen = test_inner()
    return StreamingResponse(response_gen, media_type="text/event-stream")

@observe()
async def test_inner() -> AsyncGenerator[str, None]:
    langfuse_context.update_current_trace(
        user_id="unauth"
    )
    yield "test"

Important

Fix update_current_trace to handle updates for ended traces by checking trace_already_ended and sending appropriate update events.

  • Behavior:
    • In update_current_trace, handle cases where traces end before observations by checking trace_already_ended.
    • If trace_already_ended, send an update event for all attributes, not just server_merged_attributes.
    • Avoid updating _observation_params_context if trace_already_ended.

This description was created by Ellipsis for a8f972e. It will automatically update as commits are pushed.

Greptile Summary

Disclaimer: Experimental PR review

Modified the update_current_trace function to handle cases where observations outlive their traces, particularly in FastAPI StreamingResponse scenarios.

  • Added check in langfuse/decorators/langfuse_decorator.py to detect if trace has already ended by checking observation params context
  • Modified behavior to send all parameters directly to server instead of storing in context when trace has ended
  • Fixed issue with metadata and tags not being properly updated for ended traces
  • Added test coverage in tests/test_decorators.py to verify async streaming scenarios where observations continue after trace completion

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

…urrent_trace

It's possible to have observations that 'outlive' their trace, for
example when returning a StreamingResponse in FastAPI.

Currently, calling update_current_trace in such cases only worked
for the server_merged_attributes. We can work around this
by sending the metadata in .trace() instead of storing it in the context.
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@CLAassistant
Copy link

CLAassistant commented Jan 29, 2025

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants