From 0123e69cb4d4916b5009a5563b2e65f1f7c7d815 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 22 May 2018 13:02:51 +0200 Subject: [PATCH 1/8] Add ability to net/http to override servicename. +fix documentation --- docs/GettingStarted.md | 2 +- lib/ddtrace/contrib/http/patcher.rb | 5 ++++- spec/ddtrace/contrib/http/patcher_spec.rb | 6 ++++++ 3 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 spec/ddtrace/contrib/http/patcher_spec.rb diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md index b5f9d7cbe2d..5d618313da2 100644 --- a/docs/GettingStarted.md +++ b/docs/GettingStarted.md @@ -514,7 +514,7 @@ Where `options` is an optional `Hash` that accepts the following parameters: | Key | Description | Default | | --- | --- | --- | -| ``service_name`` | Service name used for `http` instrumentation | http | +| ``service_name`` | Service name used for `http` instrumentation | net/http | | ``distributed_tracing`` | Enables distributed tracing | ``false`` | If you wish to configure each connection object individually, you may use the ``Datadog.configure`` as it follows: diff --git a/lib/ddtrace/contrib/http/patcher.rb b/lib/ddtrace/contrib/http/patcher.rb index aa176c93595..355e6fb8ed3 100644 --- a/lib/ddtrace/contrib/http/patcher.rb +++ b/lib/ddtrace/contrib/http/patcher.rb @@ -48,6 +48,7 @@ module Patcher include Base register_as :http, auto_patch: true option :distributed_tracing, default: false + option :service_name, default: SERVICE @patched = false @@ -90,7 +91,9 @@ def patch_http end def initialize(*args) - pin = Datadog::Pin.new(SERVICE, app: APP, app_type: Datadog::Ext::AppTypes::WEB) + service = Datadog.configuration[:http][:service_name] + + pin = Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB) pin.onto(self) initialize_without_datadog(*args) end diff --git a/spec/ddtrace/contrib/http/patcher_spec.rb b/spec/ddtrace/contrib/http/patcher_spec.rb new file mode 100644 index 00000000000..4ef0373a985 --- /dev/null +++ b/spec/ddtrace/contrib/http/patcher_spec.rb @@ -0,0 +1,6 @@ +require 'spec_helper' +require 'ddtrace/contrib/http/patcher' + +RSpec.describe Datadog::Contrib::HTTP::Patcher do + +end \ No newline at end of file From 322249a7fb61b2d517409e8284126f034a71f962 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 22 May 2018 16:18:40 +0200 Subject: [PATCH 2/8] Add tracer option to net/http integration. and fix rentrancy for patching --- docs/GettingStarted.md | 2 + lib/ddtrace/contrib/http/patcher.rb | 22 +++++---- spec/ddtrace/contrib/http/patcher_spec.rb | 54 +++++++++++++++++++++-- 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md index 5d618313da2..0a84ba3579c 100644 --- a/docs/GettingStarted.md +++ b/docs/GettingStarted.md @@ -516,6 +516,8 @@ Where `options` is an optional `Hash` that accepts the following parameters: | --- | --- | --- | | ``service_name`` | Service name used for `http` instrumentation | net/http | | ``distributed_tracing`` | Enables distributed tracing | ``false`` | +| ``tracer`` | A ``Datadog::Tracer`` instance used to instrument the application. Usually you don't need to set that. | ``Datadog.tracer`` | + If you wish to configure each connection object individually, you may use the ``Datadog.configure`` as it follows: diff --git a/lib/ddtrace/contrib/http/patcher.rb b/lib/ddtrace/contrib/http/patcher.rb index 355e6fb8ed3..9eebc935bab 100644 --- a/lib/ddtrace/contrib/http/patcher.rb +++ b/lib/ddtrace/contrib/http/patcher.rb @@ -49,6 +49,7 @@ module Patcher register_as :http, auto_patch: true option :distributed_tracing, default: false option :service_name, default: SERVICE + option :tracer, default: Datadog.tracer @patched = false @@ -65,7 +66,7 @@ def patch require 'ddtrace/ext/net' require 'ddtrace/ext/distributed' - patch_http() + patch_http @patched = true rescue StandardError => e @@ -75,7 +76,7 @@ def patch @patched end - # patched? tells wether patch has been successfully applied + # patched? tells whether patch has been successfully applied def patched? @patched end @@ -85,29 +86,34 @@ def patched? # rubocop:disable Metrics/AbcSize def patch_http ::Net::HTTP.class_eval do - alias_method :initialize_without_datadog, :initialize + alias_method :initialize_without_datadog, :initialize unless private_instance_methods.include?(:initialize_without_datadog) + Datadog::Patcher.without_warnings do - remove_method :initialize + remove_method :initialize if private_instance_methods(false).include?(:initialize) end def initialize(*args) service = Datadog.configuration[:http][:service_name] + tracer = Datadog.configuration[:http][:tracer] - pin = Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB) + pin = Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB, tracer: tracer) pin.onto(self) initialize_without_datadog(*args) end - alias_method :request_without_datadog, :request + alias_method :request_without_datadog, :request unless method_defined?(:request_without_datadog) remove_method :request def request(req, body = nil, &block) # :yield: +response+ pin = Datadog::Pin.get_from(self) + return request_without_datadog(req, body, &block) unless pin && pin.tracer transport = pin.tracer.writer.transport - return request_without_datadog(req, body, &block) if - Datadog::Contrib::HTTP.should_skip_tracing?(req, @address, @port, transport, pin) + + if Datadog::Contrib::HTTP.should_skip_tracing?(req, @address, @port, transport, pin) + return request_without_datadog(req, body, &block) + end pin.tracer.trace(NAME) do |span| begin diff --git a/spec/ddtrace/contrib/http/patcher_spec.rb b/spec/ddtrace/contrib/http/patcher_spec.rb index 4ef0373a985..a7bae73b25f 100644 --- a/spec/ddtrace/contrib/http/patcher_spec.rb +++ b/spec/ddtrace/contrib/http/patcher_spec.rb @@ -1,6 +1,52 @@ require 'spec_helper' -require 'ddtrace/contrib/http/patcher' +require 'ddtrace' +require 'net/http' -RSpec.describe Datadog::Contrib::HTTP::Patcher do - -end \ No newline at end of file +RSpec.describe 'net/http patcher' do + let(:tracer) { ::Datadog::Tracer.new(writer: FauxWriter.new) } + + before do + WebMock.disable_net_connect!(allow_localhost: true) + WebMock.enable! + + stub_request(:any, /example.com/) + tracer.writer.spans(:clean) + Datadog.registry[:http].instance_variable_set('@patched', false) + + Datadog.configure do |c| + c.use :http, tracer: tracer + end + end + + before :each do + tracer.writer.spans(:clean) + end + + let(:request_span) do + tracer.writer.spans(:keep).find { |span| span.name == Datadog::Contrib::HTTP::NAME } + end + + describe 'with default configuration' do + it 'uses default service name' do + Net::HTTP.get('example.com', '/index.html').to_s + + expect(request_span.service).to eq('net/http') + end + end + + describe 'with changed service name' do + let(:new_service_name) { 'new_service_name' } + + before do + Datadog.configure do |c| + c.use :http, tracer: tracer, service_name: new_service_name + end + end + + it 'uses new service name' do + Net::HTTP.get('example.com', '/index.html').to_s + + expect(request_span.service).to eq(new_service_name) + end + end +end From e3c92bad5f05fc28ce48e56b5dcefdb68dc056a5 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 22 May 2018 16:21:52 +0200 Subject: [PATCH 3/8] Cleanup tests --- lib/ddtrace/contrib/http/patcher.rb | 3 ++- spec/ddtrace/contrib/http/patcher_spec.rb | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/ddtrace/contrib/http/patcher.rb b/lib/ddtrace/contrib/http/patcher.rb index 9eebc935bab..34f10b70d26 100644 --- a/lib/ddtrace/contrib/http/patcher.rb +++ b/lib/ddtrace/contrib/http/patcher.rb @@ -86,7 +86,8 @@ def patched? # rubocop:disable Metrics/AbcSize def patch_http ::Net::HTTP.class_eval do - alias_method :initialize_without_datadog, :initialize unless private_instance_methods.include?(:initialize_without_datadog) + alias_method :initialize_without_datadog, :initialize unless private_instance_methods + .include?(:initialize_without_datadog) Datadog::Patcher.without_warnings do remove_method :initialize if private_instance_methods(false).include?(:initialize) diff --git a/spec/ddtrace/contrib/http/patcher_spec.rb b/spec/ddtrace/contrib/http/patcher_spec.rb index a7bae73b25f..87103aff2e0 100644 --- a/spec/ddtrace/contrib/http/patcher_spec.rb +++ b/spec/ddtrace/contrib/http/patcher_spec.rb @@ -4,15 +4,15 @@ RSpec.describe 'net/http patcher' do let(:tracer) { ::Datadog::Tracer.new(writer: FauxWriter.new) } + let(:host) { 'example.com' } before do WebMock.disable_net_connect!(allow_localhost: true) WebMock.enable! - stub_request(:any, /example.com/) - tracer.writer.spans(:clean) - Datadog.registry[:http].instance_variable_set('@patched', false) + stub_request(:any, host) + Datadog.registry[:http].instance_variable_set('@patched', false) Datadog.configure do |c| c.use :http, tracer: tracer end @@ -28,7 +28,7 @@ describe 'with default configuration' do it 'uses default service name' do - Net::HTTP.get('example.com', '/index.html').to_s + Net::HTTP.get(host) expect(request_span.service).to eq('net/http') end @@ -44,7 +44,7 @@ end it 'uses new service name' do - Net::HTTP.get('example.com', '/index.html').to_s + Net::HTTP.get(host) expect(request_span.service).to eq(new_service_name) end From 9c06a46be6e9471a6effcd9416fc28648ee66531 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 22 May 2018 16:45:53 +0200 Subject: [PATCH 4/8] Include tests for net/http instrumentation in test runs --- Rakefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Rakefile b/Rakefile index 6c54ae502e2..448c54797ad 100644 --- a/Rakefile +++ b/Rakefile @@ -232,11 +232,13 @@ task :ci do sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:graphql' sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:racecar' sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:redis' + sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:http' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:active_record' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:active_support' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:dalli' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:faraday' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:redis' + sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:http' when 2 sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:sidekiq' sh 'rvm $SIDEKIQ_OLD_VERSIONS --verbose do appraisal contrib-old rake test:sidekiq' From 7960960dee63dd1c49e495f2bca4511b7bd81f29 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 22 May 2018 18:47:48 +0200 Subject: [PATCH 5/8] Cleanup code after configuration reset fix --- lib/ddtrace/contrib/http/patcher.rb | 5 ++--- spec/ddtrace/contrib/http/patcher_spec.rb | 10 +++------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/ddtrace/contrib/http/patcher.rb b/lib/ddtrace/contrib/http/patcher.rb index 34f10b70d26..a63fbd10117 100644 --- a/lib/ddtrace/contrib/http/patcher.rb +++ b/lib/ddtrace/contrib/http/patcher.rb @@ -86,8 +86,7 @@ def patched? # rubocop:disable Metrics/AbcSize def patch_http ::Net::HTTP.class_eval do - alias_method :initialize_without_datadog, :initialize unless private_instance_methods - .include?(:initialize_without_datadog) + alias_method :initialize_without_datadog, :initialize Datadog::Patcher.without_warnings do remove_method :initialize if private_instance_methods(false).include?(:initialize) @@ -102,7 +101,7 @@ def initialize(*args) initialize_without_datadog(*args) end - alias_method :request_without_datadog, :request unless method_defined?(:request_without_datadog) + alias_method :request_without_datadog, :request remove_method :request def request(req, body = nil, &block) # :yield: +response+ diff --git a/spec/ddtrace/contrib/http/patcher_spec.rb b/spec/ddtrace/contrib/http/patcher_spec.rb index 87103aff2e0..bc2c4b7d4c6 100644 --- a/spec/ddtrace/contrib/http/patcher_spec.rb +++ b/spec/ddtrace/contrib/http/patcher_spec.rb @@ -12,23 +12,19 @@ stub_request(:any, host) - Datadog.registry[:http].instance_variable_set('@patched', false) + Datadog.registry[:http].reset_options! Datadog.configure do |c| c.use :http, tracer: tracer end end - before :each do - tracer.writer.spans(:clean) - end - let(:request_span) do tracer.writer.spans(:keep).find { |span| span.name == Datadog::Contrib::HTTP::NAME } end describe 'with default configuration' do it 'uses default service name' do - Net::HTTP.get(host) + Net::HTTP.get(host, '/') expect(request_span.service).to eq('net/http') end @@ -44,7 +40,7 @@ end it 'uses new service name' do - Net::HTTP.get(host) + Net::HTTP.get(host, '/') expect(request_span.service).to eq(new_service_name) end From 698c45bc13e3b3b72d39a5fd20d195f34dd78cb5 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 23 May 2018 12:11:11 +0200 Subject: [PATCH 6/8] Lazy pin initialization - remove need to override #initialize --- lib/ddtrace/contrib/http/patcher.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/ddtrace/contrib/http/patcher.rb b/lib/ddtrace/contrib/http/patcher.rb index a63fbd10117..aa3b8e9f8ba 100644 --- a/lib/ddtrace/contrib/http/patcher.rb +++ b/lib/ddtrace/contrib/http/patcher.rb @@ -86,26 +86,24 @@ def patched? # rubocop:disable Metrics/AbcSize def patch_http ::Net::HTTP.class_eval do - alias_method :initialize_without_datadog, :initialize + alias_method :request_without_datadog, :request + remove_method :request - Datadog::Patcher.without_warnings do - remove_method :initialize if private_instance_methods(false).include?(:initialize) + def datadog_fetch_pin + Datadog::Pin.get_from(self) || datadog_initialize_pin end - def initialize(*args) + def datadog_initialize_pin service = Datadog.configuration[:http][:service_name] tracer = Datadog.configuration[:http][:tracer] pin = Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB, tracer: tracer) pin.onto(self) - initialize_without_datadog(*args) + pin end - alias_method :request_without_datadog, :request - remove_method :request - def request(req, body = nil, &block) # :yield: +response+ - pin = Datadog::Pin.get_from(self) + pin = datadog_fetch_pin return request_without_datadog(req, body, &block) unless pin && pin.tracer From c06aa243b7a7b9447575a9a90fffa94d977a6296 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 23 May 2018 12:12:32 +0200 Subject: [PATCH 7/8] Rearrange Rakefile in correct order --- Rakefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Rakefile b/Rakefile index 448c54797ad..63e1895c796 100644 --- a/Rakefile +++ b/Rakefile @@ -230,15 +230,15 @@ task :ci do sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:dalli' sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:faraday' sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:graphql' + sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:http' sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:racecar' sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:redis' - sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:http' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:active_record' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:active_support' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:dalli' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:faraday' - sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:redis' sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:http' + sh 'rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake spec:redis' when 2 sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:sidekiq' sh 'rvm $SIDEKIQ_OLD_VERSIONS --verbose do appraisal contrib-old rake test:sidekiq' From f82706a76ddf94f10d345554f06be8930601e2fa Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 23 May 2018 12:17:43 +0200 Subject: [PATCH 8/8] use datadog_pin method --- lib/ddtrace/contrib/http/patcher.rb | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/ddtrace/contrib/http/patcher.rb b/lib/ddtrace/contrib/http/patcher.rb index aa3b8e9f8ba..98da5b0e88b 100644 --- a/lib/ddtrace/contrib/http/patcher.rb +++ b/lib/ddtrace/contrib/http/patcher.rb @@ -89,22 +89,17 @@ def patch_http alias_method :request_without_datadog, :request remove_method :request - def datadog_fetch_pin - Datadog::Pin.get_from(self) || datadog_initialize_pin - end - - def datadog_initialize_pin - service = Datadog.configuration[:http][:service_name] - tracer = Datadog.configuration[:http][:tracer] + def datadog_pin + @datadog_pindatadog_pin ||= begin + service = Datadog.configuration[:http][:service_name] + tracer = Datadog.configuration[:http][:tracer] - pin = Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB, tracer: tracer) - pin.onto(self) - pin + Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB, tracer: tracer) + end end def request(req, body = nil, &block) # :yield: +response+ - pin = datadog_fetch_pin - + pin = datadog_pin return request_without_datadog(req, body, &block) unless pin && pin.tracer transport = pin.tracer.writer.transport