-
Notifications
You must be signed in to change notification settings - Fork 599
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
Refactor method tracers to use Module#prepend, no longer class_eval strings #728
Conversation
Making a TODO list while I'm thinking of it:
The last item came about when I realized that's why the ActiveMerchant tests are broken. In our instrumentation we have:
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. |
972d7f3
to
e609479
Compare
a882930
to
5caec32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amhuntsman oh, can you add a changelog entry for this?
5caec32
to
348f325
Compare
This PR incorporates the four proposed changes from #502.
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:
This gives all classes and modules access to
add_method_tracer
, though in our documentation and custom instrumentation (and many tests) we still explicitlyinclude 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 longerclass_eval
stringsFairly 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: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 anArgumentError
will be raised. The above will be rewritten as: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 theruby2_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
: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.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 theModule#prepend
paradigmremove_method_tracer
now only takes a single argument (the method name)Related Issues
#717
#718
#719
#720
Reviewer Checklist