diff --git a/CHANGELOG.md b/CHANGELOG.md index 5162897e27..61999a0f30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,24 @@ * **Remove Merb Support** This release removes the remaining support for the [Merb](https://weblog.rubyonrails.org/2008/12/23/merb-gets-merged-into-rails-3/) framework. It merged with Rails during the 3.0 release. Now that the Ruby agent supports Rails 3.2 and above, we thought it was time to say goodbye. + * **`add_method_tracer` refactored to use prepend over alias_method chaining** + This release overhauls the implementation of `add_method_tracer`, as detailed in [issue #502](https://github.com/newrelic/newrelic-ruby-agent/issues/502). The main breaking updates are as follows: + - A metric name passed to `add_method_tracer` will no longer be interpolated in an instance context as before. To maintain this behavior, pass a Proc object with the same arity as the method being traced. For example: + + # OLD + add_method_tracer :foo, '#{args[0]}.#{args[1]}' + + # NEW + add_method_tracer :foo, -> (*args) { "#{args[0].#{args[1]" } + + - Similarly, the `:code_header` and `:code_footer` options to `add_method_tracer` will *only* accept a Proc object, which will be bound to the calling instance when the traced method is invoked. + + - Calling `add_method_tracer` for a method will overwrite any previously defined tracers for that method. To specify multiple metric names for a single method tracer, pass them to `add_method_tracer` as an array. + + See updated documentation on the following pages for full details: + https://docs.newrelic.com/docs/agents/ruby-agent/api-guides/ruby-custom-instrumentation/#method_tracers + https://rubydoc.info/github/newrelic/newrelic-ruby-agent/NewRelic/Agent/MethodTracer/ClassMethods#add_method_tracer-instance_method + ## v7.2.0 * **Expected Errors and Ignore Errors** diff --git a/lib/new_relic/agent.rb b/lib/new_relic/agent.rb index 5c08327959..8e81655e0a 100644 --- a/lib/new_relic/agent.rb +++ b/lib/new_relic/agent.rb @@ -139,20 +139,20 @@ def logger=(log) # is initialized; these methods enable us to defer these calls # until we have started up and can process them. # - def add_or_defer_method_tracer(receiver, method_name, metric_name_code, options) + def add_or_defer_method_tracer(receiver, method_name, metric_name, options) @tracer_lock.synchronize do if @agent - receiver.send(:_add_method_tracer_now, method_name, metric_name_code, options) + receiver.send(:_nr_add_method_tracer_now, method_name, metric_name, options) else - @tracer_queue << [receiver, method_name, metric_name_code, options] + @tracer_queue << [receiver, method_name, metric_name, options] end end end def add_deferred_method_tracers_now @tracer_lock.synchronize do - @tracer_queue.each do |receiver, method_name, metric_name_code, options| - receiver.send(:_add_method_tracer_now, method_name, metric_name_code, options) + @tracer_queue.each do |receiver, method_name, metric_name, options| + receiver.send(:_nr_add_method_tracer_now, method_name, metric_name, options) end @tracer_queue = [] diff --git a/lib/new_relic/agent/instrumentation/active_merchant.rb b/lib/new_relic/agent/instrumentation/active_merchant.rb index d75dc0a20a..45a4cbd54d 100644 --- a/lib/new_relic/agent/instrumentation/active_merchant.rb +++ b/lib/new_relic/agent/instrumentation/active_merchant.rb @@ -26,9 +26,9 @@ class ActiveMerchant::Billing::Gateway gateway_name = self.name.split('::').last [:authorize, :purchase, :credit, :void, :capture, :recurring, :store, :unstore, :update].each do |operation| if implemented_methods.include?(operation) - add_method_tracer operation, "ActiveMerchant/gateway/#{gateway_name}/#{operation}" - add_method_tracer operation, "ActiveMerchant/gateway/#{gateway_name}", :push_scope => false - add_method_tracer operation, "ActiveMerchant/operation/#{operation}", :push_scope => false + add_method_tracer operation, [-> (*) { "ActiveMerchant/gateway/#{gateway_name}/#{operation}" }, + -> (*) { "ActiveMerchant/gateway/#{gateway_name}" }, + -> (*) { "ActiveMerchant/operation/#{operation}" }] end end end diff --git a/lib/new_relic/agent/instrumentation/rails/action_controller.rb b/lib/new_relic/agent/instrumentation/rails/action_controller.rb index 40dbd8b573..7a3d43316d 100644 --- a/lib/new_relic/agent/instrumentation/rails/action_controller.rb +++ b/lib/new_relic/agent/instrumentation/rails/action_controller.rb @@ -20,13 +20,13 @@ executes do ActionView::PartialTemplate.class_eval do include NewRelic::Agent::MethodTracer - add_method_tracer :render, 'View/#{path_without_extension[%r{^(/.*/)?(.*)$},2]}.#{@view.template_format}.#{extension}/Partial' + add_method_tracer :render, -> (*) { "View/#{path_without_extension[%r{^(/.*/)?(.*)$},2]}.#{@view.template_format}.#{extension}/Partial" } end # this is for template rendering, as opposed to partial rendering. ActionView::Template.class_eval do include NewRelic::Agent::MethodTracer - add_method_tracer :render, 'View/#{(path_without_extension || @view.controller.newrelic_metric_path)[%r{^(/.*/)?(.*)$},2]}.#{@view.template_format}.#{extension}/Rendering' + add_method_tracer :render, -> (*) { "View/#{(path_without_extension || @view.controller.newrelic_metric_path)[%r{^(/.*/)?(.*)$},2]}.#{@view.template_format}.#{extension}/Rendering" } end end end @@ -47,7 +47,7 @@ executes do ActionController::Base.class_eval do include NewRelic::Agent::MethodTracer - add_method_tracer :render, 'View/#{newrelic_metric_path}/Rendering' + add_method_tracer :render, -> (*) { "View/#{newrelic_metric_path}/Rendering" } end end end @@ -68,12 +68,12 @@ executes do ActionView::RenderablePartial.module_eval do include NewRelic::Agent::MethodTracer - add_method_tracer :render_partial, 'View/#{path[%r{^(/.*/)?(.*)$},2]}/Partial' + add_method_tracer :render_partial, -> (*) { "View/#{path[%r{^(/.*/)?(.*)$},2]}/Partial" } end ActionView::Template.class_eval do include NewRelic::Agent::MethodTracer - add_method_tracer :render, 'View/#{path[%r{^(/.*/)?(.*)$},2]}/Rendering' + add_method_tracer :render, -> (*) { "View/#{path[%r{^(/.*/)?(.*)$},2]}/Rendering" } end end end diff --git a/lib/new_relic/agent/instrumentation/rails/action_web_service.rb b/lib/new_relic/agent/instrumentation/rails/action_web_service.rb index 00202535a4..c239afcdef 100644 --- a/lib/new_relic/agent/instrumentation/rails/action_web_service.rb +++ b/lib/new_relic/agent/instrumentation/rails/action_web_service.rb @@ -38,7 +38,7 @@ if defined?(ActionController) && defined?(ActionController::Base) ActionController::Base.class_eval do if method_defined? :perform_invocation - add_method_tracer :perform_invocation, 'WebService/#{controller_name}/#{args.first}' + add_method_tracer :perform_invocation, -> (*args) { "WebService/#{controller_name}/#{args.first}" } end end end diff --git a/lib/new_relic/agent/method_tracer.rb b/lib/new_relic/agent/method_tracer.rb index 5a8125d137..fad34528a3 100644 --- a/lib/new_relic/agent/method_tracer.rb +++ b/lib/new_relic/agent/method_tracer.rb @@ -48,12 +48,12 @@ module Agent module MethodTracer - def self.included clazz - clazz.extend ClassMethods + def self.included(klass) + klass.extend(ClassMethods) end - def self.extended clazz - clazz.extend ClassMethods + def self.extended(klass) + klass.extend(ClassMethods) end # Trace a given block with stats and keep track of the caller. @@ -69,7 +69,7 @@ def self.extended clazz # @api public # def trace_execution_scoped(metric_names, options=NewRelic::EMPTY_HASH) #THREAD_LOCAL_ACCESS - NewRelic::Agent.record_api_supportability_metric :trace_execution_scoped + NewRelic::Agent.record_api_supportability_metric :trace_execution_scoped unless options[:internal] NewRelic::Agent::MethodTracerHelpers.trace_execution_scoped(metric_names, options) do # Using an implicit block avoids object allocation for a &block param yield @@ -79,13 +79,13 @@ def trace_execution_scoped(metric_names, options=NewRelic::EMPTY_HASH) #THREAD_L # Trace a given block with stats assigned to the given metric_name. It does not # provide scoped measurements, meaning whatever is being traced will not 'blame the # Controller'--that is to say appear in the breakdown chart. - # This is code is inlined in #add_method_tracer. + # # * metric_names is a single name or an array of names of metrics # # @api public # def trace_execution_unscoped(metric_names, options=NewRelic::EMPTY_HASH) #THREAD_LOCAL_ACCESS - NewRelic::Agent.record_api_supportability_metric :trace_execution_unscoped + NewRelic::Agent.record_api_supportability_metric :trace_execution_unscoped unless options[:internal] return yield unless NewRelic::Agent.tl_is_execution_traced? t0 = Time.now begin @@ -103,50 +103,38 @@ module ClassMethods module AddMethodTracer ALLOWED_KEYS = [:metric, :push_scope, :code_header, :code_footer].freeze - # raises an error when the - # NewRelic::Agent::MethodTracer::ClassMethods#add_method_tracer - # method is called with improper keys. This aids in - # debugging new instrumentation by failing fast - def check_for_illegal_keys!(method_name, options) - unrecognized_keys = options.keys - ALLOWED_KEYS - - if unrecognized_keys.any? - raise "Unrecognized options when adding method tracer to #{method_name}: " + - unrecognized_keys.join(', ') - end - end - - # validity checking - add_method_tracer must receive either - # push scope or metric, or else it would record no - # data. Raises an error if this is the case - def check_for_push_scope_and_metric(options) - unless options[:push_scope] || options[:metric] - raise "Can't add a tracer where push_scope is false and metric is false" - end - end - DEFAULT_SETTINGS = {:push_scope => true, :metric => true, :code_header => "", :code_footer => "" }.freeze # Checks the provided options to make sure that they make # sense. Raises an error if the options are incorrect to # assist with debugging, so that errors occur at class # construction time rather than instrumentation run time - def validate_options(method_name, options) + def _nr_validate_method_tracer_options(method_name, options) unless options.is_a?(Hash) raise TypeError.new("Error adding method tracer to #{method_name}: provided options must be a Hash") end - check_for_illegal_keys!(method_name, options) + + unrecognized_keys = options.keys - ALLOWED_KEYS + if unrecognized_keys.any? + raise "Unrecognized options when adding method tracer to #{method_name}: " + + unrecognized_keys.join(', ') + end + options = DEFAULT_SETTINGS.merge(options) - check_for_push_scope_and_metric(options) + unless options[:push_scope] || options[:metric] + raise "Can't add a tracer where push_scope is false and metric is false" + end + options end # Default to the class where the method is defined. # # Example: - # Foo.default_metric_name_code('bar') #=> "Custom/#{Foo.name}/bar" - def default_metric_name_code(method_name) - "Custom/#{derived_class_name}/#{method_name}" + # Foo._nr_default_metric_name_code('bar') #=> "Custom/#{Foo.name}/bar" + def _nr_default_metric_name(method_name) + class_name = _nr_derived_class_name + -> (*) { "Custom/#{class_name}/#{method_name}" } end # Checks to see if the method we are attempting to trace @@ -154,7 +142,7 @@ def default_metric_name_code(method_name) # anything if the method doesn't exist. def newrelic_method_exists?(method_name) exists = method_defined?(method_name) || private_method_defined?(method_name) - ::NewRelic::Agent.logger.error("Did not trace #{derived_class_name}##{method_name} because that method does not exist") unless exists + ::NewRelic::Agent.logger.error("Did not trace #{_nr_derived_class_name}##{method_name} because that method does not exist") unless exists exists end @@ -162,67 +150,25 @@ def newrelic_method_exists?(method_name) # given metric by checking to see if the traced method # exists. Warns the user if methods are being double-traced # to help with debugging custom instrumentation. - def traced_method_exists?(method_name, metric_name_code) - exists = method_defined?(_traced_method_name(method_name, metric_name_code)) - ::NewRelic::Agent.logger.error("Attempt to trace a method twice with the same metric: Method = #{method_name}, Metric Name = #{metric_name_code}") if exists + def method_traced?(method_name) + exists = method_name && _nr_traced_method_module.method_defined?(method_name) + ::NewRelic::Agent.logger.error("Attempt to trace a method twice: Method = #{method_name}") if exists exists end - # Returns a code snippet to be eval'd that skips tracing - # when the agent is not tracing execution. turns - # instrumentation into effectively one method call overhead - # when the agent is disabled - def assemble_code_header(method_name, metric_name_code, options) - header = "return #{_untraced_method_name(method_name, metric_name_code)}(#{ARGS_FOR_RUBY_VERSION}) unless NewRelic::Agent.tl_is_execution_traced?\n" - header += options[:code_header].to_s - header + # Returns an anonymous module that stores prepended trace methods. + def _nr_traced_method_module + @_nr_traced_method_module ||= Module.new end - # returns an eval-able string that contains the traced - # method code used if the agent is not creating a scope for - # use in scoped metrics. - def method_without_push_scope(method_name, metric_name_code, options) - "def #{_traced_method_name(method_name, metric_name_code)}(#{ARGS_FOR_RUBY_VERSION}) - #{assemble_code_header(method_name, metric_name_code, options)} - t0 = Time.now - begin - #{_untraced_method_name(method_name, metric_name_code)}(#{ARGS_FOR_RUBY_VERSION})\n - ensure - duration = (Time.now - t0).to_f - NewRelic::Agent.record_metric(\"#{metric_name_code}\", duration) - #{options[:code_footer]} - end - end" - end - - # returns an eval-able string that contains the tracing code - # for a fully traced metric including scoping - def method_with_push_scope(method_name, metric_name_code, options) - "def #{_traced_method_name(method_name, metric_name_code)}(#{ARGS_FOR_RUBY_VERSION}) - #{assemble_code_header(method_name, metric_name_code, options)} - result = ::NewRelic::Agent::MethodTracerHelpers.trace_execution_scoped(\"#{metric_name_code}\", - :metric => #{options[:metric]}) do - #{_untraced_method_name(method_name, metric_name_code)}(#{ARGS_FOR_RUBY_VERSION}) - end - #{options[:code_footer]} - result - end" - end - - # Decides which code snippet we should be eval'ing in this - # context, based on the options. - def code_to_eval(method_name, metric_name_code, options) - options = validate_options(method_name, options) - if options[:push_scope] - method_with_push_scope(method_name, metric_name_code, options) - else - method_without_push_scope(method_name, metric_name_code, options) + # for testing only + def _nr_clear_traced_methods! + _nr_traced_method_module.module_eval do + self.instance_methods.each { |m| remove_method m } end end - private - - def derived_class_name + def _nr_derived_class_name return self.name if self.name && !self.name.empty? return "AnonymousModule" if self.to_s.start_with?("# { "Custom/#{self.class.name}/foo" } # # This would name the metric according to the class of the runtime - # intance, as opposed to the class where +foo+ is defined. + # instance, as opposed to the class where +foo+ is defined. # # If not provided, the metric name will be Custom/ClassName/method_name. # - # @param [Symbol] method_name the name of the method to trace - # @param [String] metric_name_code the metric name to record calls to - # the traced method under. This may be either a static string, or Ruby - # code to be evaluated at call-time in order to determine the metric + # @param method_name [Symbol] the name of the method to trace + # @param metric_name [String,Proc,Array] the metric name to record calls to + # the traced method under. This may be either a String, or a Proc + # to be evaluated at call-time in order to determine the metric # name dynamically. + # This method also accepts an array of Strings/Procs, in which case the + # first metric given will be scoped, while the remaining metrics will be + # recorded as though passed with :push_scope => false. If an Array of + # metric names is given with :push_scope => false, all metrics will be + # unscoped. # @param [Hash] options additional options controlling how the method is # traced. # @option options [Boolean] :push_scope (true) If false, the traced method will @@ -272,16 +222,17 @@ def derived_class_name # @option options [Boolean] :metric (true) If false, the traced method will # only appear in transaction traces, but no metrics will be recorded # for it. - # @option options [String] :code_header ('') Ruby code to be inserted and run + # @option options [Proc] :code_header ('') Ruby code to be inserted and run # before the tracer begins timing. - # @option options [String] :code_footer ('') Ruby code to be inserted and run + # @option options [Proc] :code_footer ('') Ruby code to be inserted and run # after the tracer stops timing. # # @example # add_method_tracer :foo # # # With a custom metric name - # add_method_tracer :foo, 'Custom/#{self.class.name}/foo' + # add_method_tracer :foo, "Custom/MyClass/foo" + # add_method_tracer :bar, -> { "Custom/#{self.class.name}/bar" } # # # Instrument foo only for custom dashboards (not in transaction # # traces or breakdown charts) @@ -292,65 +243,108 @@ def derived_class_name # # @api public # - def add_method_tracer(method_name, metric_name_code = nil, options = {}) - ::NewRelic::Agent.add_or_defer_method_tracer(self, method_name, metric_name_code, options) + def add_method_tracer(method_name, metric_name = nil, options = {}) + ::NewRelic::Agent.add_or_defer_method_tracer(self, method_name, metric_name, options) end # For tests only because tracers must be removed in reverse-order # from when they were added, or else other tracers that were added to the same method # may get removed as well. - def remove_method_tracer(method_name, metric_name_code) # :nodoc: + def remove_method_tracer(method_name) # :nodoc: return unless Agent.config[:agent_enabled] - if method_defined? "#{_traced_method_name(method_name, metric_name_code)}" - alias_method method_name, "#{_untraced_method_name(method_name, metric_name_code)}" - undef_method "#{_traced_method_name(method_name, metric_name_code)}" - ::NewRelic::Agent.logger.debug("removed method tracer #{method_name} #{metric_name_code}\n") + if _nr_traced_method_module.method_defined?(method_name) + _nr_traced_method_module.send(:remove_method, method_name) + ::NewRelic::Agent.logger.debug("removed method tracer #{method_name}\n") else - raise "No tracer for '#{metric_name_code}' on method '#{method_name}'" + raise "No tracer on method '#{method_name}'" end end private - def _add_method_tracer_now(method_name, metric_name_code, options) + def _nr_add_method_tracer_now(method_name, metric_name, options) NewRelic::Agent.record_api_supportability_metric(:add_method_tracer) - return unless newrelic_method_exists?(method_name) - metric_name_code ||= default_metric_name_code(method_name) - return if traced_method_exists?(method_name, metric_name_code) + remove_method_tracer(method_name) if method_traced?(method_name) - traced_method = code_to_eval(method_name, metric_name_code, options) + options = _nr_validate_method_tracer_options(method_name, options) visibility = NewRelic::Helper.instance_method_visibility self, method_name - class_eval traced_method, __FILE__, __LINE__ - alias_method _untraced_method_name(method_name, metric_name_code), method_name - alias_method method_name, _traced_method_name(method_name, metric_name_code) - ruby2_keywords(_traced_method_name(method_name, metric_name_code)) if respond_to?(:ruby2_keywords, true) - send visibility, method_name - send visibility, _traced_method_name(method_name, metric_name_code) - ::NewRelic::Agent.logger.debug("Traced method: class = #{derived_class_name},"+ + scoped_metric, unscoped_metrics = _nr_scoped_unscoped_metrics(metric_name, method_name, push_scope: options[:push_scope]) + + _nr_define_traced_method(method_name, scoped_metric: scoped_metric, unscoped_metrics: unscoped_metrics, + code_header: options[:code_header], code_footer: options[:code_footer], + record_metrics: options[:metric], visibility: visibility) + + prepend(_nr_traced_method_module) + + ::NewRelic::Agent.logger.debug("Traced method: class = #{_nr_derived_class_name},"+ "method = #{method_name}, "+ - "metric = '#{metric_name_code}'") + "metric = '#{metric_name}'") end - # given a method and a metric, this method returns the - # untraced alias of the method name - def _untraced_method_name(method_name, metric_name) - "#{_sanitize_name(method_name)}_without_trace_#{_sanitize_name(metric_name)}" + # See #add_method_tracer; if multiple metric names are given, the first is + # treated as scoped, the rest unscoped. If options[:push_scope] is false, + # all given metrics are unscoped. + def _nr_scoped_unscoped_metrics(metric_name, method_name, push_scope: true) + if metric_name.is_a?(Array) && push_scope + [metric_name.shift, metric_name] + elsif push_scope + [metric_name || _nr_default_metric_name(method_name), []] + else + [nil, Array(metric_name)] + end end - # given a method and a metric, this method returns the traced - # alias of the method name - def _traced_method_name(method_name, metric_name) - "#{_sanitize_name(method_name)}_with_trace_#{_sanitize_name(metric_name)}" - end + def _nr_define_traced_method(method_name, scoped_metric: nil, unscoped_metrics: [], + code_header: nil, code_footer: nil, record_metrics: true, + visibility: :public) + _nr_traced_method_module.module_eval do + define_method(method_name) do |*args, &block| + return super(*args, &block) unless NewRelic::Agent.tl_is_execution_traced? + scoped_metric_eval, unscoped_metrics_eval = nil, [] + + scoped_metric_eval = case scoped_metric + when Proc + instance_exec(*args, &scoped_metric) + when String + scoped_metric + else + nil + end + + unscoped_metrics_eval = unscoped_metrics.map do |metric| + metric.kind_of?(Proc) ? instance_exec(*args, &metric) : metric.to_s + end + + instance_exec(&code_header) if code_header.kind_of?(Proc) - # makes sure that method names do not contain characters that - # might break the interpreter, for example ! or ? characters - # that are not allowed in the middle of method names - def _sanitize_name(name) - name.to_s.tr_s('^a-zA-Z0-9', '_') + # If tracing multiple metrics on this method, nest one unscoped trace inside the scoped trace. + begin + if scoped_metric_eval + self.class.trace_execution_scoped(scoped_metric_eval, metric: record_metrics, internal: true) do + if unscoped_metrics_eval.empty? + super(*args, &block) + else + self.class.trace_execution_unscoped(unscoped_metrics_eval, internal: true) do + super(*args, &block) + end + end + end + elsif !unscoped_metrics_eval.empty? + self.class.trace_execution_unscoped(unscoped_metrics_eval, internal: true) do + super(*args, &block) + end + end + ensure + instance_exec(&code_footer) if code_footer.kind_of?(Proc) + end + end + + send visibility, method_name + ruby2_keywords(method_name) if respond_to?(:ruby2_keywords, true) + end end end diff --git a/lib/new_relic/constants.rb b/lib/new_relic/constants.rb index d50f102b20..880d04b455 100644 --- a/lib/new_relic/constants.rb +++ b/lib/new_relic/constants.rb @@ -28,10 +28,6 @@ module NewRelic TRACEPARENT_KEY = "traceparent" TRACESTATE_KEY = "tracestate" - # Right now, old and new Rubies are the same. Intention is to use "(...)" for - # Ruby 2.7+ for argument delegation, but doing so is a breaking API change. - ARGS_FOR_RUBY_VERSION = RUBY_VERSION < "2.7.0" ? "*args, &block" : "*args, &block" - HTTP_TRACEPARENT_KEY = "HTTP_#{TRACEPARENT_KEY.upcase}" HTTP_TRACESTATE_KEY = "HTTP_#{TRACESTATE_KEY.upcase}" HTTP_NEWRELIC_KEY = "HTTP_#{NEWRELIC_KEY.upcase}" diff --git a/test/new_relic/agent/method_tracer/class_methods/add_method_tracer_test.rb b/test/new_relic/agent/method_tracer/class_methods/add_method_tracer_test.rb index 402424aafa..85cafa817b 100644 --- a/test/new_relic/agent/method_tracer/class_methods/add_method_tracer_test.rb +++ b/test/new_relic/agent/method_tracer/class_methods/add_method_tracer_test.rb @@ -16,31 +16,10 @@ class AddMethodTracerTest < Minitest::Test def test_validate_options_nonhash assert_raises(TypeError) do - validate_options(:fluttershy, []) + _nr_validate_method_tracer_options(:fluttershy, []) end end - def test_validate_options_defaults - self.expects(:check_for_illegal_keys!) - self.expects(:check_for_push_scope_and_metric) - validate_options(:applejack, {}) - end - - def test_validate_options_override - opts = {:push_scope => false, :metric => false, :force => true} - self.expects(:check_for_illegal_keys!) - self.expects(:check_for_push_scope_and_metric) - val = validate_options(:pinkie_pie, opts) - assert val.is_a?(Hash) - assert (val[:push_scope] == false), val.inspect - assert (val[:metric] == false), val.inspect - assert (val[:force] == true), val.inspect - end - - def test_default_metric_name_code - assert_equal "Custom/#{self.class.name}/test_method", self.class.default_metric_name_code('test_method') - end - def test_newrelic_method_exists_positive self.expects(:method_defined?).returns(true) assert newrelic_method_exists?('test_method') @@ -55,55 +34,30 @@ def test_newrelic_method_exists_negative def test_check_for_illegal_keys_positive assert_raises(RuntimeError) do - check_for_illegal_keys!(:twilight_sparkle, {:unknown_key => nil}) + _nr_validate_method_tracer_options(:twilight_sparkle, {:unknown_key => nil}) end end def test_check_for_illegal_keys_negative - test_keys = Hash[*ALLOWED_KEYS.map {|x| [x, nil]}.flatten] - check_for_illegal_keys!(:rainbow_dash, test_keys) + test_keys = Hash[*ALLOWED_KEYS.map {|x| [x, true]}.flatten] + _nr_validate_method_tracer_options(:rainbow_dash, test_keys) end def test_traced_method_exists_positive - self.expects(:_traced_method_name) - self.expects(:method_defined?).returns(true) - assert traced_method_exists?('test_method', 'Custom/Test/test_method') + self._nr_traced_method_module.expects(:method_defined?).returns(true) + assert method_traced?('test_method') end def test_traced_method_exists_negative - self.expects(:_traced_method_name) - self.expects(:method_defined?).returns(false) - assert !traced_method_exists?(nil, nil) - end - - def test_assemble_code_header_unforced - self.expects(:_untraced_method_name).returns("method_name_without_tracing") - opts = {:force => false, :code_header => 'CODE HEADER'} - assert_equal "return method_name_without_tracing(*args, &block) unless NewRelic::Agent.tl_is_execution_traced?\nCODE HEADER", assemble_code_header('test_method', 'Custom/Test/test_method', opts) - end - - def test_check_for_push_scope_and_metric_positive - check_for_push_scope_and_metric({:push_scope => true}) - check_for_push_scope_and_metric({:metric => true}) + self._nr_traced_method_module.expects(:method_defined?).returns(false) + refute method_traced?('test_method') end def test_check_for_push_scope_and_metric_negative assert_raises(RuntimeError) do - check_for_push_scope_and_metric({:push_scope => false, :metric => false}) + _nr_validate_method_tracer_options(:foo, {:push_scope => false, :metric => false}) end end - - def test_code_to_eval_scoped - self.expects(:validate_options).returns({:push_scope => true}) - self.expects(:method_with_push_scope).with('test_method', 'Custom/Test/test_method', {:push_scope => true}) - code_to_eval('test_method', 'Custom/Test/test_method', {}) - end - - def test_code_to_eval_unscoped - self.expects(:validate_options).returns({:push_scope => false}) - self.expects(:method_without_push_scope).with('test', 'Custom/Test/test', {:push_scope => false}) - code_to_eval('test', 'Custom/Test/test', {}) - end end end end diff --git a/test/new_relic/agent/method_tracer_params_test.rb b/test/new_relic/agent/method_tracer_params_test.rb index 4ab5af7258..68149bb4f1 100644 --- a/test/new_relic/agent/method_tracer_params_test.rb +++ b/test/new_relic/agent/method_tracer_params_test.rb @@ -126,13 +126,13 @@ def assert_common_tracing_behavior traced_class ["traced_metric_methods_unscoped", TracedMetricMethodsUnscoped], ].each do |traced_class_name, traced_class| - # We're doing it all in one big super test because order of invocation matters! - # When many small test scenarios, if the tests for deprecation warnings emitted - # by the compiler are not invoked first, then we miss our chance to capture - # that output and assert/refute reliably. - # This very large run ensures order of calls always happen in predictable order. - define_method "test_expected_results_#{traced_class_name}" do - + # We're doing it all in one big super test because order of invocation matters! + # When many small test scenarios, if the tests for deprecation warnings emitted + # by the compiler are not invoked first, then we miss our chance to capture + # that output and assert/refute reliably. + # This very large run ensures order of calls always happen in predictable order. + define_method "test_expected_results_#{traced_class_name}" do + expected = {foo: {bar: "foobar"}} expected369 = {1=>3, 2=>6, 3=>9} instance = traced_class.new @@ -146,12 +146,6 @@ def assert_common_tracing_behavior traced_class refute_deprecation_warning { instance.wildcard_args(:foo, bar: "foobar") } refute_deprecation_warning { instance.wildcard_args(:foo, {bar: "foobar"}) } - if RUBY_VERSION >= "2.7.0" && RUBY_VERSION < "3.0.0" - assert_deprecation_warning { instance.args_and_kwargs(:foo, {bar: "foobar"}) } - else - refute_deprecation_warning { instance.args_and_kwargs(:foo, {bar: "foobar"}) } - end - refute_deprecation_warning { instance.last_arg_is_a_keyword(:foo, bar: "foobar") } refute_deprecation_warning { instance.all_args_are_keywords(foo: :foo, bar: {bar: "foobar"}) } refute_deprecation_warning { instance.args_and_kwargs(:foo, bar: "foobar") } diff --git a/test/new_relic/agent/method_tracer_test.rb b/test/new_relic/agent/method_tracer_test.rb index 65f4a1dbfa..883b147284 100644 --- a/test/new_relic/agent/method_tracer_test.rb +++ b/test/new_relic/agent/method_tracer_test.rb @@ -3,15 +3,7 @@ # See https://github.com/newrelic/newrelic-ruby-agent/blob/main/LICENSE for complete details. require File.expand_path(File.join(File.dirname(__FILE__),'..','..','test_helper')) - -class Module - def method_traced?(method_name, metric_name) - traced_method_prefix = _traced_method_name(method_name, metric_name) - - method_defined? traced_method_prefix - end -end - +require 'pry' class Insider def initialize(stats_engine) @stats_engine = stats_engine @@ -43,17 +35,19 @@ def module_method_to_be_traced(x, testcase) end module TestModuleWithLog - extend self - def other_method - #just here to be traced - log "12345" - end + class << self + def other_method + #just here to be traced + log "12345" + end + + def log( msg ) + msg + end - def log( msg ) - msg + include NewRelic::Agent::MethodTracer + add_method_tracer :other_method, 'Custom/foo/bar' end - include NewRelic::Agent::MethodTracer - add_method_tracer :other_method, 'Custom/foo/bar' end class MyClass @@ -66,6 +60,16 @@ class << self end end +module MyModule + def self.module_method + end + + class << self + include NewRelic::Agent::MethodTracer + add_method_tracer :module_method + end +end + class NewRelic::Agent::MethodTracerTest < Minitest::Test attr_reader :stats_engine @@ -84,11 +88,8 @@ def setup def teardown @stats_engine.clear_stats - begin - self.class.remove_method_tracer :method_to_be_traced, @metric_name if @metric_name - rescue RuntimeError - # ignore 'no tracer' errors from remove method tracer - end + + self.class._nr_clear_traced_methods! @metric_name = nil NewRelic::Agent.shutdown @@ -138,7 +139,7 @@ def test_add_method_tracer end begin - self.class.remove_method_tracer :method_to_be_traced, METRIC + self.class.remove_method_tracer :method_to_be_traced rescue RuntimeError # ignore 'no tracer' errors from remove method tracer end @@ -166,6 +167,15 @@ def test_add_class_method_tracer assert_metrics_recorded metric => {:call_count => 1} end + def test_add_module_method_tracer + in_transaction do + MyModule.module_method + end + + metric = "Custom/MyModule/Class/module_method" + assert_metrics_recorded metric => {:call_count => 1} + end + def test_add_anonymous_class_method_tracer cls = Class.new do def instance_method; end @@ -208,11 +218,11 @@ def test_add_method_tracer_keyword_args end def test_method_traced? - assert !self.class.method_traced?(:method_to_be_traced, METRIC) + assert !self.class.method_traced?(:method_to_be_traced) self.class.add_method_tracer :method_to_be_traced, METRIC - assert self.class.method_traced?(:method_to_be_traced, METRIC) + assert self.class.method_traced?(:method_to_be_traced) begin - self.class.remove_method_tracer :method_to_be_traced, METRIC + self.class.remove_method_tracer :method_to_be_traced rescue RuntimeError # ignore 'no tracer' errors from remove method tracer end @@ -261,7 +271,7 @@ def test_add_same_tracer_twice end begin - self.class.remove_method_tracer :method_to_be_traced, METRIC + self.class.remove_method_tracer :method_to_be_traced rescue RuntimeError # ignore 'no tracer' errors from remove method tracer end @@ -270,7 +280,7 @@ def test_add_same_tracer_twice end def test_add_tracer_with_dynamic_metric - metric_code = '#{args[0]}.#{args[1]}' + metric_code = -> (*args) { "#{args[0]}.#{args[1]}" } @metric_name = metric_code expected_metric = "1.2" self.class.add_method_tracer :method_to_be_traced, metric_code @@ -280,7 +290,7 @@ def test_add_tracer_with_dynamic_metric end begin - self.class.remove_method_tracer :method_to_be_traced, metric_code + self.class.remove_method_tracer :method_to_be_traced rescue RuntimeError # ignore 'no tracer' errors from remove method tracer end @@ -299,15 +309,9 @@ def test_trace_method_with_block assert_metrics_recorded METRIC => {:call_count => 1, :total_call_time => 0.15} end - def test_trace_module_method - NewRelic::Agent.add_method_tracer :module_method_to_be_traced, '#{args[0]}' - NewRelic::Agent.module_method_to_be_traced "x", self - NewRelic::Agent.remove_method_tracer :module_method_to_be_traced, '#{args[0]}' - end - def test_remove self.class.add_method_tracer :method_to_be_traced, METRIC - self.class.remove_method_tracer :method_to_be_traced, METRIC + self.class.remove_method_tracer :method_to_be_traced method_to_be_traced 1,2,3,false,METRIC @@ -322,7 +326,6 @@ def test_multiple_metrics__scoped end end - assert_metrics_recorded({ 'first' => {:call_count => 1, :total_call_time => 0.05}, 'second' => {:call_count => 1, :total_call_time => 0.05}, @@ -363,12 +366,10 @@ def test_add_multiple_tracers in_transaction('test_txn') do self.class.add_method_tracer :method_to_be_traced, 'XX', :push_scope => false method_to_be_traced 1,2,3,true,nil + self.class.remove_method_tracer :method_to_be_traced + method_to_be_traced 1,2,3,true,nil self.class.add_method_tracer :method_to_be_traced, 'YY' method_to_be_traced 1,2,3,true,'YY' - self.class.remove_method_tracer :method_to_be_traced, 'YY' - method_to_be_traced 1,2,3,true,nil - self.class.remove_method_tracer :method_to_be_traced, 'XX' - method_to_be_traced 1,2,3,false,'XX' end assert_metrics_recorded({ @@ -376,16 +377,17 @@ def test_add_multiple_tracers }) end - def test_add_method_tracer_module_double_inclusion - mod = Module.new { def traced_method; end } - cls = Class.new { include mod } - - mod.module_eval do - include NewRelic::Agent::MethodTracer - add_method_tracer :traced_method + def test_add_multiple_metrics + in_transaction('test_txn') do + self.class.add_method_tracer :method_to_be_traced, ['XX', 'YY', -> (*) { 'ZZ' }] + method_to_be_traced 1, 2, 3, true, nil end - cls.new.traced_method + assert_metrics_recorded([ + ['XX', 'test_txn'], + 'YY', + 'ZZ' + ]) end # This test validates that including the MethodTracer module does not pollute @@ -412,7 +414,7 @@ def trace_no_push_scope in_transaction 'test_txn' do self.class.add_method_tracer :method_to_be_traced, 'X', :push_scope => false method_to_be_traced 1,2,3,true,nil - self.class.remove_method_tracer :method_to_be_traced, 'X' + self.class.remove_method_tracer :method_to_be_traced method_to_be_traced 1,2,3,false,'X' end