-
Notifications
You must be signed in to change notification settings - Fork 483
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 ray integration support (#2400) #2444
Conversation
4ff38ec
to
9064e16
Compare
Is it planned to merge this PR? |
@leokster Taking a look now – likely this PR will require some changes before we can merge |
1e32127
to
44e3d24
Compare
The integration includes performance support. Also, add tests for the integration. Closes getsentry#2400
44e3d24
to
a2091db
Compare
@szokeasaurusrex you can do the review, I will adjust accordingly |
Might be interesting to review the PR for Langchain #2911 I am not familiar with Ray, but if it is a framework similar to Langchain, then following a similar model should be ideal cc @colin-sentry if you have any thoughts on this integration |
Hey @glowskir, thanks for reaching out again and for contributing this PR. We are currently working on an initiative to support AI tools in the Sentry SDK, and we would be interested in learning how you are using Ray and what you hope to be able to achieve by integrating Ray with Sentry. If you are interested, we can set up a customer call with you and anyone else interested in seeing a Ray integration in Sentry, so that we can learn how you are using Ray and how a Ray integration in the SDK could help you. You can reach me at daniel.szoke@sentry.io – please let me know when you would have time. |
@szokeasaurusrex This PR introduces RayIntegration to enable out of the box distributed tracing support. Ray has high level task interface and we want to see chain of tasks as single transaction in performance tab. At the moment of creating this PR there was no Opentelemetry support, now in newer versions of ray one can use Opentelemetry facitlities provided by Ray. I don't know how Sentry integrations relate to Opentelemetry hooks tbh, whether you are planning to adapt them or ignore them but this is a thing to consider. |
@glowskir, understood. In that case, we can help to get this PR merged. Before I do an in depth review, I wanted to mention a few things that might be helpful here. First of all, since it looks like Ray is mostly used with AI, it might make sense to design this integration so it is compatible with our LLM Monitoring product. If you need some inspiration for how to do this, you can look at our existing Langchain integration, which works with the LLM Monitoring feature. Additionally, I noticed while looking at your PR that you are using Please let me know whether you have any questions! I am happy to help |
Hi @glowskir , i tried to use your integration with a sample ray app (I have never used ray before) Can you tell me how I can initialize sentry in the ray worker that is started when I call |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #2444 +/- ##
==========================================
- Coverage 79.71% 79.33% -0.38%
==========================================
Files 132 133 +1
Lines 14264 14335 +71
Branches 3003 3013 +10
==========================================
+ Hits 11370 11373 +3
- Misses 2071 2140 +69
+ Partials 823 822 -1
|
The integration when one defines a task as a function. If a task is defined as a class (called an Actor) then the integrations breaks the ray application. I think we can not release it in this state. @glowskir do you have time to look into why Actors do not work with the integration? |
This is the |
I fixed the crash when using Actors. Performance data is still not captured when using Actors but at least we are not crashing users code anymore. I also mention this in the docs, so everyone knows that Actors are not yet supported. |
…nto pr/glowskir/2444
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.
See one comment, else LGTM
Thanks for the work on this ya'll, excited to use this when merged. |
Co-authored-by: Ivana Kellyer <ivana.kellyer@sentry.io>
Adds a basic instrumentation for the Ray framework (https://www.ray.io/)
Closes #2400
Documentation PR: getsentry/sentry-docs#11029