-
-
Notifications
You must be signed in to change notification settings - Fork 540
feat: add config.hub_isolation_level for fiber-safe hub storage (Falcon/async) #3018
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
Changes from all commits
0402b8d
545d72a
ab54490
07e1f80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -389,6 +389,23 @@ class Configuration | |
| # @return [Boolean] | ||
| attr_accessor :strict_trace_continuation | ||
|
|
||
| # Which execution primitive owns the SDK's current hub. | ||
| # | ||
| # [+:thread+ (default)] Store the hub in thread-local storage. Correct for | ||
| # thread-based servers (Puma, Unicorn) and background processors (Sidekiq, | ||
| # Resque). Every fiber on a thread shares one hub. | ||
| # [+:fiber+] Store the hub in Fiber Storage (Ruby 3.2+). Each fiber gets its | ||
| # own hub and child fibers inherit it, so concurrent requests on a | ||
| # fiber-based server (Falcon/async) are isolated instead of sharing and | ||
| # corrupting one another's scope. Requested on a Ruby without Fiber | ||
| # Storage (< 3.2), the SDK logs a warning and falls back to +:thread+. | ||
| # | ||
| # @return [Symbol] | ||
| attr_reader :hub_isolation_level | ||
|
|
||
| # Isolation levels the SDK understands for hub storage. | ||
| ISOLATION_LEVELS = %i[thread fiber].freeze | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit and could be a follow-up mini-refactor, but: we have a |
||
|
|
||
| # these are not config options | ||
| # @!visibility private | ||
| attr_reader :errors, :gem_specs | ||
|
|
@@ -541,6 +558,7 @@ def initialize | |
| self.capture_queue_time = true | ||
| self.org_id = nil | ||
| self.strict_trace_continuation = false | ||
| self.hub_isolation_level = :thread | ||
|
|
||
| spotlight_env = ENV["SENTRY_SPOTLIGHT"] | ||
| spotlight_bool = Sentry::Utils::EnvHelper.env_to_bool(spotlight_env, strict: true) | ||
|
|
@@ -610,6 +628,23 @@ def release=(value) | |
| @release = value | ||
| end | ||
|
|
||
| def hub_isolation_level=(level) | ||
| level = level.to_sym if level.respond_to?(:to_sym) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for allowing non-symbols over here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just syntax sugar |
||
|
|
||
| unless ISOLATION_LEVELS.include?(level) | ||
| raise ArgumentError, "hub_isolation_level must be one of #{ISOLATION_LEVELS.inspect}, got #{level.inspect}" | ||
| end | ||
|
|
||
| # :fiber relies on Fiber Storage (Ruby 3.2+); downgrade so the hub is never | ||
| # asked to call Fiber[] on a Ruby that lacks it. | ||
| if level == :fiber && !fiber_storage_available? | ||
| log_warn("hub_isolation_level :fiber requires Ruby 3.2+ Fiber Storage; falling back to :thread on Ruby #{RUBY_VERSION}.") | ||
| level = :thread | ||
| end | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could completely avoid that and define writer method dynamically depending on the availability, which would simplify runtime production code path.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we still want to have the warning message if they try to set it, so this needs to stay |
||
|
|
||
| @hub_isolation_level = level | ||
| end | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| def breadcrumbs_logger=(logger) | ||
| loggers = | ||
| if logger.is_a?(Array) | ||
|
|
@@ -806,6 +841,10 @@ def run_after_close_callbacks | |
|
|
||
| private | ||
|
|
||
| def fiber_storage_available? | ||
| ::Fiber.respond_to?(:[]) && ::Fiber.respond_to?(:[]=) | ||
| end | ||
|
|
||
| def init_dsn(dsn_string) | ||
| return if dsn_string.nil? || dsn_string.empty? | ||
|
|
||
|
|
||
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.
Was this comment inaccurate and that's why it got removed?
Uh oh!
There was an error while loading. Please reload this page.
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.
we should generally not have this clanker generated comment bloat, so I will remove it wherever I see it unless it really adds any value.