-
-
Notifications
You must be signed in to change notification settings - Fork 517
patch #2735
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
base: master
Are you sure you want to change the base?
patch #2735
Conversation
Thread.current.thread_variable_set(Sentry::ConcurrentRuby::SENTRY_SPAN_KEY, current_span) | ||
Sentry.with_child_span(op: "api.request", description: "Concurrent::Promise") do |child_span| | ||
Sentry.get_current_scope.set_span(child_span) | ||
block.call(*args) |
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.
Bug: Span Propagation Inconsistency
Span context propagation is incomplete. In patch_promises
, current_span
is retrieved from the current scope, ignoring the propagated thread variable. Both patch_futures
and patch_promises
also create a child span but don't restore the original span, which can lead to inconsistent scope state and incorrect span hierarchy.
current_hub = Thread.current.thread_variable_get(Sentry::ConcurrentRuby::SENTRY_THREAD_KEY) || Sentry.get_current_hub | ||
current_span = Sentry.get_current_scope.get_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.
Potential bug: The on_resolution
method retrieves current_hub
from a thread variable but gets current_span
from Sentry.get_current_scope
, which may use a different hub, breaking trace context.
-
Description: In the
on_resolution
method,current_hub
is retrieved from either a thread-local variable (SENTRY_THREAD_KEY
) orSentry.get_current_hub
. However,current_span
is always fetched viaSentry.get_current_scope.get_span
, which internally callsSentry.get_current_hub
. If the thread variable contains a hub different from the current thread's hub (e.g., when a promise chain crosses threads), the span will be associated with the wrong hub. This leads to an incorrect span hierarchy and broken tracing information. The implementation also ignores the stored span in theSENTRY_SPAN_KEY
thread variable. -
Suggested fix: Retrieve the span from the same source as the hub. When
current_hub
is loaded from the thread variable, the corresponding span should also be loaded from its thread variable (SENTRY_SPAN_KEY
) to ensure the hub and span contexts are consistent.
severity: 0.6, confidence: 0.9
Did we get this right? 👍 / 👎 to inform future reviews.
end | ||
|
||
# Install after Sentry loaded | ||
Sentry::ConcurrentRuby.install if defined?(Sentry) |
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.
Potential bug: The integration accesses ::Concurrent::Promises
without checking if the concurrent-ruby
gem is loaded, which can cause a NameError
on application startup.
-
Description: The
Sentry::ConcurrentRuby.install
method is called whenlib/concurrent_ruby.rb
is loaded. This method accesses constants like::Concurrent::Promises
. However, there is no check to ensure theconcurrent-ruby
gem has been loaded first. If a user includes this file without havingconcurrent-ruby
in their Gemfile, the application will crash with aNameError
during initialization. This behavior is inconsistent with other Sentry integrations, which perform adefined?()
check on the dependency's constants before attempting to patch them. -
Suggested fix: Add a check for the presence of the
Concurrent::Promises
constant before callingSentry::ConcurrentRuby.install
, similar to other integrations. For example:Sentry::ConcurrentRuby.install if defined?(Sentry) && defined?(::Concurrent::Promises)
.
severity: 0.8, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Thanks for your Pull Request 🎉
Please keep these instructions in mind so we can review it more efficiently:
feat:
,fix:
,ref:
,meta:
)Other Notes
Description
Describe your changes:
This introduces a custom Sentry integration for the concurrent-ruby library, ensuring proper hub and span propagation across background threads created by Concurrent::Promises.future and Concurrent::Promises::Promise.
Key changes:
Added a Sentry::ConcurrentRuby module that patches:
Concurrent::Promises.future
Concurrent::Promises::Future#on_resolution
Automatically propagates the current Sentry hub and active span to background threads.
Wraps each future/promise execution inside Sentry.with_child_span, creating a child span for async tasks.
Sets the child span as the current span for that thread to preserve the correct trace hierarchy.
Removes the need for manually calling Sentry.with_child_span inside individual futures.
Result: