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

feat!: tracing with concurrency #8489

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

Conversation

LastRemote
Copy link
Contributor

Related Issues

Proposed Changes:

Issue: I ran into some issues after deploying a pipeline as a service with tracing enabled. If there are concurrent calls to run the pipeline, the structure of spans kinda messed up due to the fact that the "haystack.component.run" spans are attached to the "haystack.pipeline.run" span. I experimented primarily with Langfuse, and this gave me a giant span with nested pipeline runs overlapping each other, which is definitely not ideal.

Purposed solution: I have come up with a solution by adding an optional parent_span parameter in the trace context. Furthermore, I tweaked the Langfuse tracer integration such that it creates a new span/generation on top of the parent span if it is provided, or create a root span if not.

How did you test it?

  1. unit test
  2. I also tried running concurrent pipeline runs with Langfuse setup that it seem to work correctly.
Screenshot 2024-10-24 at 12 16 08

Notes for the reviewer

  1. I am not very familiar with datadog/opentelemetry so I am not confident there.
  2. This could be related to async logic and please let me know if you have any better solutions in mind.

Checklist

@LastRemote LastRemote requested review from a team as code owners October 24, 2024 04:16
@LastRemote LastRemote requested review from dfokina and vblagoje and removed request for a team October 24, 2024 04:16
@silvanocerza silvanocerza requested review from silvanocerza and removed request for vblagoje October 24, 2024 07:54
@silvanocerza
Copy link
Contributor

Can you please add a test that reproduces the issue you reported? Also there are some other tests failing, we should fix those too.

Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Just requesting changes to block merging of this PR as I predict it would cause annoying conflicts with #8431.

We'll take care of fixing the conflicts after #8431 is merged.

@LastRemote
Copy link
Contributor Author

LastRemote commented Oct 24, 2024

Can you please add a test that reproduces the issue you reported? Also there are some other tests failing, we should fix those too.

Yes sure. I edited one of the existing unit tests to check for this additional parameter but it is failing now. I am taking a closer look at it now.

Just requesting changes to block merging of this PR as I predict it would cause annoying conflicts with #8431.

We'll take care of fixing the conflicts after #8431 is merged.

I agree. This will surely cause code conflicts with the cycle PR.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 11495743757

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 21 unchanged lines in 4 files lost coverage.
  • Overall coverage remained the same at 90.471%

Files with Coverage Reduction New Missed Lines %
tracing/datadog.py 1 94.59%
tracing/opentelemetry.py 1 97.14%
tracing/tracer.py 5 91.95%
core/pipeline/pipeline.py 14 79.55%
Totals Coverage Status
Change from base Build 11463116725: 0.0%
Covered Lines: 7548
Relevant Lines: 8343

💛 - Coveralls

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

Successfully merging this pull request may close these issues.

[Potential breaking change] Tracing with concurrency
3 participants