-
Notifications
You must be signed in to change notification settings - Fork 164
feat: Unify traces of sub-pipelines within pipelines with Langfuse #1624
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
assert langfuse_connector.public_key == Secret.from_env_var("LANGFUSE_PUBLIC_KEY") | ||
assert isinstance(langfuse_connector.span_handler, CustomSpanHandler) | ||
|
||
def test_pipeline_serialization(self, monkeypatch): |
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.
This was moved from the test_tracing.py
file
@@ -1,71 +0,0 @@ | |||
# SPDX-FileCopyrightText: 2023-present deepset GmbH <[email protected]> |
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.
This file got refactored into the test_tracer.py
file, following our standard practice of having the test files follow the same name structure as our module files.
@pytest.mark.integration | ||
def test_custom_span_handler(): | ||
"""Test that custom span handler properly sets Langfuse levels.""" | ||
def test_tracing_with_sub_pipelines(): |
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.
This is the new test, that checks that when using sub-pipelines within a pipeline we correctly collect the traces together.
tags=tags, | ||
# We use the current active span as the parent span if not provided to handle nested pipelines | ||
# The nested pipeline (or sub-pipeline) will be a child of the current active span | ||
parent_span=parent_span or self.current_span(), |
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.
This line is what fixed the issue
pinging @vblagoje in case you are interested |
Very nice @sjrl |
new_pipe = Pipeline.from_dict(serialized) | ||
|
||
# Verify pipeline is the same | ||
assert new_pipe == pipe |
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.
Great to see that you've introduced multiple test cases. Maybe in future we can cover some edge casestest_invalid_span_handler
to verify how the system behaves. But its your call.
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.
Good point! I think that is something we can do in the future.
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.
Great work! Although I may not have full context on Langfuse, I reviewed the code based on my understanding. I am guessing @vblagoje has also had a chance to take a look.
Related Issues
AsyncPipeline
Creates Multiple Traces Instead of a Single Unified Trace in Langfuse #1604 and Support Unified Tracing withLangfuseConnector
Across Main and Sub-Pipelines in Haystack #1605Proposed Changes:
How did you test it?
Notes for the reviewer
Here is some example code
which produces this trace in Langfuse
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.