appsignal / appsignal-ruby

🟥 AppSignal for Ruby gem
https://www.appsignal.com/ruby
MIT License
183 stars 116 forks source link

Use thread-local variables instead of fiber-local variables to store global configuration #1013

Closed pdany1116 closed 10 months ago

pdany1116 commented 10 months ago

Currently, Appsignal is using fiber-local variables to store the current appsignal transaction and some information about logger.

When opening a new Fiber, you will not have access to the Appsignal current transaction inside the new fiber since the transaction it's stored in a fiber-local variable and they are not shared between fibers. (https://docs.ruby-lang.org/en/3.2/Thread.html#method-i-5B-5D) See below example:

require 'appsignal'

Appsignal::Transaction.create(
  SecureRandom.uuid,
  Appsignal::Transaction::HTTP_REQUEST,
  {}
)

pp Appsignal::Transaction.current # => Appsignal::Transaction:0x00007f865a78f2c0

Fiber.new do
  pp Appsignal::Transaction.current # => Appsignal::Transaction::NilTransaction:0x00007f865a5b96f8
end.resume

My suggestion is to make the Appsignal transaction available as a thread local variable, instead of using Thread.current[:appsignal_transaction] to use Thread.current.thread_variable_set(:appsignal_transaction, ...) and Thread.current.thread_variable_get(:appsignal_transaction)`.

I have encountered this issue using dry-effects library. Please see this issue.

I am not sure if there are any other concerns, but I can open up a PR with the changes if green light.

Thank you!

TODO

tombruijn commented 10 months ago

Hi @pdany1116,

Thanks for the suggestion!

With the way our transaction API works, it doesn't support async events. Having fibers concurrently instrumented will result in a broken event timeline. Best way to go about it right now is to create a new transaction in a fiber. We'll update the Ruby gem's API in the future, to support concurrent events better.