fix(context): do not modify stack array directly when attach and detach

[tientt-holistics]

As mentioned in open-telemetry/opentelemetry-ruby/pull/1598

As identified in open-telemetry/opentelemetry-ruby-contrib#772, Rails copies all thread-local variables in ActionController::Live. Our use of an array to back the Context stack results in two threads sharing the same stack. Context.clear only clears the array, but continues to share it (in fact, its effect is visible to both threads), so there is no way to cleanly separate the thread's Context stacks using the public API.

This PR will replace the stack.push and stack.pop with Thread.current assignment

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: tientt-holistics / name: tien.tt-holistics (4058a91)

[kaylareopelle]

Hi @tientt-holistics, thank you for your contribution! It looks like there's a few failures related to Rubocop in the CI: https://github.com/open-telemetry/opentelemetry-ruby/actions/runs/11834674510/job/33062312585?pr=1760#step:6:267

Could you take a look?

[tientt-holistics]

Hi @kaylareopelle, I just fixed the rubocop warning. But I don't know how to trigger the workflow again. Could you mind help me do that? Thank you

[kaylareopelle]

Hi @tientt-holistics, thanks for the fix! Since this is your first PR in this repository, a maintainer or an approve will need to approve your CI runs. I just kicked them off again. If you're ever stuck waiting after a commit, post a comment and someone should be able to approve the runs soon after.

[arielvalentin]

I have added some details to an issue for reviewers: #1766

My concern here is that switching to using Immutable Arrays results in additional object allocations per attach and detach. I am not sure how much of an impact that will be, I suppose Depth of the Tree(N*2)?

[fbogsany]

This approach requires additional allocations, as @arielvalentin noted. I implemented an alternative with a linked list back in February: #1597. I rejected that due to performance concerns, however when I repeat the benchmarks with this PR, the immutable array approach is consistently worse:

bogsanyi@MacBook-Pro-2 benchmarks % bundle exec ruby --yjit otel_context.rb                         
Allocations -------------------------------------
linked list with_value
                           4/0  alloc/ret        0/0  strings/ret
    array with_value       4/1  alloc/ret        0/0  strings/ret
immutable array with_value
                           7/1  alloc/ret        0/0  strings/ret
Warming up --------------------------------------
linked list with_value
                       294.073k i/100ms
    array with_value   339.589k i/100ms
immutable array with_value
                       280.528k i/100ms
Calculating -------------------------------------
linked list with_value
                          3.964M (± 1.0%) i/s -     19.997M
    array with_value      4.800M (± 0.9%) i/s -     24.111M
immutable array with_value
                          3.780M (± 0.9%) i/s -     19.076M

Comparison:
    array with_value:  4800498.2 i/s
linked list with_value:  3963968.1 i/s - 1.21x slower
immutable array with_value:  3780407.9 i/s - 1.27x slower

Allocations -------------------------------------
linked list with_values
                           3/0  alloc/ret        0/0  strings/ret
   array with_values       2/0  alloc/ret        0/0  strings/ret
immutable array with_values
                           5/1  alloc/ret        0/0  strings/ret
Warming up --------------------------------------
linked list with_values
                       315.094k i/100ms
   array with_values   366.713k i/100ms
immutable array with_values
                       301.321k i/100ms
Calculating -------------------------------------
linked list with_values
                          4.308M (± 0.8%) i/s -     21.741M
   array with_values      5.403M (± 4.1%) i/s -     27.137M
immutable array with_values
                          4.063M (± 2.7%) i/s -     20.490M

Comparison:
   array with_values:  5402826.2 i/s
linked list with_values:  4308029.2 i/s - 1.25x slower
immutable array with_values:  4063300.0 i/s - 1.33x slower

Allocations -------------------------------------
linked list with_value recursive
                          30/0  alloc/ret        0/0  strings/ret
array with_value recursive
                          20/0  alloc/ret        0/0  strings/ret
immutable array with_value recursive
                          50/1  alloc/ret        0/0  strings/ret
Warming up --------------------------------------
linked list with_value recursive
                        36.262k i/100ms
array with_value recursive
                        44.950k i/100ms
immutable array with_value recursive
                        31.701k i/100ms
Calculating -------------------------------------
linked list with_value recursive
                        379.819k (± 5.3%) i/s -      1.922M
array with_value recursive
                        475.577k (± 0.7%) i/s -      2.382M
immutable array with_value recursive
                        330.950k (± 1.2%) i/s -      1.680M

Comparison:
array with_value recursive:   475576.7 i/s
linked list with_value recursive:   379819.5 i/s - 1.25x slower
immutable array with_value recursive:   330949.6 i/s - 1.44x slower

[fbogsany]

An alternative using the existing array code with a Fiber attribute, like ActiveSupport::IsolatedExecutionState, instead of a Fiber-local variable is slower than the Fiber-local version, but faster than the linked list or immutable array versions.

Comparison:
array with_value recursive:   505508.1 i/s
fiber attribute with_value recursive:   418918.9 i/s - 1.21x slower
linked list with_value recursive:   397144.2 i/s - 1.27x slower
immutable array with_value recursive:   337211.0 i/s - 1.50x slower

[fbogsany]

Another option, if we're willing to take a dependency on concurrent-ruby is Concurrent::FiberLocalVar. A mutable array version of Context using FiberLocalVar yields the highest performance of a thread/fiber-safe implementation:

Comparison:
array with_value recursive:   499772.9 i/s
fiber local var with_value recursive:   463638.7 i/s - 1.08x slower
fiber attribute with_value recursive:   415219.4 i/s - 1.20x slower
linked list with_value recursive:   389115.4 i/s - 1.28x slower
immutable array with_value recursive:   344883.7 i/s - 1.45x slower

[fbogsany]

Another option, if we're willing to take a dependency on concurrent-ruby is Concurrent::FiberLocalVar.

@arielvalentin pointed out that this uses fiber-local variables in its implementation, so it suffers from the same problem of other gems breaking encapsulation and copying the contents of the Fiber local entries.

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[gjtorikian]

Definitely still an issue, robot.