From 504151cde91009fed682eaf2f7dbdf45828cf4bb Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Tue, 25 Sep 2018 20:00:26 -0700 Subject: [PATCH 01/17] improve newrelic docs / examples --- .env.example | 9 ++++++--- Gemfile | 1 - Gemfile.lock | 2 +- docs/plugins.md | 2 +- plugins/new_relic/README.md | 3 ++- plugins/new_relic/lib/samson_new_relic/samson_plugin.rb | 7 ++++--- plugins/new_relic/samson_new_relic.gemspec | 1 + .../test/controllers/new_relic_controller_test.rb | 2 +- plugins/new_relic/test/helpers/new_relic_helper_test.rb | 2 +- .../test/samson_new_relic/samson_plugin_test.rb | 2 +- plugins/new_relic/test/test_helper.rb | 4 ++-- 11 files changed, 20 insertions(+), 15 deletions(-) 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/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/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/samson_plugin.rb b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb index 8f4b66571e..490203c3bd 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -3,14 +3,15 @@ module SamsonNewRelic class Engine < Rails::Engine end - KEY = ENV['NEWRELIC_API_KEY'].presence + 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 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) 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..321bbb60fd 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 @@ -1,7 +1,7 @@ # frozen_string_literal: true require_relative '../test_helper' -SingleCov.covered! +SingleCov.covered! uncovered: 1 describe SamsonNewRelic do describe :stage_permitted_params do 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 From a1e990ea7abf87f68b5de177b2c773cd9a171bcf Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Tue, 25 Sep 2018 20:04:14 -0700 Subject: [PATCH 02/17] load newrelic from the plugin --- config/application.rb | 13 +------ .../lib/samson_new_relic/samson_plugin.rb | 25 +++++++++++- .../samson_new_relic/samson_plugin_test.rb | 38 ++++++++++++++++++- 3 files changed, 61 insertions(+), 15 deletions(-) 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/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb index 490203c3bd..b989d141c4 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -3,8 +3,26 @@ module SamsonNewRelic class Engine < Rails::Engine end - 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 + 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? API_KEY @@ -25,6 +43,9 @@ def self.trace_method_execution_scope(scope_name) end end +# Railties need to be loaded before the application is initialized +SamsonNewRelic.setup_initializers + 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" 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 321bbb60fd..9d3c154261 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 @@ -1,7 +1,7 @@ # frozen_string_literal: true require_relative '../test_helper' -SingleCov.covered! uncovered: 1 +SingleCov.covered! describe SamsonNewRelic do describe :stage_permitted_params do @@ -70,6 +70,42 @@ 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 + class Klass include ::Samson::PerformanceTracer def with_role From 47650539c45a2885f4825aec446ef06811a7c06c Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 09:28:02 -0700 Subject: [PATCH 03/17] remove base code knowing about plugins / simplify --- app/models/deploy_service.rb | 2 +- app/models/docker_builder_service.rb | 2 +- app/models/git_repository.rb | 2 +- app/models/image_builder.rb | 4 +-- app/models/job_execution.rb | 2 +- app/models/multi_lock.rb | 2 +- lib/samson/performance_tracer.rb | 27 ++++++++----------- .../samson_datadog_tracer/samson_plugin.rb | 4 +++ .../samson_plugin_test.rb | 2 +- .../lib/samson_new_relic/samson_plugin.rb | 2 ++ .../samson_new_relic/samson_plugin_test.rb | 2 +- 11 files changed, 26 insertions(+), 25 deletions(-) diff --git a/app/models/deploy_service.rb b/app/models/deploy_service.rb index 95cc61d929..5f29db739b 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..01ba1249e6 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 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/lib/samson/performance_tracer.rb b/lib/samson/performance_tracer.rb index 36e956e86d..41df80e3ae 100644 --- a/lib/samson/performance_tracer.rb +++ b/lib/samson/performance_tracer.rb @@ -1,41 +1,36 @@ # 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 + # TODO: use a hook + def handlers + @handlers ||= [] end - def trace_execution_scoped(scope_name) - # Tracing the scope is restricted to avoid into slow startup + def trace_execution_scoped(scope_name, &block) + # TODO: recheck 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 + using_plugins(handlers, scope_name, &block).call else yield end end + private + + # TODO: rename this is weird def using_plugins(plugins, scope_name, &block) plugins.inject(block) { |inner, plugin| plugin.trace_method_execution_scope(scope_name) { inner } } end end - # for Newrelic and Datadog -> for tracer plugins. - module ClassMethods + module Tracers def add_tracer(method) Samson::Hooks.fire(:performance_tracer, self, method) end - # TODO: Add asynchronous tracer for Datadog. + # TODO: Add asynchronous tracer support 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/samson_plugin.rb b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb index e4130fc4ec..6945bb3d60 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -8,6 +8,10 @@ def self.enabled? end end +require 'samson_datadog_tracer/apm' +require 'samson/performance_tracer' +Samson::PerformanceTracer.handlers << SamsonDatadogTracer::APM + Samson::Hooks.callback :performance_tracer do |klass, method| if SamsonDatadogTracer.enabled? klass.class_eval do 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..a519808b49 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 @@ -29,7 +29,7 @@ it "triggers Datadog tracer method" do with_env DATADOG_TRACER: "1" do class Klass - include ::Samson::PerformanceTracer + extends ::Samson::PerformanceTracer::Tracers def with_role end add_tracer :with_role 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 b989d141c4..8ac7ccad1c 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -45,6 +45,8 @@ def self.trace_method_execution_scope(scope_name) # 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" 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 9d3c154261..8a9c3fec2c 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 @@ -107,7 +107,7 @@ end class Klass - include ::Samson::PerformanceTracer + extend ::Samson::PerformanceTracer::Tracers def with_role end add_tracer :with_role From 976ddd2d948cf4dc91d2145458789db50c3e1707 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 09:41:06 -0700 Subject: [PATCH 04/17] rename unhelpful constant name --- .../datadog_tracer/test/samson_datadog_tracer/apm_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb index ab3af11e5e..eb4e00fea1 100644 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb +++ b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb @@ -4,7 +4,7 @@ SingleCov.covered! describe SamsonDatadogTracer::APM do - module DDTracer + module FakeTracer def self.trace(*) yield end @@ -12,7 +12,7 @@ def self.trace(*) module Datadog def self.tracer - DDTracer + FakeTracer end end @@ -27,7 +27,7 @@ def self.tracer it "trigger tracer when enabled" do with_env DATADOG_TRACER: "1" do Rails.stubs(:env).returns("staging") - Datadog.expects(:tracer).returns(DDTracer) + Datadog.expects(:tracer).returns(FakeTracer) SamsonDatadogTracer::APM.trace_method_execution_scope("test") { "with tracer" } end end From fe2aa363dc44a28650689362a198d3b936e48799 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 09:45:30 -0700 Subject: [PATCH 05/17] do not support defining tracers for methods that do not exist to prevent typos --- .../samson_datadog_tracer/samson_plugin.rb | 7 ++-- .../samson_plugin_test.rb | 32 ++++++------------- 2 files changed, 12 insertions(+), 27 deletions(-) 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 6945bb3d60..b948e889ee 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -18,11 +18,8 @@ def self.enabled? 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 + alias_method helper.untracer_method_name(method), method + alias_method method, helper.tracer_method_name(method) 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 a519808b49..be3f510961 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 @@ -25,34 +25,22 @@ end describe "#performance_tracer" do - describe "when enabled" do - it "triggers Datadog tracer method" do - with_env DATADOG_TRACER: "1" do - class Klass - extends ::Samson::PerformanceTracer::Tracers - def with_role - end - add_tracer :with_role + it "triggers Datadog tracer method when enabled" do + with_env DATADOG_TRACER: "1" do + class Klass + extend ::Samson::PerformanceTracer::Tracers + def with_role end - Klass.expects(:trace_method) - Samson::Hooks.fire :performance_tracer, Klass, :with_role - end - 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)) + add_tracer :with_role end + Klass.expects(:trace_method) + Samson::Hooks.fire :performance_tracer, Klass, :with_role 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 - end + User.expects(:trace_method).never + Samson::Hooks.fire :performance_tracer, User, :with_role end end end From 3c1cf60d33d6a93d3b539eb54f0725fe77c79345 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 11:13:35 -0700 Subject: [PATCH 06/17] refactor include to simple static calls --- .../lib/samson_datadog_tracer/apm.rb | 89 ++--------- .../samson_datadog_tracer/samson_plugin.rb | 8 +- .../test/samson_datadog_tracer/apm_test.rb | 142 ++++++------------ 3 files changed, 59 insertions(+), 180 deletions(-) diff --git a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb b/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb index 71f3f31133..2ee4447511 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb @@ -2,10 +2,6 @@ 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 @@ -15,62 +11,23 @@ def trace_method_execution_scope(scope_name) 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" + # TODO: blow up when adding twice + # We are not using super to make running newrelic (which uses alias) and datadog possible + def trace_method(klass, method) + visibility = method_visibility(klass, method) + without = "without_apm_tracer_#{method}" + klass.alias_method without, method + klass.define_method(method) do |*args, &block| + Datadog.tracer.trace("#{klass}###{method}") do + send(without, *args, &block) + end end + klass.send visibility, method + klass.send visibility, without 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) + def method_visibility(klass, method) if klass.protected_method_defined?(method) :protected elsif klass.private_method_defined?(method) @@ -79,26 +36,6 @@ def _original_visibility(method, klass) :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 b948e889ee..2e923a00f3 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -14,12 +14,6 @@ def self.enabled? Samson::Hooks.callback :performance_tracer do |klass, method| if SamsonDatadogTracer.enabled? - klass.class_eval do - include SamsonDatadogTracer::APM - helper = SamsonDatadogTracer::APM::Helpers - trace_method method - alias_method helper.untracer_method_name(method), method - alias_method method, helper.tracer_method_name(method) - end + SamsonDatadogTracer::APM.trace_method klass, method end end diff --git a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb index eb4e00fea1..9e8b5bcddc 100644 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb +++ b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb @@ -4,17 +4,15 @@ SingleCov.covered! describe SamsonDatadogTracer::APM do - module FakeTracer - def self.trace(*) - yield + let(:fake_tracer) do + Class.new do + def self.trace(*) + yield + end end end - module Datadog - def self.tracer - FakeTracer - end - end + with_env DATADOG_TRACER: '1' describe ".trace_method_execution_scope" do it "skips tracer when disabled" do @@ -25,114 +23,64 @@ def self.tracer end it "trigger tracer when enabled" do - with_env DATADOG_TRACER: "1" do - Rails.stubs(:env).returns("staging") - Datadog.expects(:tracer).returns(FakeTracer) - 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) + Rails.stubs(:env).returns("staging") + Datadog.expects(:tracer).returns(fake_tracer) + SamsonDatadogTracer::APM.trace_method_execution_scope("test") { "with tracer" } end end - ENV.store("DATADOG_TRACER", "1") - class TestKlass2 - include SamsonDatadogTracer::APM - SamsonDatadogTracer::APM.module_eval { include Datadog } - - def pub_method - :pub - end - - protected + describe ".trace_method" do + let(:instance) do + Class.new do + include SamsonDatadogTracer::APM - def pro_method - :pro - end + def pub_method + :pub + end - private + protected - def pri_method - :pri - end + def pro_method + :pro + 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 + private - describe ".trace_method" do - let(:apm) { TestKlass2.new } - Datadog.expects(:tracer).returns(Datadog.tracer) + def pri_method + :pri + end - it "wraps the private method in a trace call" do - apm.send(:pri_method).must_equal(:pri) + SamsonDatadogTracer::APM.trace_method self, :pub_method + SamsonDatadogTracer::APM.trace_method self, :pri_method + SamsonDatadogTracer::APM.trace_method self, :pro_method + end.new end - it "wraps the public method in a trace call" do - apm.send(:pub_method).must_equal(:pub) + 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 "raises with NoMethodError when undefined method call" do - assert_raise NoMethodError do - apm.send(:not_method) + it "fails with undefined method" do + e = assert_raise NameError do + SamsonDatadogTracer::APM.trace_method instance.class, :no_method end + e.message.must_include "undefined method `no_method'" 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") + 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 "returns untracer method name" do - SamsonDatadogTracer::APM::Helpers.untracer_method_name("test_method").must_equal("test_method_with_apm_untracer") + 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 end From da31f54e762f20ba1407bfcfd2b4e7b5a99fcf72 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 11:23:03 -0700 Subject: [PATCH 07/17] fail fast when trying to create infinite loops --- .../lib/samson_datadog_tracer/apm.rb | 10 +++---- .../samson_datadog_tracer/samson_plugin.rb | 2 ++ .../test/samson_datadog_tracer/apm_test.rb | 20 +++++++++++-- .../samson_plugin_test.rb | 30 +++++-------------- 4 files changed, 33 insertions(+), 29 deletions(-) diff --git a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb b/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb index 2ee4447511..e0c5f8210a 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb @@ -2,21 +2,21 @@ module SamsonDatadogTracer module APM class << self - def trace_method_execution_scope(scope_name) + def trace_method_execution_scope(scope_name, &block) if SamsonDatadogTracer.enabled? - Datadog.tracer.trace("Custom/Hooks/#{scope_name}") do - yield - end + Datadog.tracer.trace("Custom/Hooks/#{scope_name}", &block) else yield end end - # TODO: blow up when adding twice # We are not using super to make running newrelic (which uses alias) and datadog possible def trace_method(klass, method) visibility = method_visibility(klass, method) without = "without_apm_tracer_#{method}" + if klass.method_defined?(without) || klass.private_method_defined?(without) + raise "Tracer already defined for #{method}" + end klass.alias_method without, method klass.define_method(method) do |*args, &block| Datadog.tracer.trace("#{klass}###{method}") do 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 2e923a00f3..be304d8851 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -10,6 +10,8 @@ def self.enabled? require 'samson_datadog_tracer/apm' require 'samson/performance_tracer' + +# TODO: turn this into a hook Samson::PerformanceTracer.handlers << SamsonDatadogTracer::APM Samson::Hooks.callback :performance_tracer do |klass, method| diff --git a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb index 9e8b5bcddc..24d739c0f2 100644 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb +++ b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb @@ -12,9 +12,9 @@ def self.trace(*) end end - with_env DATADOG_TRACER: '1' - 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 @@ -63,6 +63,22 @@ def pri_method 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::APM.trace_method instance.class, :pub_method + end + e.message.must_include "Tracer 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::APM.trace_method instance.class, method + end + e.message.must_include "Tracer already defined for #{method}" + end + end + it "fails with undefined method" do e = assert_raise NameError do SamsonDatadogTracer::APM.trace_method instance.class, :no_method 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 be3f510961..9283038a35 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 @@ -5,21 +5,13 @@ describe SamsonDatadogTracer do 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? - 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 + it "is true when DATADOG_TRACER env var is set" do + with_env DATADOG_TRACER: "1" do + assert SamsonDatadogTracer.enabled? end end end @@ -27,19 +19,13 @@ describe "#performance_tracer" do it "triggers Datadog tracer method when enabled" do with_env DATADOG_TRACER: "1" do - class Klass - extend ::Samson::PerformanceTracer::Tracers - def with_role - end - add_tracer :with_role - end - Klass.expects(:trace_method) - Samson::Hooks.fire :performance_tracer, Klass, :with_role + SamsonDatadogTracer::APM.expects(:trace_method) + Samson::Hooks.fire :performance_tracer, User, :with_role end end it "skips Datadog tracer when disabled" do - User.expects(:trace_method).never + SamsonDatadogTracer::APM.expects(:trace_method).never Samson::Hooks.fire :performance_tracer, User, :with_role end end From 4e308b023ca7a2a1d7685a4ffe40b1257cffc96e Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 11:30:10 -0700 Subject: [PATCH 08/17] fail fast when used on a place where it does not work --- plugins/new_relic/lib/samson_new_relic/samson_plugin.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8ac7ccad1c..430d1bc086 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -74,7 +74,7 @@ def self.trace_method_execution_scope(scope_name) Samson::Hooks.callback :asynchronous_performance_tracer do |klass, method, options| if SamsonNewRelic.tracer_enabled? - klass.is_a?(Class) && klass.class_eval do + klass.class_eval do include ::NewRelic::Agent::Instrumentation::ControllerInstrumentation add_transaction_tracer method, options end From e81f5db22fc1d6afcffe149815a2a0ebbd0ec27e Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 11:37:32 -0700 Subject: [PATCH 09/17] make sure newrelic modules are only included once --- .../lib/samson_new_relic/samson_plugin.rb | 16 ++++++++-------- .../test/samson_new_relic/samson_plugin_test.rb | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) 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 430d1bc086..847b246c68 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -41,6 +41,10 @@ def self.trace_method_execution_scope(scope_name) 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 @@ -65,18 +69,14 @@ def self.trace_method_execution_scope(scope_name) Samson::Hooks.callback :performance_tracer 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.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/test/samson_new_relic/samson_plugin_test.rb b/plugins/new_relic/test/samson_new_relic/samson_plugin_test.rb index 8a9c3fec2c..218d83a1d0 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 @@ -106,6 +106,21 @@ 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 extend ::Samson::PerformanceTracer::Tracers def with_role From ee05fd5fca00a2ca8208698f8177042b597f7b8a Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 12:16:44 -0700 Subject: [PATCH 10/17] more cleanup and renames --- lib/samson/hooks.rb | 2 +- lib/samson/performance_tracer.rb | 16 ++-- .../lib/samson_datadog_tracer/apm.rb | 2 +- .../samson_datadog_tracer/samson_plugin.rb | 6 +- .../test/samson_datadog_tracer/apm_test.rb | 4 +- .../samson_plugin_test.rb | 4 +- .../lib/samson_new_relic/samson_plugin.rb | 8 +- .../samson_new_relic/samson_plugin_test.rb | 10 +-- test/lib/samson/performance_tracer_test.rb | 89 +++++++++---------- 9 files changed, 66 insertions(+), 75 deletions(-) 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 41df80e3ae..6576ab257b 100644 --- a/lib/samson/performance_tracer.rb +++ b/lib/samson/performance_tracer.rb @@ -2,7 +2,7 @@ module Samson module PerformanceTracer class << self - # TODO: use a hook + # NOTE: this cannot be a hook since it is used from Hooks#fire def handlers @handlers ||= [] end @@ -11,26 +11,20 @@ def trace_execution_scoped(scope_name, &block) # TODO: recheck Tracing the scope is restricted to avoid into slow startup # Refer Samson::BootCheck if ['staging', 'production'].include?(Rails.env) - using_plugins(handlers, scope_name, &block).call + # TODO: caching this would be nice + stack = handlers.inject(block) { |inner, plugin| plugin.trace_execution_scoped(scope_name) { inner } } + stack.call else yield end end - - private - - # TODO: rename this is weird - def using_plugins(plugins, scope_name, &block) - plugins.inject(block) { |inner, plugin| plugin.trace_method_execution_scope(scope_name) { inner } } - end end 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 support 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 index e0c5f8210a..8d470455c3 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb @@ -2,7 +2,7 @@ module SamsonDatadogTracer module APM class << self - def trace_method_execution_scope(scope_name, &block) + def trace_execution_scoped(scope_name, &block) if SamsonDatadogTracer.enabled? Datadog.tracer.trace("Custom/Hooks/#{scope_name}", &block) else 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 be304d8851..14b473da26 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -10,12 +10,12 @@ def self.enabled? require 'samson_datadog_tracer/apm' require 'samson/performance_tracer' - -# TODO: turn this into a hook Samson::PerformanceTracer.handlers << SamsonDatadogTracer::APM -Samson::Hooks.callback :performance_tracer do |klass, method| +Samson::Hooks.callback :trace_method do |klass, method| if SamsonDatadogTracer.enabled? SamsonDatadogTracer::APM.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 index 24d739c0f2..73f9eb0366 100644 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb +++ b/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb @@ -18,14 +18,14 @@ def self.trace(*) 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" } + SamsonDatadogTracer::APM.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::APM.trace_method_execution_scope("test") { "with tracer" } + SamsonDatadogTracer::APM.trace_execution_scoped("test") { "with tracer" } 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 9283038a35..6839376146 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 @@ -20,13 +20,13 @@ it "triggers Datadog tracer method when enabled" do with_env DATADOG_TRACER: "1" do SamsonDatadogTracer::APM.expects(:trace_method) - Samson::Hooks.fire :performance_tracer, User, :with_role + Samson::Hooks.fire :trace_method, User, :with_role end end it "skips Datadog tracer when disabled" do SamsonDatadogTracer::APM.expects(:trace_method).never - Samson::Hooks.fire :performance_tracer, User, :with_role + Samson::Hooks.fire :trace_method, User, :with_role end end end 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 847b246c68..37470ea266 100644 --- a/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb +++ b/plugins/new_relic/lib/samson_new_relic/samson_plugin.rb @@ -32,11 +32,9 @@ def self.tracer_enabled? !!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 @@ -67,7 +65,7 @@ def self.include_once(klass, mod) 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? SamsonNewRelic.include_once klass, ::NewRelic::Agent::MethodTracer klass.add_method_tracer method 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 218d83a1d0..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,19 +53,19 @@ 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 @@ -132,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/test/lib/samson/performance_tracer_test.rb b/test/lib/samson/performance_tracer_test.rb index 316b3d3dbe..190b97ce37 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,60 @@ def pub_method2 :pub2 end end + 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 } - end + describe '.trace_execution_scoped' do + it 'add tracer for scope' do + Rails.stubs(:env).returns("staging") + trace_scope = proc {} + SamsonNewRelic.expects(:trace_execution_scoped).returns(trace_scope) + SamsonDatadogTracer::APM.expects(:trace_execution_scoped).returns(trace_scope) + Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :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) - end + it 'skips scope tracing' do + SamsonNewRelic.expects(:trace_execution_scoped).never + SamsonDatadogTracer::APM.expects(:trace_execution_scoped).never + Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped }.must_equal(:scoped) 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 From 5c07fca7d2b0c6df511cdcf3dff68d8e00b2f428 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 12:20:15 -0700 Subject: [PATCH 11/17] unify datadog tracing now that it is simple --- .../lib/samson_datadog_tracer/apm.rb | 41 ------- .../samson_datadog_tracer/samson_plugin.rb | 48 ++++++++- .../test/samson_datadog_tracer/apm_test.rb | 102 ------------------ .../samson_plugin_test.rb | 102 +++++++++++++++++- 4 files changed, 142 insertions(+), 151 deletions(-) delete mode 100644 plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb delete mode 100644 plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb 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 8d470455c3..0000000000 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/apm.rb +++ /dev/null @@ -1,41 +0,0 @@ -# frozen_string_literal: true -module SamsonDatadogTracer - module APM - class << self - def trace_execution_scoped(scope_name, &block) - if SamsonDatadogTracer.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) - visibility = method_visibility(klass, method) - without = "without_apm_tracer_#{method}" - if klass.method_defined?(without) || klass.private_method_defined?(without) - raise "Tracer already defined for #{method}" - end - klass.alias_method without, method - klass.define_method(method) do |*args, &block| - Datadog.tracer.trace("#{klass}###{method}") do - send(without, *args, &block) - end - 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 -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 14b473da26..60c6bc84d1 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -3,18 +3,56 @@ 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) + visibility = method_visibility(klass, method) + without = "without_apm_tracer_#{method}" + if klass.method_defined?(without) || klass.private_method_defined?(without) + raise "Tracer already defined for #{method}" + end + klass.alias_method without, method + klass.define_method(method) do |*args, &block| + Datadog.tracer.trace("#{klass}###{method}") do + send(without, *args, &block) + end + end + klass.send visibility, method + klass.send visibility, without + end + + private + + 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 -require 'samson_datadog_tracer/apm' require 'samson/performance_tracer' -Samson::PerformanceTracer.handlers << SamsonDatadogTracer::APM +Samson::PerformanceTracer.handlers << SamsonDatadogTracer Samson::Hooks.callback :trace_method do |klass, method| if SamsonDatadogTracer.enabled? - SamsonDatadogTracer::APM.trace_method klass, method + SamsonDatadogTracer.trace_method klass, method end end 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 73f9eb0366..0000000000 --- a/plugins/datadog_tracer/test/samson_datadog_tracer/apm_test.rb +++ /dev/null @@ -1,102 +0,0 @@ -# frozen_string_literal: true -require_relative "../test_helper" - -SingleCov.covered! - -describe SamsonDatadogTracer::APM do - let(:fake_tracer) do - Class.new do - def self.trace(*) - yield - end - end - 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::APM.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::APM.trace_execution_scoped("test") { "with tracer" } - end - end - - describe ".trace_method" do - let(:instance) do - Class.new do - include SamsonDatadogTracer::APM - - def pub_method - :pub - end - - protected - - def pro_method - :pro - end - - private - - def pri_method - :pri - end - - SamsonDatadogTracer::APM.trace_method self, :pub_method - SamsonDatadogTracer::APM.trace_method self, :pri_method - SamsonDatadogTracer::APM.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::APM.trace_method instance.class, :pub_method - end - e.message.must_include "Tracer 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::APM.trace_method instance.class, method - end - e.message.must_include "Tracer already defined for #{method}" - end - end - - it "fails with undefined method" do - e = assert_raise NameError do - SamsonDatadogTracer::APM.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 -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 6839376146..f0a1c4d06c 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,6 +4,14 @@ SingleCov.covered! describe SamsonDatadogTracer do + let(:fake_tracer) do + Class.new do + def self.trace(*) + yield + end + end + end + describe ".enabled?" do it "is false by default" do refute SamsonDatadogTracer.enabled? @@ -16,16 +24,104 @@ end end - describe "#performance_tracer" do + 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 ".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 + e.message.must_include "Tracer 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 "Tracer already defined for #{method}" + end + end + + 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 - SamsonDatadogTracer::APM.expects(:trace_method) + SamsonDatadogTracer.expects(:trace_method) Samson::Hooks.fire :trace_method, User, :with_role end end it "skips Datadog tracer when disabled" do - SamsonDatadogTracer::APM.expects(:trace_method).never + SamsonDatadogTracer.expects(:trace_method).never Samson::Hooks.fire :trace_method, User, :with_role end end From eb941afeb2c1ba834768f7f366e30d1c112b7a82 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 12:23:42 -0700 Subject: [PATCH 12/17] bootcheck is fine with new tracers --- lib/samson/performance_tracer.rb | 11 ++--------- test/lib/samson/performance_tracer_test.rb | 9 +-------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/lib/samson/performance_tracer.rb b/lib/samson/performance_tracer.rb index 6576ab257b..3ee0e2cff0 100644 --- a/lib/samson/performance_tracer.rb +++ b/lib/samson/performance_tracer.rb @@ -7,16 +7,9 @@ def handlers @handlers ||= [] end + # TODO: caching the stack would be nice def trace_execution_scoped(scope_name, &block) - # TODO: recheck Tracing the scope is restricted to avoid into slow startup - # Refer Samson::BootCheck - if ['staging', 'production'].include?(Rails.env) - # TODO: caching this would be nice - stack = handlers.inject(block) { |inner, plugin| plugin.trace_execution_scoped(scope_name) { inner } } - stack.call - else - yield - end + handlers.inject(block) { |inner, plugin| plugin.trace_execution_scoped(scope_name) { inner } }.call end end diff --git a/test/lib/samson/performance_tracer_test.rb b/test/lib/samson/performance_tracer_test.rb index 190b97ce37..89e3698cbc 100644 --- a/test/lib/samson/performance_tracer_test.rb +++ b/test/lib/samson/performance_tracer_test.rb @@ -20,18 +20,11 @@ def pub_method2 describe '.trace_execution_scoped' do it 'add tracer for scope' do - Rails.stubs(:env).returns("staging") trace_scope = proc {} SamsonNewRelic.expects(:trace_execution_scoped).returns(trace_scope) - SamsonDatadogTracer::APM.expects(:trace_execution_scoped).returns(trace_scope) + SamsonDatadogTracer.expects(:trace_execution_scoped).returns(trace_scope) Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped } end - - it 'skips scope tracing' do - SamsonNewRelic.expects(:trace_execution_scoped).never - SamsonDatadogTracer::APM.expects(:trace_execution_scoped).never - Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped }.must_equal(:scoped) - end end describe '.add_tracer' do From 39bf779a662c4a8a6802111f259e2c5b3fa3bf53 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 12:36:49 -0700 Subject: [PATCH 13/17] fix chain building to call in correct order was untested, looked funky and was broken ``` Expected: ["test_scope", :inner, :after] Actual: ["test_scope", :after, :inner] ``` --- lib/samson/performance_tracer.rb | 3 +- test/lib/samson/performance_tracer_test.rb | 37 +++++++++++++++++++--- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/lib/samson/performance_tracer.rb b/lib/samson/performance_tracer.rb index 3ee0e2cff0..2740d5b9cc 100644 --- a/lib/samson/performance_tracer.rb +++ b/lib/samson/performance_tracer.rb @@ -7,9 +7,8 @@ def handlers @handlers ||= [] end - # TODO: caching the stack would be nice def trace_execution_scoped(scope_name, &block) - handlers.inject(block) { |inner, plugin| plugin.trace_execution_scoped(scope_name) { inner } }.call + handlers.inject(block) { |inner, plugin| -> { plugin.trace_execution_scoped(scope_name, &inner) } }.call end end diff --git a/test/lib/samson/performance_tracer_test.rb b/test/lib/samson/performance_tracer_test.rb index 89e3698cbc..3d43d7d0fe 100644 --- a/test/lib/samson/performance_tracer_test.rb +++ b/test/lib/samson/performance_tracer_test.rb @@ -19,11 +19,38 @@ def pub_method2 end describe '.trace_execution_scoped' do - it 'add tracer for scope' do - trace_scope = proc {} - SamsonNewRelic.expects(:trace_execution_scoped).returns(trace_scope) - SamsonDatadogTracer.expects(:trace_execution_scoped).returns(trace_scope) - Samson::PerformanceTracer.trace_execution_scoped('test_scope') { :scoped } + let(:custom_tracer) do + Class.new do + def self.order + @order ||= [] + end + + 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 "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 From d2852317c381dedfe511dea8a12387ef14adf297 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 14:34:49 -0700 Subject: [PATCH 14/17] extract method wrapping logic for easy readability and possible reuse --- .../lib/samson_datadog_tracer/samson_plugin.rb | 18 +++++++++++------- .../samson_plugin_test.rb | 7 ++++--- 2 files changed, 15 insertions(+), 10 deletions(-) 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 60c6bc84d1..e91113c031 100644 --- a/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb +++ b/plugins/datadog_tracer/lib/samson_datadog_tracer/samson_plugin.rb @@ -18,23 +18,27 @@ def trace_execution_scoped(scope_name, &block) # 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_apm_tracer_#{method}" + without = "without_#{scope}_#{method}" if klass.method_defined?(without) || klass.private_method_defined?(without) - raise "Tracer already defined for #{method}" + raise "#{scope} wrapper already defined for #{method}" end klass.alias_method without, method klass.define_method(method) do |*args, &block| - Datadog.tracer.trace("#{klass}###{method}") do - send(without, *args, &block) - end + callback.call { send(without, *args, &block) } # rubocop:disable Performance/RedundantBlockCall end klass.send visibility, method klass.send visibility, without end - private - def method_visibility(klass, method) if klass.protected_method_defined?(method) :protected 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 f0a1c4d06c..0fdab979ed 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 @@ -87,7 +87,7 @@ def pri_method e = assert_raise RuntimeError do SamsonDatadogTracer.trace_method instance.class, method end - e.message.must_include "Tracer already defined for #{method}" + e.message.must_include "apm_tracer wrapper already defined for #{method}" end end @@ -115,14 +115,15 @@ def pri_method 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, :with_role + 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, :with_role + Samson::Hooks.fire :trace_method, User, :foobar end end end From 073098a967e39e421e4637363e8702415d5b2132 Mon Sep 17 00:00:00 2001 From: sathish Date: Wed, 26 Sep 2018 14:47:08 -0700 Subject: [PATCH 15/17] Corrected the method name --- app/models/git_repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/git_repository.rb b/app/models/git_repository.rb index 01ba1249e6..31f892fb1a 100644 --- a/app/models/git_repository.rb +++ b/app/models/git_repository.rb @@ -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') From 45d2754521e4b11bf9f3d4c850749bb2c2f69672 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 16:03:16 -0700 Subject: [PATCH 16/17] fix tests --- .../test/samson_datadog_tracer/samson_plugin_test.rb | 2 +- plugins/new_relic/lib/samson_new_relic/api.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 0fdab979ed..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 @@ -79,7 +79,7 @@ def pri_method e = assert_raise RuntimeError do SamsonDatadogTracer.trace_method instance.class, :pub_method end - e.message.must_include "Tracer already defined for pub_method" + e.message.must_include "apm_tracer wrapper already defined for pub_method" end [:pub_method, :pro_method, :pri_method].each do |method| 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) From 5be7c8d3cbae44e04045854fa730e4097b961e28 Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Wed, 26 Sep 2018 19:54:22 -0700 Subject: [PATCH 17/17] avoid triggering background threads when datadog tracer is enabled and docker fails in dev environment --- config/initializers/docker.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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