Refactor method tracers to use Module#prepend, no longer class_eval strings

[amhuntsman]

This PR incorporates the four proposed changes from #502.

• Implement using prepend instead of eval and call super to invoke the original method that is wrapped.
• Change code_header and code_footer to accept Procs instead of Strings that is Ruby code. This will allow us to also avoid having to use eval
• Allow metric names to be String or Proc and no longer an embedded, interpolatable String.
• Handle method parameter delegation in a way that is future-proof and works from Ruby 2.0 through Ruby 3.2+ and does not cause compiler warnings that attribute syntax issues to the Ruby agent. That is, any such syntax errors are correctly surfacing at the call site or point of original method's definition rather than as side-effect of Ruby agent wrapping the original method.

These are wide-reaching concerns that were difficult to separate from each other in the same PR, hence their being grouped together here.

Switch to Module#prepend

add_method_tracer now sets a class instance variable that holds an anonymous Module, which is prepended to the calling class. A method is created with the same name as the traced method, that wraps the original method in tracing logic. This works with tracing class methods as well.

One point of confusion here is that the Ruby agent runs the following on initialization:

Module.send :include, NewRelic::Agent::MethodTracer::ClassMethods
Module.send :include, NewRelic::Agent::MethodTracer

This gives all classes and modules access to add_method_tracer, though in our documentation and custom instrumentation (and many tests) we still explicitly include NewRelic::Agent::MethodTracer. Changing this is beyond the scope of this milestone, but we should revisit whether the above step is necessary.

Change :code_header and :code_footer to accept Procs, no longer class_eval strings

Fairly straightforward change: anything that was a string is now a Proc, which is evaluated by instance_exec in the traced method.

Allow metric names to be String or Proc; String no longer interpolated

Similar to the above. One big catch here: the arity of this Proc is important. It should have the same arity of the method being traced (or accept any arguments, i.e. -> (*) { #code }. This was done in order to preserve a test case like this:

add_method_tracer :foo, '#{args[0]}.#{args[1]}'

Previously args was provided by the tracer method body built by string. After this change, the provided Proc must accept at least the arguments passed to it, or an ArgumentError will be raised. The above will be rewritten as:

add_method_tracer :foo, -> (*args) { "#{args[0]}.#{args[1]}" }

Future-proof method delegation (Ruby 3 kwargs)

This is handled as suggested in this article: https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html

That is, the delegate method uses the (*args, &block) parameter format, and the ruby2_keywords feature in Ruby 2.7+.

We've had some discussion on what to do when ruby2_keywords is removed from Ruby. I recommend not worrying about it until that actually happens, so we can see what the Ruby community recommends. The good news is that, with this refactoring, there's only one line in method tracing that we'll need to change.

Breaking changes

• Strings provided to :code_header and :code_footer will be ignored; only Procs will be processed. Strings passed as metric names will not be interpolated at instance level.
• Calling add_method_tracer twice for the same method will not overwrite the first (as of this PR; we can probably change this behavior)
• add_method_tracer-generated methods are public regardless of the original method's visibility - a natural consequence of the Module#prepend paradigm
• remove_method_tracer now only takes a single argument (the method name)

Related Issues

#717
#718
#719
#720

Reviewer Checklist

• Perform code review
• Add performance label
• Perform appropriate level of performance testing
• Confirm all checks passed
• Add version label prior to acceptance

[amhuntsman]

Making a TODO list while I'm thinking of it:

• update changelog
• add parameter checks to method tracer tests (spread over multiple tests or have one test that uses a variety of formats)
• replace method visibility code and tests
• allow for setting multiple metrics/options on the same method tracer

The last item came about when I realized that's why the ActiveMerchant tests are broken. In our instrumentation we have:

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

which currently only records the first tracer and ignores the next two. This functionality is not really documented, and looking at our current code I'm not even sure how it's working right now. At any rate, this shouldn't be too hard to implement under the new model.

[tannalynn]

@amhuntsman oh, can you add a changelog entry for this?