diff --git a/.env.example b/.env.example index 489289cb3b..13ee5d4a97 100644 --- a/.env.example +++ b/.env.example @@ -108,9 +108,12 @@ PERIODICAL=stop_expired_deploys:60,remove_expired_locks:60,report_system_stats:6 # RELEASE_TAG_IN_REPO_RETRIES=10 # how often to retry finding a tag after it is released to github ## NewRelic: optional report performance stats see https://docs.newrelic.com/docs/agents/ruby-agent/configuration/ruby-agent-configuration -# NEW_RELIC_LICENSE_KEY: my-key -# NEW_RELIC_APP_NAME: Samson -# NEW_RELIC_LOG_FILE_PATH: STDOUT +# NEW_RELIC_LICENSE_KEY= +# NEW_RELIC_APP_NAME=Samson +# NEW_RELIC_LOG_FILE_PATH=STDOUT +# +# optional: show graphs during/after deploy +# NEW_RELIC_API_KEY= ## Memcache: configure servers or we use localhost # MEMCACHIER_SERVERS=a:123,b:234 diff --git a/Gemfile b/Gemfile index c35f87143b..54e8f256f0 100644 --- a/Gemfile +++ b/Gemfile @@ -42,7 +42,6 @@ gem 'coderay' gem 'net-http-persistent' gem 'concurrent-ruby' gem 'vault' -gem 'newrelic_rpm' gem 'lograge' gem 'logstash-event' gem 'diffy' diff --git a/Gemfile.lock b/Gemfile.lock index c440982773..96f98e85f8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -94,6 +94,7 @@ PATH remote: plugins/new_relic specs: samson_new_relic (0.0.0) + newrelic_rpm PATH remote: plugins/pipelines @@ -583,7 +584,6 @@ DEPENDENCIES momentjs-rails mysql2 net-http-persistent - newrelic_rpm octokit omniauth omniauth-bitbucket diff --git a/app/models/deploy_service.rb b/app/models/deploy_service.rb index 72023aab30..f7505b6d3d 100644 --- a/app/models/deploy_service.rb +++ b/app/models/deploy_service.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true class DeployService - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers attr_reader :user def initialize(user) diff --git a/app/models/docker_builder_service.rb b/app/models/docker_builder_service.rb index 111bd1b501..b68d8dc1ae 100644 --- a/app/models/docker_builder_service.rb +++ b/app/models/docker_builder_service.rb @@ -2,7 +2,7 @@ require 'docker' class DockerBuilderService - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers def initialize(build) @build = build diff --git a/app/models/git_repository.rb b/app/models/git_repository.rb index f70560e869..31f892fb1a 100644 --- a/app/models/git_repository.rb +++ b/app/models/git_repository.rb @@ -2,7 +2,7 @@ # Responsible for all git knowledge of a repo # Caches a local mirror (not a full checkout) and creates a workspace when deploying class GitRepository - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers attr_accessor :executor # others set this to listen in on commands being executed @@ -132,7 +132,7 @@ def clone! def create_workspace(temp_dir) executor.execute "git clone #{repo_cache_dir} #{temp_dir}" end - add_tracer :create_workspace! + add_tracer :create_workspace def update! executor.execute("cd #{repo_cache_dir}", 'git fetch -p') diff --git a/app/models/image_builder.rb b/app/models/image_builder.rb index 1f5df35e9c..f0f77f2ca9 100644 --- a/app/models/image_builder.rb +++ b/app/models/image_builder.rb @@ -4,9 +4,9 @@ class ImageBuilder class << self - DIGEST_SHA_REGEX = /Digest:.*(sha256:[0-9a-f]{64})/i + extend ::Samson::PerformanceTracer::Tracers - include ::Samson::PerformanceTracer + DIGEST_SHA_REGEX = /Digest:.*(sha256:[0-9a-f]{64})/i def build_image(dir, build, executor, tag_as_latest:, **args) if DockerRegistry.all.empty? diff --git a/app/models/job_execution.rb b/app/models/job_execution.rb index 9c6583882e..14deb38cda 100644 --- a/app/models/job_execution.rb +++ b/app/models/job_execution.rb @@ -2,7 +2,7 @@ require 'shellwords' class JobExecution - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers cattr_accessor(:cancel_timeout, instance_writer: false) { 15.seconds } diff --git a/app/models/multi_lock.rb b/app/models/multi_lock.rb index 545b746bc1..9bdfdc6786 100644 --- a/app/models/multi_lock.rb +++ b/app/models/multi_lock.rb @@ -4,7 +4,7 @@ class MultiLock cattr_accessor(:locks) { {} } class << self - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers def lock(id, holder, options) locked = wait_for_lock(id, holder, options) diff --git a/config/application.rb b/config/application.rb index 63d538e7f3..8728a28c38 100644 --- a/config/application.rb +++ b/config/application.rb @@ -17,22 +17,11 @@ Bundler.require(:assets) if Rails.env.development? || ENV["PRECOMPILE"] ### -# Railties need to be loaded before the application is defined +# Railties need to be loaded before the application is initialized if ['development', 'staging'].include?(Rails.env) && ENV["SERVER_MODE"] require 'rack-mini-profiler' # side effect: removes expires headers Rack::MiniProfiler.config.authorization_mode = :allow_all end - -if ['staging', 'production'].include?(Rails.env) - require 'newrelic_rpm' -else - # avoids circular dependencies warning - # https://discuss.newrelic.com/t/circular-require-in-ruby-agent-lib-new-relic-agent-method-tracer-rb/42737 - require 'new_relic/control' - - # needed even in dev/test mode - require 'new_relic/agent/method_tracer' -end # END Railties ### diff --git a/config/initializers/docker.rb b/config/initializers/docker.rb index 0ca6c03049..54340692eb 100644 --- a/config/initializers/docker.rb +++ b/config/initializers/docker.rb @@ -16,6 +16,7 @@ end rescue warn "Unable to verify local docker!" - ErrorNotifier.notify($!, sync: true) # sync to avoid background threads breaking boot_check.rb thread checker + # errors and hooks they trigger cause background threads, that would break boot_check.rb thread checker + ErrorNotifier.notify($!) unless Rails.env.development? end end diff --git a/docs/plugins.md b/docs/plugins.md index 3768f9d88b..0332a1644d 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -25,7 +25,7 @@ Available plugins: - [Kubernetes](https://github.com/zendesk/samson/tree/master/plugins/kubernetes) - [Ledger](https://github.com/zendesk/samson/tree/master/plugins/ledger) - [Release Number From CI](https://github.com/redbubble/samson-release-number-from-ci) - - [NewRelic monitoring](https://github.com/zendesk/samson/tree/master/plugins/new_relic) + - [NewRelic](https://github.com/zendesk/samson/tree/master/plugins/new_relic) - [Pipelined deploys](https://github.com/zendesk/samson/tree/master/plugins/pipelines) - [Slack deploys](https://github.com/zendesk/samson/tree/master/plugins/slack_app) - [Slack notifications](https://github.com/zendesk/samson/tree/master/plugins/slack_webhooks) diff --git a/lib/samson/hooks.rb b/lib/samson/hooks.rb index 8db322af2c..53a732ec6f 100644 --- a/lib/samson/hooks.rb +++ b/lib/samson/hooks.rb @@ -58,7 +58,7 @@ class UserError < StandardError :release_deploy_conditions, :stage_clone, :stage_permitted_params, - :performance_tracer, + :trace_method, :asynchronous_performance_tracer ].freeze diff --git a/lib/samson/performance_tracer.rb b/lib/samson/performance_tracer.rb index 36e956e86d..2740d5b9cc 100644 --- a/lib/samson/performance_tracer.rb +++ b/lib/samson/performance_tracer.rb @@ -1,41 +1,22 @@ # frozen_string_literal: true module Samson module PerformanceTracer - # It's used to trace the hook fire event, - # We can't use the samson hook to fetch the available plugins. - TRACER_PLUGINS = ['SamsonNewRelic', 'SamsonDatadogTracer::APM'].freeze - class << self - def included(clazz) - clazz.extend ClassMethods - end - - def trace_execution_scoped(scope_name) - # Tracing the scope is restricted to avoid into slow startup - # Refer Samson::BootCheck - if ['staging', 'production'].include?(Rails.env) - plugins = TRACER_PLUGINS.map(&:safe_constantize).compact - execution = using_plugins plugins, scope_name do - yield - end - execution.call - else - yield - end + # NOTE: this cannot be a hook since it is used from Hooks#fire + def handlers + @handlers ||= [] end - def using_plugins(plugins, scope_name, &block) - plugins.inject(block) { |inner, plugin| plugin.trace_method_execution_scope(scope_name) { inner } } + def trace_execution_scoped(scope_name, &block) + handlers.inject(block) { |inner, plugin| -> { plugin.trace_execution_scoped(scope_name, &inner) } }.call end end - # for Newrelic and Datadog -> for tracer plugins. - module ClassMethods + module Tracers def add_tracer(method) - Samson::Hooks.fire(:performance_tracer, self, method) + Samson::Hooks.fire(:trace_method, self, method) end - # TODO: Add asynchronous tracer for Datadog. def add_asynchronous_tracer(method, options) Samson::Hooks.fire(:asynchronous_performance_tracer, self, method, options) end diff --git a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb b/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb deleted file mode 100644 index 71f3f31133..0000000000 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb +++ /dev/null @@ -1,104 +0,0 @@ -# frozen_string_literal: true -module SamsonDatadogTracer - module APM - class << self - def included(clazz) - clazz.extend ClassMethods - end - - def trace_method_execution_scope(scope_name) - if SamsonDatadogTracer.enabled? - Datadog.tracer.trace("Custom/Hooks/#{scope_name}") do - yield - end - else - yield - end - end - end - - module ClassMethods - def trace_method(method) - return unless SamsonDatadogTracer.enabled? - # Wrap the helper methods and alias method into the module. - # prepend the wraped module to base class - @apm_module ||= begin - mod = Module.new - mod.extend(SamsonDatadogTracer::APM::Helpers) - prepend(mod) - mod - end - if method_defined?(method) || private_method_defined?(method) - _add_wrapped_method_to_module(method) - end - - @traced_methods ||= [] - @traced_methods << method - end - - private - - def _add_wrapped_method_to_module(method) - klass = self - - @apm_module.module_eval do - _wrap_method(method, klass) - end - end - end - - module Helpers - class << self - def sanitize_name(name) - name.to_s.parameterize.tr('-', '_') - end - - def tracer_method_name(method_name) - "#{sanitize_name(method_name)}_with_apm_tracer" - end - - def untracer_method_name(method_name) - "#{sanitize_name(method_name)}_with_apm_untracer" - end - end - - private - - def _wrap_method(method, klass) - visibility = _original_visibility(method, klass) - _define_traced_method(method, "#{klass}##{method}") - _set_visibility(method, visibility) - end - - def _original_visibility(method, klass) - if klass.protected_method_defined?(method) - :protected - elsif klass.private_method_defined?(method) - :private - else - :public - end - end - - def _define_traced_method(method, trace_name) - define_method(Helpers.tracer_method_name(method)) do |*args, &block| - Datadog.tracer.trace(trace_name) do - send(Helpers.untracer_method_name(method), *args, &block) - end - end - end - - def _set_visibility(method, visibility) - method_name = Helpers.tracer_method_name(method) - case visibility - when :protected - protected(method_name) - when :private - private(method_name) - else - public(method_name) - end - end - end - end -end diff --git a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb index e4130fc4ec..e91113c031 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -3,22 +3,61 @@ module SamsonDatadogTracer class Engine < Rails::Engine end - def self.enabled? - !!ENV['DATADOG_TRACER'] + class << self + def enabled? + !!ENV['DATADOG_TRACER'] + end + + def trace_execution_scoped(scope_name, &block) + if enabled? + Datadog.tracer.trace("Custom/Hooks/#{scope_name}", &block) + else + yield + end + end + + # We are not using super to make running newrelic (which uses alias) and datadog possible + def trace_method(klass, method) + wrap_method klass, method, "apm_tracer" do |&block| + Datadog.tracer.trace("#{klass}###{method}", &block) + end + end + + private + + def wrap_method(klass, method, scope, &callback) + visibility = method_visibility(klass, method) + without = "without_#{scope}_#{method}" + if klass.method_defined?(without) || klass.private_method_defined?(without) + raise "#{scope} wrapper already defined for #{method}" + end + klass.alias_method without, method + klass.define_method(method) do |*args, &block| + callback.call { send(without, *args, &block) } # rubocop:disable Performance/RedundantBlockCall + end + klass.send visibility, method + klass.send visibility, without + end + + def method_visibility(klass, method) + if klass.protected_method_defined?(method) + :protected + elsif klass.private_method_defined?(method) + :private + else + :public + end + end end end -Samson::Hooks.callback :performance_tracer do |klass, method| +require 'samson/performance_tracer' +Samson::PerformanceTracer.handlers << SamsonDatadogTracer + +Samson::Hooks.callback :trace_method do |klass, method| if SamsonDatadogTracer.enabled? - klass.class_eval do - include SamsonDatadogTracer::APM - helper = SamsonDatadogTracer::APM::Helpers - trace_method method - - if method_defined?(method) || private_method_defined?(method) - alias_method helper.untracer_method_name(method), method - alias_method method, helper.tracer_method_name(method) - end - end + SamsonDatadogTracer.trace_method klass, method end end + +# TODO: support :asynchronous_performance_tracer hook, see lib/samson/performance_tracer.rb diff --git a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb deleted file mode 100644 index ab3af11e5e..0000000000 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb +++ /dev/null @@ -1,138 +0,0 @@ -# frozen_string_literal: true -require_relative "../test_helper" - -SingleCov.covered! - -describe SamsonDatadogTracer::APM do - module DDTracer - def self.trace(*) - yield - end - end - - module Datadog - def self.tracer - DDTracer - end - end - - describe ".trace_method_execution_scope" do - it "skips tracer when disabled" do - with_env DATADOG_TRACER: nil do - Datadog.expects(:tracer).never - SamsonDatadogTracer::APM.trace_method_execution_scope("test") { "without tracer" } - end - end - - it "trigger tracer when enabled" do - with_env DATADOG_TRACER: "1" do - Rails.stubs(:env).returns("staging") - Datadog.expects(:tracer).returns(DDTracer) - SamsonDatadogTracer::APM.trace_method_execution_scope("test") { "with tracer" } - end - end - end - - class TestKlass1 - include SamsonDatadogTracer::APM - SamsonDatadogTracer::APM.module_eval { include Datadog } - - def pub_method - :pub - end - - trace_method :pub_method - end - - describe "skips APM trace methods" do - let(:klass) { TestKlass1.new } - it "skips tracker when apm is not enabled" do - Datadog.expects(:tracer).never - klass.send(:pub_method) - end - end - - ENV.store("DATADOG_TRACER", "1") - class TestKlass2 - include SamsonDatadogTracer::APM - SamsonDatadogTracer::APM.module_eval { include Datadog } - - def pub_method - :pub - end - - protected - - def pro_method - :pro - end - - private - - def pri_method - :pri - end - - trace_method :pub_method - trace_method :pri_method - trace_method :pro_method - trace_method :not_method - alias_method :pub_method_with_apm_untracer, :pub_method - end - - describe ".trace_method" do - let(:apm) { TestKlass2.new } - Datadog.expects(:tracer).returns(Datadog.tracer) - - it "wraps the private method in a trace call" do - apm.send(:pri_method).must_equal(:pri) - end - - it "wraps the public method in a trace call" do - apm.send(:pub_method).must_equal(:pub) - end - - it "raises with NoMethodError when undefined method call" do - assert_raise NoMethodError do - apm.send(:not_method) - end - end - - it "preserves method visibility" do - assert apm.class.public_method_defined?(:pub_method) - refute apm.class.public_method_defined?(:pri_method) - assert apm.class.private_method_defined?(:pri_method) - end - end - - describe "#Alias methods" do - let(:apm) { TestKlass2.new } - it "defines alias methods for trace" do - assert apm.class.method_defined?("pub_method_with_apm_tracer") - assert apm.class.private_method_defined?("pri_method_with_apm_tracer") - end - - it "defines alias methods for untrace" do - assert apm.class.method_defined?("pub_method_with_apm_untracer") - refute apm.class.private_method_defined?("pri_method_with_apm_untracer") - end - - it "reponds to alias methods" do - apm.send(:pub_method_with_apm_tracer).must_equal(:pub) - end - end - - describe "#Helpers" do - it "returns sanitize name" do - SamsonDatadogTracer::APM::Helpers.sanitize_name("Test@123").must_equal("test_123") - end - - it "returns tracer method name" do - SamsonDatadogTracer::APM::Helpers.tracer_method_name("test_method").must_equal("test_method_with_apm_tracer") - end - - it "returns untracer method name" do - SamsonDatadogTracer::APM::Helpers.untracer_method_name("test_method").must_equal("test_method_with_apm_untracer") - end - end -end diff --git a/plugins/datadog_tracer/test/samson_datadog_tracer/samson_plugin_test.rb b/plugins/datadog_tracer/test/samson_datadog_tracer/samson_plugin_test.rb index 8fc41cf41a..a1f28ea88f 100644 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/samson_plugin_test.rb +++ b/plugins/datadog_tracer/test/samson_datadog_tracer/samson_plugin_test.rb @@ -4,55 +4,126 @@ SingleCov.covered! describe SamsonDatadogTracer do + let(:fake_tracer) do + Class.new do + def self.trace(*) + yield + end + end + end + describe ".enabled?" do - before(:all) do - ENV.delete('DATADOG_TRACER') + it "is false by default" do + refute SamsonDatadogTracer.enabled? end - context "in any environment" do - it "is false by default" do - refute SamsonDatadogTracer.enabled? + + it "is true when DATADOG_TRACER env var is set" do + with_env DATADOG_TRACER: "1" do + assert SamsonDatadogTracer.enabled? end + end + end - it "is true when DATADOG_TRACER env var is set" do - with_env DATADOG_TRACER: "1" do - assert SamsonDatadogTracer.enabled? - end - with_env DATADOG_TRACER: nil do - refute SamsonDatadogTracer.enabled? - end + describe ".trace_method_execution_scope" do + with_env DATADOG_TRACER: '1' + + it "skips tracer when disabled" do + with_env DATADOG_TRACER: nil do + Datadog.expects(:tracer).never + SamsonDatadogTracer.trace_execution_scoped("test") { "without tracer" } end end + + it "trigger tracer when enabled" do + Rails.stubs(:env).returns("staging") + Datadog.expects(:tracer).returns(fake_tracer) + SamsonDatadogTracer.trace_execution_scoped("test") { "with tracer" } + end end - describe "#performance_tracer" do - describe "when enabled" do - it "triggers Datadog tracer method" do - with_env DATADOG_TRACER: "1" do - class Klass - include ::Samson::PerformanceTracer - def with_role - end - add_tracer :with_role - end - Klass.expects(:trace_method) - Samson::Hooks.fire :performance_tracer, Klass, :with_role + describe ".trace_method" do + let(:instance) do + Class.new do + include SamsonDatadogTracer + + def pub_method + :pub + end + + protected + + def pro_method + :pro + end + + private + + def pri_method + :pri end + + SamsonDatadogTracer.trace_method self, :pub_method + SamsonDatadogTracer.trace_method self, :pri_method + SamsonDatadogTracer.trace_method self, :pro_method + end.new + end + + it "wraps methods in a trace call" do + Datadog.expects(:tracer).times(3).returns(fake_tracer) + instance.send(:pub_method).must_equal(:pub) + instance.send(:pro_method).must_equal(:pro) + instance.send(:pri_method).must_equal(:pri) + end + + it "refuses to add the same wrapper twice since that would lead to infinite loops" do + e = assert_raise RuntimeError do + SamsonDatadogTracer.trace_method instance.class, :pub_method end - it "skips tracer with missing method" do - with_env DATADOG_TRACER: "1" do - helper = SamsonDatadogTracer::APM::Helpers - method = :with_role - Samson::Hooks.fire :performance_tracer, User, method - refute User.method_defined?(helper.tracer_method_name(method)) + e.message.must_include "apm_tracer wrapper already defined for pub_method" + end + + [:pub_method, :pro_method, :pri_method].each do |method| + it "refuses to add the same wrapper twice for #{method}" do + e = assert_raise RuntimeError do + SamsonDatadogTracer.trace_method instance.class, method end + e.message.must_include "apm_tracer wrapper already defined for #{method}" end end - it "skips Datadog tracer when disabled" do - with_env DATADOG_TRACER: nil do - User.expects(:trace_method).never - Samson::Hooks.fire :performance_tracer, User, :with_role + it "fails with undefined method" do + e = assert_raise NameError do + SamsonDatadogTracer.trace_method instance.class, :no_method + end + e.message.must_include "undefined method `no_method'" + end + + it "preserves method visibility" do + instance.public_methods.must_include :pub_method + instance.public_methods.wont_include :pri_method + instance.protected_methods.must_include :pro_method + instance.private_methods.must_include :pri_method + end + + it "defines alias methods for without" do + instance.public_methods.must_include :without_apm_tracer_pub_method + instance.protected_methods.must_include :without_apm_tracer_pro_method + instance.private_methods.must_include :without_apm_tracer_pri_method + end + end + + describe "trace_method hook" do + it "triggers Datadog tracer method when enabled" do + with_env DATADOG_TRACER: "1" do + Datadog.expects(:tracer).returns(fake_tracer) # hook fire triggers tracing too + SamsonDatadogTracer.expects(:trace_method) + Samson::Hooks.fire :trace_method, User, :foobar end end + + it "skips Datadog tracer when disabled" do + SamsonDatadogTracer.expects(:trace_method).never + Samson::Hooks.fire :trace_method, User, :foobar + end end end diff --git a/plugins/new_relic/README.md b/plugins/new_relic/README.md index 349f75983f..6c95e11b3f 100644 --- a/plugins/new_relic/README.md +++ b/plugins/new_relic/README.md @@ -1 +1,2 @@ -Display newrelic graphs during/after deploy. + - Display newrelic graphs during/after deploy, with `NEW_RELIC_API_KEY` + - Add Newrelic instrumentation, with `NEW_RELIC_LICENSE_KEY` diff --git a/plugins/new_relic/lib/samson_new_relic/api.rb b/plugins/new_relic/lib/samson_new_relic/api.rb index 0c234eed3a..9279815be3 100644 --- a/plugins/new_relic/lib/samson_new_relic/api.rb +++ b/plugins/new_relic/lib/samson_new_relic/api.rb @@ -21,7 +21,7 @@ def metrics(application_names, initial = false) def get(path, params = {}) response = Faraday.get("https://api.newrelic.com#{path}", params) do |request| request.options.open_timeout = 2 - request.headers['X-Api-Key'] = KEY + request.headers['X-Api-Key'] = API_KEY end raise "Newrelic request to #{path} failed #{response.status}" unless response.status == 200 JSON.parse(response.body) diff --git a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb index 8f4b66571e..37470ea266 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -3,27 +3,53 @@ module SamsonNewRelic class Engine < Rails::Engine end - KEY = ENV['NEWRELIC_API_KEY'].presence + def self.find_api_key + api_key = ENV['NEW_RELIC_API_KEY'].presence + raise "Use NEW_RELIC_API_KEY, not NEWRELIC_API_KEY" if ENV['NEWRELIC_API_KEY'] && !api_key + api_key + end + + def self.setup_initializers + if ['staging', 'production'].include?(Rails.env) + require 'newrelic_rpm' + else + # avoids circular dependencies warning + # https://discuss.newrelic.com/t/circular-require-in-ruby-agent-lib-new-relic-agent-method-tracer-rb/42737 + require 'new_relic/control' + + # needed even in dev/test mode + require 'new_relic/agent/method_tracer' + end + end + + API_KEY = find_api_key def self.enabled? - KEY + API_KEY end def self.tracer_enabled? - !!ENV['NEW_RELIC_LICENSE_KEY'] + !!ENV['NEW_RELIC_LICENSE_KEY'] # same key as the newrelic_rpm gem uses end - def self.trace_method_execution_scope(scope_name) + def self.trace_execution_scoped(scope_name, &block) if tracer_enabled? - NewRelic::Agent::MethodTracerHelpers.trace_execution_scoped("Custom/Hooks/#{scope_name}") do - yield - end + NewRelic::Agent::MethodTracerHelpers.trace_execution_scoped("Custom/Hooks/#{scope_name}", &block) else yield end end + + def self.include_once(klass, mod) + klass.include mod unless klass.include?(mod) + end end +# Railties need to be loaded before the application is initialized +SamsonNewRelic.setup_initializers +require 'samson/performance_tracer' +Samson::PerformanceTracer.handlers << SamsonNewRelic + Samson::Hooks.view :stage_form, "samson_new_relic/fields" Samson::Hooks.view :deploy_tab_nav, "samson_new_relic/deploy_tab_nav" Samson::Hooks.view :deploy_tab_body, "samson_new_relic/deploy_tab_body" @@ -39,20 +65,16 @@ def self.trace_method_execution_scope(scope_name) new_stage.new_relic_applications.build(old_applications) end -Samson::Hooks.callback :performance_tracer do |klass, method| +Samson::Hooks.callback :trace_method do |klass, method| if SamsonNewRelic.tracer_enabled? - klass.class_eval do - include ::NewRelic::Agent::MethodTracer - add_method_tracer method - end + SamsonNewRelic.include_once klass, ::NewRelic::Agent::MethodTracer + klass.add_method_tracer method end end Samson::Hooks.callback :asynchronous_performance_tracer do |klass, method, options| if SamsonNewRelic.tracer_enabled? - klass.is_a?(Class) && klass.class_eval do - include ::NewRelic::Agent::Instrumentation::ControllerInstrumentation - add_transaction_tracer method, options - end + SamsonNewRelic.include_once klass, ::NewRelic::Agent::Instrumentation::ControllerInstrumentation + klass.add_transaction_tracer method, options end end diff --git a/plugins/new_relic/samson_new_relic.gemspec b/plugins/new_relic/samson_new_relic.gemspec index 9c99bcae89..44afc4449b 100644 --- a/plugins/new_relic/samson_new_relic.gemspec +++ b/plugins/new_relic/samson_new_relic.gemspec @@ -3,4 +3,5 @@ Gem::Specification.new "samson_new_relic", "0.0.0" do |s| s.summary = "Samson NewRelic integration" s.authors = ["Michael Grosser"] s.email = "mgrosser@zendesk.com" + s.add_runtime_dependency "newrelic_rpm" end diff --git a/plugins/new_relic/test/controllers/new_relic_controller_test.rb b/plugins/new_relic/test/controllers/new_relic_controller_test.rb index a28fd2ffa2..e58bc59da4 100644 --- a/plugins/new_relic/test/controllers/new_relic_controller_test.rb +++ b/plugins/new_relic/test/controllers/new_relic_controller_test.rb @@ -25,7 +25,7 @@ end it "requires a NewReclic api key" do - silence_warnings { SamsonNewRelic::KEY = nil } + silence_warnings { SamsonNewRelic::API_KEY = nil } get :show, params: {project_id: projects(:test), stage_id: stages(:test_staging)} assert_response :precondition_failed end diff --git a/plugins/new_relic/test/helpers/new_relic_helper_test.rb b/plugins/new_relic/test/helpers/new_relic_helper_test.rb index 99a14e2114..b69160ab45 100644 --- a/plugins/new_relic/test/helpers/new_relic_helper_test.rb +++ b/plugins/new_relic/test/helpers/new_relic_helper_test.rb @@ -22,7 +22,7 @@ end it "is false when api is not configured" do - silence_warnings { SamsonNewRelic::KEY = nil } + silence_warnings { SamsonNewRelic::API_KEY = nil } refute newrelic_enabled_for_deploy? end end diff --git a/plugins/new_relic/test/samson_new_relic/samson_plugin_test.rb b/plugins/new_relic/test/samson_new_relic/samson_plugin_test.rb index f6d33d569c..7f17293f57 100644 --- a/plugins/new_relic/test/samson_new_relic/samson_plugin_test.rb +++ b/plugins/new_relic/test/samson_new_relic/samson_plugin_test.rb @@ -53,25 +53,76 @@ describe ".trace_method_execution_scope" do it "skips method trace when tracer disabled" do NewRelic::Agent::MethodTracerHelpers.expects(:trace_execution_scoped).never - SamsonNewRelic.trace_method_execution_scope("test") { "without tracer" } + SamsonNewRelic.trace_execution_scoped("test") { "without tracer" } end it "trace execution scope when enabled" do with_env NEW_RELIC_LICENSE_KEY: "1" do NewRelic::Agent::MethodTracerHelpers.expects(:trace_execution_scoped) - SamsonNewRelic.trace_method_execution_scope("test") { "with tracer" } + SamsonNewRelic.trace_execution_scoped("test") { "with tracer" } end end it "trace scope and returns execution result" do with_env NEW_RELIC_LICENSE_KEY: "1" do - SamsonNewRelic.trace_method_execution_scope("test") { "with tracer" }.must_equal("with tracer") + SamsonNewRelic.trace_execution_scoped("test") { "with tracer" }.must_equal("with tracer") end end end + describe ".find_api_key" do + it "finds no key" do + SamsonNewRelic.find_api_key.must_be_nil + end + + it "finds new key" do + with_env NEW_RELIC_API_KEY: 'foo' do + SamsonNewRelic.find_api_key.must_equal 'foo' + end + end + + it "finds new key when old is set too (for easy transition)" do + with_env NEW_RELIC_API_KEY: 'foo', NEWRELIC_API_KEY: 'foo' do + SamsonNewRelic.find_api_key.must_equal 'foo' + end + end + + it "alerts when using only old key" do + with_env NEWRELIC_API_KEY: 'foo' do + e = assert_raises(RuntimeError) { SamsonNewRelic.find_api_key } + e.message.must_equal "Use NEW_RELIC_API_KEY, not NEWRELIC_API_KEY" + end + end + end + + describe ".setup_initializers" do + it "loads basics in test mode" do + SamsonNewRelic.setup_initializers + end + + it "loads everything in staging" do + Rails.expects(:env).returns('production') + SamsonNewRelic.setup_initializers # no side-effects, but coverage will be 100% + end + end + + describe ".include_once" do + it "includes once" do + calls = 0 + a = Class.new + b = Module.new do + (class << self; self; end).define_method :included do |_| + calls += 1 + end + end + SamsonNewRelic.include_once a, b + SamsonNewRelic.include_once a, b + calls.must_equal 1 + end + end + class Klass - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers def with_role end add_tracer :with_role @@ -81,14 +132,14 @@ def with_role it "triggers method tracer when enabled" do with_env NEW_RELIC_LICENSE_KEY: "1" do Klass.expects(:add_method_tracer) - Samson::Hooks.fire :performance_tracer, Klass, [:with_role] + Samson::Hooks.fire :trace_method, Klass, [:with_role] end end it "skips method tracer when disabled" do with_env NEW_RELIC_LICENSE_KEY: nil do Klass.expects(:add_method_tracer).never - Samson::Hooks.fire :performance_tracer, Klass, [:with_role] + Samson::Hooks.fire :trace_method, Klass, [:with_role] end end end diff --git a/plugins/new_relic/test/test_helper.rb b/plugins/new_relic/test/test_helper.rb index 3c1f4f7917..93cd04af1a 100644 --- a/plugins/new_relic/test/test_helper.rb +++ b/plugins/new_relic/test/test_helper.rb @@ -4,11 +4,11 @@ ActiveSupport::TestCase.class_eval do def self.with_new_relic_plugin_enabled before do - silence_warnings { SamsonNewRelic.const_set(:KEY, '123') } + silence_warnings { SamsonNewRelic.const_set(:API_KEY, '123') } end after do - silence_warnings { SamsonNewRelic.const_set(:KEY, nil) } + silence_warnings { SamsonNewRelic.const_set(:API_KEY, nil) } SamsonNewRelic::Api.instance_variable_set(:@applications, nil) # clear cache end end diff --git a/test/lib/samson/performance_tracer_test.rb b/test/lib/samson/performance_tracer_test.rb index 316b3d3dbe..3d43d7d0fe 100644 --- a/test/lib/samson/performance_tracer_test.rb +++ b/test/lib/samson/performance_tracer_test.rb @@ -1,13 +1,12 @@ # frozen_string_literal: true -# require_relative '../../test_helper' SingleCov.covered! describe Samson::PerformanceTracer do - describe '#ClassMethods' do - class TestKlass - include Samson::PerformanceTracer + let(:klass) do + Class.new do + extend Samson::PerformanceTracer::Tracers def pub_method1 :pub1 @@ -17,60 +16,80 @@ def pub_method2 :pub2 end end + end + + describe '.trace_execution_scoped' do + let(:custom_tracer) do + Class.new do + def self.order + @order ||= [] + end - describe '.trace_execution_scoped' do - it 'add tracer for scope' do - Rails.stubs(:env).returns("staging") - trace_scope = proc {} - SamsonNewRelic.expects(:trace_method_execution_scope).returns(trace_scope) - SamsonDatadogTracer::APM.expects(:trace_method_execution_scope).returns(trace_scope) - Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped } + def self.trace_execution_scoped(scope) + order << scope + yield + ensure + order << :after + end end + end + + it 'calls all tracers' do + SamsonDatadogTracer.expects(:enabled?) + SamsonNewRelic.expects(:tracer_enabled?) + Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped }.must_equal :scoped + end - it 'skips scope tracing' do - SamsonNewRelic.expects(:trace_method_execution_scope).never - SamsonDatadogTracer::APM.expects(:trace_method_execution_scope).never - Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped }.must_equal(:scoped) + it "calls tracer in correct order" do + begin + Samson::PerformanceTracer.handlers << custom_tracer + Samson::PerformanceTracer.trace_execution_scoped('test_scope') do + custom_tracer.order << :inner + :scoped + end.must_equal :scoped + custom_tracer.order.must_equal ['test_scope', :inner, :after] + ensure + Samson::PerformanceTracer.handlers.pop end end + end - describe '.add_tracer' do - it 'add method tracer from performance_tracer hook' do - performance_tracer_callback = ->(_, _) { true } - Rails.stubs(:env).returns("staging") - Samson::Hooks.with_callback(:performance_tracer, performance_tracer_callback) do - assert TestKlass.add_tracer(:pub_method1) - assert TestKlass.add_tracer(:pub_method2) - end + describe '.add_tracer' do + it 'add method tracer from performance_tracer hook' do + performance_tracer_callback = ->(_, _) { true } + Rails.stubs(:env).returns("staging") + Samson::Hooks.with_callback(:trace_method, performance_tracer_callback) do + assert klass.add_tracer(:pub_method1) + assert klass.add_tracer(:pub_method2) end + end - it 'raises with invalid arguments' do - performance_tracer_callback = ->(_) { true } - assert_raises ArgumentError do - Samson::Hooks.with_callback(:performance_tracer, performance_tracer_callback) do - assert TestKlass.add_tracer(:pub_method1) - end + it 'raises with invalid arguments' do + performance_tracer_callback = ->(_) { true } + assert_raises ArgumentError do + Samson::Hooks.with_callback(:trace_method, performance_tracer_callback) do + assert klass.add_tracer(:pub_method1) end end end + end - describe '.add_asynchronous_tracer' do - it 'add asynchronous tracer from asynchronous_performance_tracer hook' do - methods = [:pub_method1, :pub_method2] - asyn_tracer_callback = ->(_, _, _) { true } + describe '.add_asynchronous_tracer' do + it 'add asynchronous tracer from asynchronous_performance_tracer hook' do + methods = [:pub_method1, :pub_method2] + asyn_tracer_callback = ->(_, _, _) { true } - Samson::Hooks.with_callback(:asynchronous_performance_tracer, asyn_tracer_callback) do - assert TestKlass.add_asynchronous_tracer(methods, {}) - end + Samson::Hooks.with_callback(:asynchronous_performance_tracer, asyn_tracer_callback) do + assert klass.add_asynchronous_tracer(methods, {}) end + end - it 'raises with invalid arguments' do - methods = [:pub_method1] - asyn_tracer_callback = ->(_, _) { true } - assert_raises ArgumentError do - Samson::Hooks.with_callback(:asynchronous_performance_tracer, asyn_tracer_callback) do - assert TestKlass.add_asynchronous_tracer(methods, {}) - end + it 'raises with invalid arguments' do + methods = [:pub_method1] + asyn_tracer_callback = ->(_, _) { true } + assert_raises ArgumentError do + Samson::Hooks.with_callback(:asynchronous_performance_tracer, asyn_tracer_callback) do + assert klass.add_asynchronous_tracer(methods, {}) end end end