newrelic / newrelic-ruby-agent

New Relic RPM Ruby Agent
https://docs.newrelic.com/docs/apm/agents/ruby-agent/getting-started/introduction-new-relic-ruby/
Apache License 2.0
1.2k stars 598 forks source link

Ruby 2.7 deprecation warning tracks the method wrongly to new relic line #349

Closed bishwahang closed 4 years ago

bishwahang commented 4 years ago

I'm not quite sure if this issue belongs here or in Ruby project.

Ruby tracks the message defined in another library/project to this line on NewRelic gem, https://github.com/newrelic/rpm/blob/main/lib/new_relic/agent/method_tracer.rb#L337, and then throws the deprecation warning.

# trigger_profile_confirmed_hooks.rb

class TriggerProfileConfirmedHooks
  Messenger.publish("user.confirmed", {foo: “bar”})
end

# messenger.rb
class Messenger
  def self.publish(message_type, payload = {}, opts = {})
  end
end

Warning:

app/services/trigger_profile_confirmed_hooks.rb|10 warning| Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/newrelic_rpm-6.11.0.365/lib/new_relic/agent/method_tracer.rb|337 warning| The called method `publish' is defined here

Apparently, it happens only for particular method signature. If I change the method name on my project to: def something_else(message_type, payload = {}, opts = {}), instead of def publish(message_type, payload = {}, opts = {}), the warning goes away.

May be this will go away, when New Relic gem will handle all the 2.7 warnings. Or may be this is related with Ruby wrongly tracing method call.

tannalynn commented 4 years ago

Hello @bishwahang Thank you for bringing this instance of this warning to our attention. We are working on eliminating these warnings, and will include this instance as well. We will let you know when we have released this fix.

tannalynn commented 4 years ago

Hello @bishwahang ! While looking into these warnings, I learned that this instance of the warning is a little different from the others. I was able to replicate it by adding methods that match your signatures and adding add_method_tracer for the method. We had already updated the method tracer to use the new kwargs, so I believe the warning is actually coming from the way the method is being called. When I change Messenger.publish("user.confirmed", {foo: “bar”}) to Messenger.publish("user.confirmed", **{foo: “bar”}) The warning no longer appears. So adding the double splat to the locations you call the method should remove these warnings for you.

I found this page very helpful in understanding more of the nuances of the keywords arguments changes, personally. https://blog.saeloun.com/2019/10/07/ruby-2-7-keyword-arguments-redesign.html

Hopefully that helps!

Drowze commented 4 years ago

Can we reopen this issue?

NewRelic gem is throwing 2.7 warnings on our codebase. Here is the minimal reproduceable script:

require 'newrelic_rpm'

class ComponentRenderer
  def render_component(component_name, options = {})
    puts component_name
    puts options
  end
end

class ComponentRenderer2 < ComponentRenderer
  include ::NewRelic::Agent::MethodTracer

  add_method_tracer :render_component
end

# does not throw warning
ComponentRenderer.new.render_component(:test, { props: { test: 1 } })

# throws warning
# warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
# /Users/drowze/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/newrelic_rpm-6.12.0.367/lib/new_relic/agent/method_tracer.rb:337: warning: The called method `render_component' is defined here
ComponentRenderer2.new.render_component(:test, { props: { test: 1 } })
rachelleahklein commented 4 years ago

Hi @Drowze, we are looking into this. Thanks for the minimal repro, that's very helpful. I'll let you know when there's an update.

mwlang commented 4 years ago

This should be solved by this PR: #422

github-actions[bot] commented 4 years ago

This has been marked stale after 30 days with no activity. It will be closed in 5 days if there is no activity.

angelatan2 commented 4 years ago

We are working on this issue within the bug smash milestone.