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

Fix for compatibility issue with Async 2.6.2 #2083

Merged
merged 8 commits into from Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Expand Up @@ -2,7 +2,7 @@

## dev

Version <dev> of the agent adds the ability to filter logs by level, expands instrumentation for Action Cable,provides bugfix for Code-Level Metrics, and a bugfix for `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` being incorrectly private.
Version <dev> of the agent adds the ability to filter logs by level, expands instrumentation for Action Cable,provides bugfix for Code-Level Metrics, a bugfix for how `Fiber` args are treated, and a bugfix for `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` being incorrectly private.

- **Feature: Filter forwarded logs based on level**

Expand All @@ -28,6 +28,10 @@ Version <dev> of the agent adds the ability to filter logs by level, expands ins

As part of a refactor included in a previous release of the agent, the method `NewRelic::Agent::Logging::DecoratingFormatter#clear_tags!` was incorrectly made private. This method is now public again. Thanks to [@dark-panda](https://github.com/dark-panda) for reporting this issue. [PR#](https://github.com/newrelic/newrelic-ruby-agent/pull/2078)

- **Bugfix: Fix the way args are handled for Fibers**

Previously, the agent treated Fiber args the same as it was treating Thread args, which is not correct. Args are passed to `Fiber#resume`, and not `Fiber.new`. This has been fixed, and the agent will properly preserve args for both Fiber and Thread classes. This also caused an error to occur when using Async 2.6.2, due to mismatching initalize definitions for Fiber prepended modules. This has been fixed as well. Thanks to [@travisbell](https://github.com/travisbell) for bringing this to our attention. [PR#2083](https://github.com/newrelic/newrelic-ruby-agent/pull/2083)

## v9.2.2

Version 9.2.2 of the agent fixes a bug with the `Transaction#finished?` method.
Expand Down
Expand Up @@ -13,7 +13,7 @@ def self.instrument!
def post(*args, &task)
return post_without_new_relic(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled?

traced_task = add_task_tracing(*args, &task)
traced_task = add_task_tracing(&task)
post_without_new_relic(*args, &traced_task)
end
end
Expand Down
Expand Up @@ -7,11 +7,10 @@ module ConcurrentRuby
SEGMENT_NAME = 'Concurrent/Task'
SUPPORTABILITY_METRIC = 'Supportability/ConcurrentRuby/Invoked'

def add_task_tracing(*args, &task)
def add_task_tracing(&task)
NewRelic::Agent.record_metric_once(SUPPORTABILITY_METRIC)

NewRelic::Agent::Tracer.thread_block_with_current_transaction(
*args,
segment_name: SEGMENT_NAME,
parent: NewRelic::Agent::Tracer.current_segment,
&task
Expand Down
Expand Up @@ -10,7 +10,7 @@ module Prepend
def post(*args, &task)
return super(*args, &task) unless NewRelic::Agent::Tracer.tracing_enabled?

traced_task = add_task_tracing(*args, &task)
traced_task = add_task_tracing(&task)
super(*args, &traced_task)
end
end
Expand Down
13 changes: 10 additions & 3 deletions lib/new_relic/agent/instrumentation/fiber/chain.rb
Expand Up @@ -10,9 +10,16 @@ def self.instrument!

alias_method(:initialize_without_new_relic, :initialize)

def initialize(*args, &block)
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)
traced_block = add_thread_tracing(&block)
initialize_with_newrelic_tracing { initialize_without_new_relic(&traced_block) }
end
else
def initialize(**kwargs, &block)
traced_block = add_thread_tracing(&block)
initialize_with_newrelic_tracing { initialize_without_new_relic(**kwargs, &traced_block) }
end
end
end
end
Expand Down
Expand Up @@ -11,11 +11,10 @@ def initialize_with_newrelic_tracing
yield
end

def add_thread_tracing(*args, &block)
def add_thread_tracing(&block)
return block if !NewRelic::Agent::Tracer.thread_tracing_enabled?

NewRelic::Agent::Tracer.thread_block_with_current_transaction(
*args,
segment_name: 'Ruby/Fiber',
&block
)
Expand Down
13 changes: 10 additions & 3 deletions lib/new_relic/agent/instrumentation/fiber/prepend.rb
Expand Up @@ -9,9 +9,16 @@ module MonitoredFiber
module Prepend
include NewRelic::Agent::Instrumentation::MonitoredFiber

def initialize(*args, &block)
traced_block = add_thread_tracing(*args, &block)
initialize_with_newrelic_tracing { super(*args, &traced_block) }
if RUBY_VERSION < '2.7.0'
def initialize(*_args, &block)
traced_block = add_thread_tracing(&block)
initialize_with_newrelic_tracing { super(&traced_block) }
end
else
def initialize(**kawrgs, &block)
traced_block = add_thread_tracing(&block)
initialize_with_newrelic_tracing { super(**kawrgs, &traced_block) }
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/new_relic/agent/instrumentation/thread/chain.rb
Expand Up @@ -14,7 +14,7 @@ def self.instrument!
alias_method(:initialize_without_new_relic, :initialize)

def initialize(*args, &block)
traced_block = add_thread_tracing(*args, &block)
traced_block = add_thread_tracing(&block)
initialize_with_newrelic_tracing { initialize_without_new_relic(*args, &traced_block) }
end
end
Expand Down
Expand Up @@ -17,7 +17,6 @@ def add_thread_tracing(*args, &block)
return block if !NewRelic::Agent::Tracer.thread_tracing_enabled?

NewRelic::Agent::Tracer.thread_block_with_current_transaction(
*args,
segment_name: 'Ruby/Thread',
&block
)
Expand Down
2 changes: 1 addition & 1 deletion lib/new_relic/agent/instrumentation/thread/prepend.rb
Expand Up @@ -12,7 +12,7 @@ module Prepend
include NewRelic::Agent::Instrumentation::MonitoredThread

def initialize(*args, &block)
traced_block = add_thread_tracing(*args, &block)
traced_block = add_thread_tracing(&block)
initialize_with_newrelic_tracing { super(*args, &traced_block) }
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/new_relic/agent/tracer.rb
Expand Up @@ -419,10 +419,10 @@ def thread_tracing_enabled?
NewRelic::Agent.config[:'instrumentation.thread.tracing']
end

def thread_block_with_current_transaction(*args, segment_name:, parent: nil, &block)
def thread_block_with_current_transaction(segment_name:, parent: nil, &block)
parent ||= current_segment
current_txn = ::Thread.current[:newrelic_tracer_state]&.current_transaction if ::Thread.current[:newrelic_tracer_state]&.is_execution_traced?
proc do
proc do |*args|
begin
if current_txn && !current_txn.finished?
NewRelic::Agent::Tracer.state.current_transaction = current_txn
Expand Down
5 changes: 2 additions & 3 deletions lib/new_relic/traced_thread.rb
Expand Up @@ -22,15 +22,14 @@ class TracedThread < Thread
# @api public
def initialize(*args, &block)
NewRelic::Agent.record_api_supportability_metric(:traced_thread)
traced_block = create_traced_block(*args, &block)
traced_block = create_traced_block(&block)
super(*args, &traced_block)
end

def create_traced_block(*args, &block)
def create_traced_block(&block)
return block if NewRelic::Agent.config[:'instrumentation.thread.tracing'] # if this is on, don't double trace

NewRelic::Agent::Tracer.thread_block_with_current_transaction(
*args,
segment_name: 'Ruby/TracedThread',
&block
)
Expand Down
4 changes: 3 additions & 1 deletion test/multiverse/suites/thread/Envfile
Expand Up @@ -4,4 +4,6 @@

instrumentation_methods :chain, :prepend

gemfile nil
gemfile <<~RB
# this can't be empty or the tests won't run
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo good to know. Were they not running before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RB
Expand Up @@ -73,4 +73,16 @@ def test_parents_thread_fiber
def test_parents_fiber_thread
run_nested_parent_test(Fiber, Thread)
end

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
Comment on lines +77 to +87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the agent running during these test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end