Fix for compatibility issue with Async 2.6.2 #2083
Conversation
tannalynn
requested review from
fallwith,
hannahramadan and
kaylareopelle
as code owners
| def test_thread_instrumentation_args_preserved | ||
| Thread.new('arg 1', 2) do |arg1, arg2| | ||
| assert_equal ['arg 1', 2], [arg1, arg2] | ||
| end.join | ||
| end | ||
|
|
||
| def test_fiber_instrumentation_args_preserved | ||
| Fiber.new do |arg1, arg2| | ||
| assert_equal ['arg 1', 2], [arg1, arg2] | ||
| end.resume('arg 1', 2) | ||
| end |
There was a problem hiding this comment.
Is the agent running during these test cases?
There was a problem hiding this comment.
Yes, the other tests in the files are making sure the agent is recording stuff and nesting spans correctly, so the agent is running and the instrumentation is installed. I didn't add a transaction to these tests since I didn't need to assert anything about a transaction and only cared about making sure the instrumentation was passing the args properly.
| traced_block = add_thread_tracing(*args, &block) | ||
| initialize_with_newrelic_tracing { initialize_without_new_relic(*args, &traced_block) } | ||
| if RUBY_VERSION < '2.7.0' | ||
| def initialize(*args, &block) |
There was a problem hiding this comment.
I think *args should be *_args here to convey that we don't make use of it.
There was a problem hiding this comment.
oh right! you mentioned that earlier and I totally blanked on it, my bad. thanks for the reminder.
updated in ec8a03b
| @@ -4,4 +4,6 @@ | |||
|
|
|||
| instrumentation_methods :chain, :prepend | |||
|
|
|||
| gemfile nil | |||
| gemfile <<~RB | |||
| # this can't be empty or the tests won't run | |||
There was a problem hiding this comment.
Ooo good to know. Were they not running before?
There was a problem hiding this comment.
apparently not! I know they ran at some point in the past, because they did when i was working on them, but I think i ended up switching to passing nil to gemfile as an afterthought before merging in that PR iirc, because we thought that would work fine, so that would have have broken it.
SimpleCov Report
|
|
@tannalynn can y'all get this out in a patch release soon? this is preventing us from using Async. :) |
|
This fix will be included in 9.3.0, which we plan on releasing next week. I'll let you know when it's available once it's been published. Thank you! |
|
@isaacbowen 9.3.0, which includes this fix, has been released! You should be able to use it now, please let me know if you have any other issues. |
The issue was that we were capturing
*argsand passing them on for our Fiber instrumentation. Actually though, you can't actually pass arguments toFiber.newthe same way you can as withThread.new(you would accomplish it likefiber.resume(args)). This wasn't causing any issues on it's own, however Async released a new version with fiber-annotation, that defines the Fiber initialize method as accepting only **kwargs, and not *args.lib/fiber/annotation.rbIn older Fiber versions, it seems there are no params you can pass to
Fiber.new, but in newer ruby versions, there are keyword arguments that it accepts. Since we still support older ruby versions, we will define our fiber instrumentation initialize method differently depending on ruby version. This will allow our instrumentation to work on all supported ruby versions, and will not cause errors when using Async 2.6.2.closes #2062