-
Notifications
You must be signed in to change notification settings - Fork 135
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
feat: Add custom Langfuse span handling support #1313
Conversation
@alex-stoica @ggdupont @lc-bo @LastRemote would this approach fit all of your Langfuse customization needs? |
@vblagoje I have to think deeper, but at the first glance, my answer is "partially", i.e.
So, I'd say it's a solid and elegant step in the right direction, but not the full solution. |
@alex-stoica thanks for the feedback! What you describe seems a bit orthogonal to this issue, no? Here we enable devs to customize these spans as they see fit - that's all, good first step. This prompt injection seems something different - and that's ok. |
@vblagoje
With this new modification, step 1 is resolved (not required anymore). In terms of this specific issue, the codebase moves closer to being open/closed (SOLID principles), I now only need to extend functionality (e.g., create a custom span, prompt builder, or generator) without modifying existing code. As mentioned earlier, this is a good step forward. |
@alex-stoica thanks a lot for the feedback. I added a small change to encapsulate all the parameters for span creation into SpanContext. SpanHandler.create_span had just too many parameters and looked fragile in terms of future maintenance. I'll keep this PR in draft state for this week for you and others to try it out and kick the tires - next week we can move to PR integration if there are no major changes required. 🙏 |
Thanks for the ping. Unfortunately it does not help with my current problems with Langfuse (which are unrelated to this PR), but I do think this is a more elegant approach and a good step forward. To be clear, I already implemented a slightly modified version of LangfuseTracer that is able to register new generators on the fly (this part can be done in a better way by using Things that trouble me right now, for a more transparent feedback:
|
Totally agree with that. Also, the trend is towards more "LRM"s, whatever they are |
For reference I just created a new issue for LRMs: deepset-ai/haystack#8760 |
Thanks for the feedback @alex-stoica and @LastRemote. The PR is now opened and will go into review next week, with a new langfuse integration released shortly after - if all goes as planned. If you see anything else that can be adjusted now - let us know. |
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.
I've taken a look and in principle I can't find anything wrong with the implementation....
I think we should mention this development in our docs. Can you take care of that?
Since I am not an expert on this part, another pair of eyes might be helpful (I would propose involving @mpangrazzi).
@@ -50,7 +52,7 @@ class LangfuseSpan(Span): | |||
Internal class representing a bridge between the Haystack span tracing API and Langfuse. | |||
""" | |||
|
|||
def __init__(self, span: "Union[langfuse.client.StatefulSpanClient, langfuse.client.StatefulTraceClient]") -> None: | |||
def __init__(self, span: "langfuse.client.StatefulClient") -> 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.
could you please explain this change?
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.
Thanks for asking @anakin87 . It is a mistake! StatefulClient is a superclass of all these spans and one can create other spans from StatefulClient superclass API using generation/span methods but we need to keep the previous version because we also call update on these spans which is only declared in subclasses: StatefulSpanClient, StatefulTraceClient, and StatefulGenerationClient. I guess IDE/linters didn't pick these up because of forward references. I'll make corrections. 🙏
I am also not a Langfuse expert too, but after looking at the why and the changes, I would say the look good to me. I agree with @anakin87, we should definitely mention this additional feature in the docs. |
@dfokina I updated https://docs.haystack.deepset.ai/docs/langfuseconnector but can't somehow get the formatting right for the code snippet. It seems like Python option is gone from readme. Please investigate. @anakin87 please review a743f15 pydoc additions. Daria and I will fix the doc to be just right. |
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.
Feel free to merge once tests are green
@anakin87 this change deserve a jump to 0.8.0 in https://pypi.org/project/langfuse-haystack/#history , thumbs up/down? |
@dfokina I also noticed that we should perhaps, in our LangfuseConnector doc, describe an additional init param:
As this parameter has also been added recently. It is a small change to docs:
Perhaps you can mention both |
Fixed and included in https://pypi.org/project/langfuse-haystack/0.8.0/ thank you all @alex-stoica @LastRemote @lc-bo @marcklingen @davidsbatista @anakin87 @mpangrazzi @dfokina |
Sorry for the delay @vblagoje. I fixed the code snippet and edited the text a bit. Will add that new param in a moment, too. |
Why:
The main motivation behind this change is to enable developers to easily customize Langfuse traces without having to rewrite the entire
LangfuseConnector
. By introducing theSpanHandler
interface and its default implementation,DefaultSpanHandler
, developers can now extendSpanHandler
or, even better, override thehandle
method inDefaultSpanHandler
to tailor trace processing to their needs. This simplifies the process of customizing what data is logged to Langfuse, providing a more flexible and developer-friendly integration.Enables developers to add further customization to spans as noted in these issues:
trace_id
in the response #1263What:
SpanHandler
class and its default implementationDefaultSpanHandler
for customizing span creation and processing.SpanContext
dataclass.LangfuseConnector
andLangfuseTracer
to support custom span handlers.How can it be used:
Developers can utilize custom span handlers by providing their implementations that extend the
SpanHandler
class:How did you test it:
Tests were conducted using the
QualityCheckSpanHandler
to ensure spanWARNING
levels are set correctly. Different response scenarios were simulated to check the handler's response, and the Langfuse API was polled to verify the output.Notes for the reviewer:
Pay attention to the
SpanHandler
's implementation to ensure it meets necessary specifications and seamlessly integrates with the existingLangfuseTracer
. Additionally, confirm that the new test scenarios cover diverse usage cases adequately.