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

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

Closed
1 task
pdany1116 opened this issue Nov 27, 2023 · 1 comment
Labels

Comments

@pdany1116
Copy link

pdany1116 commented Nov 27, 2023

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

  • Replace all Thread.current[] and Thread.current[]= with Thread.current.thread_variable_set and Thread.current.thread_variable_get
@tombruijn
Copy link
Member

tombruijn commented Nov 29, 2023

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.

@tombruijn tombruijn closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants