From a804af2617f895429b920dbdd27eae9de9713fe3 Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 10:58:10 +0100 Subject: [PATCH 1/7] [presto]fix tests, setup ci properly for all relevant versions, move super invocation out of return block --- .circleci/config.yml | 3 +++ Appraisals | 5 +++++ docker-compose.yml | 21 +++++++++++++++++++ lib/ddtrace/contrib/presto/instrumentation.rb | 16 +++++++------- spec/ddtrace/contrib/presto/client_spec.rb | 2 +- 5 files changed, 38 insertions(+), 9 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 3e8e5d21c60..e31790069ee 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -15,6 +15,8 @@ test_containers: - POSTGRES_PASSWORD=postgres - POSTGRES_USER=postgres - POSTGRES_DB=postgres + - &container_presto + image: prestosql/presto - &container_mysql image: mysql:5.6 environment: @@ -164,6 +166,7 @@ orbs: - BUNDLE_GEMFILE: /app/Gemfile - TEST_DATADOG_INTEGRATION: 1 - *container_postgres + - *container_presto - *container_mysql - *container_elasticsearch - *container_redis diff --git a/Appraisals b/Appraisals index debb39c33d0..297ec1f386d 100644 --- a/Appraisals +++ b/Appraisals @@ -75,6 +75,7 @@ elsif Gem::Version.new('2.0.0') <= Gem::Version.new(RUBY_VERSION) \ gem 'hiredis' gem 'mongo', '< 2.5' gem 'mysql2', '0.3.21', platform: :ruby + gem 'presto-client', '>= 0.5.14' gem 'rack', '1.4.7' gem 'rack-cache', '1.7.1' gem 'rack-test', '0.7.0' @@ -180,6 +181,7 @@ elsif Gem::Version.new('2.1.0') <= Gem::Version.new(RUBY_VERSION) \ gem 'delayed_job' gem 'delayed_job_active_record' gem 'elasticsearch-transport' + gem 'presto-client', '>= 0.5.14' gem 'ethon' gem 'excon' gem 'hiredis' @@ -335,6 +337,7 @@ elsif Gem::Version.new('2.2.0') <= Gem::Version.new(RUBY_VERSION) \ gem 'delayed_job' gem 'delayed_job_active_record' gem 'elasticsearch-transport' + gem 'presto-client', '>= 0.5.14' gem 'ethon' gem 'excon' gem 'faraday' @@ -580,6 +583,7 @@ elsif Gem::Version.new('2.4.0') <= Gem::Version.new(RUBY_VERSION) \ gem 'hiredis' gem 'mongo', '>= 2.8.0' gem 'mysql2', '< 0.5', platform: :ruby + gem 'presto-client', '>= 0.5.14' gem 'racecar', '>= 0.3.5' gem 'rack' gem 'rack-test' @@ -691,6 +695,7 @@ elsif Gem::Version.new('2.5.0') <= Gem::Version.new(RUBY_VERSION) \ gem 'hiredis' gem 'mongo', '>= 2.8.0' gem 'mysql2', '< 0.5', platform: :ruby + gem 'presto-client', '>= 0.5.14' gem 'racecar', '>= 0.3.5' gem 'rack' gem 'rack-test' diff --git a/docker-compose.yml b/docker-compose.yml index 73a82bca43d..1241f3254d9 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -10,6 +10,7 @@ services: - mongodb - mysql - postgres + - prestoo - redis env_file: ./.env environment: @@ -21,6 +22,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true @@ -38,6 +41,7 @@ services: - mongodb - mysql - postgres + - presto - redis env_file: ./.env environment: @@ -49,6 +53,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true @@ -66,6 +72,7 @@ services: - mongodb - mysql - postgres + - presto - redis env_file: ./.env environment: @@ -77,6 +84,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true @@ -94,6 +103,7 @@ services: - mongodb - mysql - postgres + - presto - redis env_file: ./.env environment: @@ -105,6 +115,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true @@ -122,6 +134,7 @@ services: - mongodb - mysql - postgres + - presto - redis env_file: ./.env environment: @@ -133,6 +146,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true @@ -181,6 +196,7 @@ services: - mongodb - mysql - postgres + - presto - redis env_file: ./.env environment: @@ -192,6 +208,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true @@ -209,6 +227,7 @@ services: - mongodb - mysql - postgres + - presto - redis env_file: ./.env environment: @@ -220,6 +239,8 @@ services: - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - TEST_POSTGRES_HOST=postgres + - TEST_PRESTO_HOST=presto + - TEST_PRESTO_PORT=8080 - TEST_REDIS_HOST=redis stdin_open: true tty: true diff --git a/lib/ddtrace/contrib/presto/instrumentation.rb b/lib/ddtrace/contrib/presto/instrumentation.rb index a798a1deaaf..f992a3c1bcd 100644 --- a/lib/ddtrace/contrib/presto/instrumentation.rb +++ b/lib/ddtrace/contrib/presto/instrumentation.rb @@ -25,9 +25,9 @@ def run(query) span.set_tag(Ext::TAG_QUERY_ASYNC, false) rescue StandardError => e Datadog::Logger.log.debug("error preparing span for presto: #{e}") - ensure - super(query) end + + super(query) end end @@ -40,9 +40,9 @@ def query(query, &blk) span.set_tag(Ext::TAG_QUERY_ASYNC, !blk.nil?) rescue StandardError => e Datadog::Logger.log.debug("error preparing span for presto: #{e}") - ensure - super(query, &blk) end + + super(query, &blk) end end @@ -56,9 +56,9 @@ def kill(query_id) span.set_tag(Ext::TAG_QUERY_ID, query_id) rescue StandardError => e Datadog::Logger.log.debug("error preparing span for presto: #{e}") - ensure - super(query_id) end + + super(query_id) end end @@ -91,8 +91,8 @@ def decorate!(span) set_nilable_tag!(span, :model_version, Ext::TAG_MODEL_VERSION) # Set analytics sample rate - if Contrib::Analytics.enabled?(configuration[:analytics_enabled]) - Contrib::Analytics.set_sample_rate(span, configuration[:analytics_sample_rate]) + if Contrib::Analytics.enabled?(datadog_configuration[:analytics_enabled]) + Contrib::Analytics.set_sample_rate(span, datadog_configuration[:analytics_sample_rate]) end end diff --git a/spec/ddtrace/contrib/presto/client_spec.rb b/spec/ddtrace/contrib/presto/client_spec.rb index bf214e9c415..49c5eb1655e 100644 --- a/spec/ddtrace/contrib/presto/client_spec.rb +++ b/spec/ddtrace/contrib/presto/client_spec.rb @@ -230,7 +230,7 @@ def suppress_warnings it 'is an error' do expect(span).to have_error expect(span).to have_error_type('Presto::Client::PrestoQueryError') - expect(span).to have_error_message("Column 'banana' cannot be resolved") + expect(span).to have_error_message(/Column 'banana' cannot be resolved/) end end end From aa277350df27c6110ecf5e64ce7e80cc6aff7485 Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 11:08:41 +0100 Subject: [PATCH 2/7] add analytics test --- spec/ddtrace/contrib/presto/client_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/ddtrace/contrib/presto/client_spec.rb b/spec/ddtrace/contrib/presto/client_spec.rb index 49c5eb1655e..cc0f7db8536 100644 --- a/spec/ddtrace/contrib/presto/client_spec.rb +++ b/spec/ddtrace/contrib/presto/client_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' require 'ddtrace' require 'presto-client' +require 'ddtrace/contrib/analytics_examples' RSpec.describe 'Presto::Client instrumentation' do let(:tracer) { get_test_tracer } @@ -83,6 +84,11 @@ def suppress_warnings expect(span.get_tag('presto.model_version')).to eq(model_version) expect(span.get_tag('out.host')).to eq("#{host}:#{port}") end + + it_behaves_like 'analytics for integration' do + let(:analytics_enabled_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_ENABLED } + let(:analytics_sample_rate_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_SAMPLE_RATE } + end end shared_examples_for 'a configurable Presto trace' do From b1a2bf89d247a81404640114e9924b7595846915 Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 11:40:31 +0100 Subject: [PATCH 3/7] [presto]add analytics test in a more considerate way, add rake tasks, fix docker config details --- Rakefile | 8 ++++++++ docker-compose.yml | 2 +- spec/ddtrace/contrib/presto/client_spec.rb | 17 ++++++++++++----- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/Rakefile b/Rakefile index 85a4d98451f..32e067b8b32 100644 --- a/Rakefile +++ b/Rakefile @@ -189,6 +189,7 @@ task :ci do sh 'bundle exec appraisal contrib-old rake spec:http' sh 'bundle exec appraisal contrib-old rake spec:mongodb' sh 'bundle exec appraisal contrib-old rake spec:mysql2' + sh 'bundle exec appraisal contrib-old rake spec:presto' sh 'bundle exec appraisal contrib-old rake spec:rack' sh 'bundle exec appraisal contrib-old rake spec:rake' sh 'bundle exec appraisal contrib-old rake spec:redis' @@ -241,6 +242,7 @@ task :ci do sh 'bundle exec appraisal contrib-old rake spec:http' sh 'bundle exec appraisal contrib-old rake spec:mongodb' sh 'bundle exec appraisal contrib-old rake spec:mysql2' + sh 'bundle exec appraisal contrib-old rake spec:presto' sh 'bundle exec appraisal contrib-old rake spec:rack' sh 'bundle exec appraisal contrib-old rake spec:rake' sh 'bundle exec appraisal contrib-old rake spec:redis' @@ -303,6 +305,7 @@ task :ci do sh 'bundle exec appraisal contrib rake spec:http' sh 'bundle exec appraisal contrib rake spec:mongodb' sh 'bundle exec appraisal contrib rake spec:mysql2' + sh 'bundle exec appraisal contrib rake spec:presto' sh 'bundle exec appraisal contrib rake spec:racecar' sh 'bundle exec appraisal contrib rake spec:rack' sh 'bundle exec appraisal contrib rake spec:rake' @@ -371,6 +374,7 @@ task :ci do sh 'bundle exec appraisal contrib rake spec:http' sh 'bundle exec appraisal contrib rake spec:mongodb' sh 'bundle exec appraisal contrib rake spec:mysql2' + sh 'bundle exec appraisal contrib rake spec:presto' sh 'bundle exec appraisal contrib rake spec:racecar' sh 'bundle exec appraisal contrib rake spec:rack' sh 'bundle exec appraisal contrib rake spec:rake' @@ -443,6 +447,7 @@ task :ci do sh 'bundle exec appraisal contrib rake spec:http' sh 'bundle exec appraisal contrib rake spec:mongodb' sh 'bundle exec appraisal contrib rake spec:mysql2' + sh 'bundle exec appraisal contrib rake spec:presto' sh 'bundle exec appraisal contrib rake spec:racecar' sh 'bundle exec appraisal contrib rake spec:rack' sh 'bundle exec appraisal contrib rake spec:rake' @@ -500,6 +505,7 @@ task :ci do sh 'bundle exec appraisal contrib rake spec:http' sh 'bundle exec appraisal contrib rake spec:mongodb' sh 'bundle exec appraisal contrib rake spec:mysql2' + sh 'bundle exec appraisal contrib rake spec:presto' sh 'bundle exec appraisal contrib rake spec:racecar' sh 'bundle exec appraisal contrib rake spec:rack' sh 'bundle exec appraisal contrib rake spec:rake' @@ -567,6 +573,7 @@ task :ci do sh 'bundle exec appraisal contrib rake spec:http' sh 'bundle exec appraisal contrib rake spec:mongodb' sh 'bundle exec appraisal contrib rake spec:mysql2' + sh 'bundle exec appraisal contrib rake spec:presto' sh 'bundle exec appraisal contrib rake spec:racecar' sh 'bundle exec appraisal contrib rake spec:rack' sh 'bundle exec appraisal contrib rake spec:rake' @@ -633,6 +640,7 @@ task :ci do sh 'bundle exec appraisal contrib rake spec:http' sh 'bundle exec appraisal contrib rake spec:mongodb' sh 'bundle exec appraisal contrib rake spec:mysql2' + sh 'bundle exec appraisal contrib rake spec:presto' sh 'bundle exec appraisal contrib rake spec:racecar' sh 'bundle exec appraisal contrib rake spec:rack' sh 'bundle exec appraisal contrib rake spec:rake' diff --git a/docker-compose.yml b/docker-compose.yml index 1241f3254d9..6e38a8311d8 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -10,7 +10,7 @@ services: - mongodb - mysql - postgres - - prestoo + - presto - redis env_file: ./.env environment: diff --git a/spec/ddtrace/contrib/presto/client_spec.rb b/spec/ddtrace/contrib/presto/client_spec.rb index cc0f7db8536..0e1edd71dae 100644 --- a/spec/ddtrace/contrib/presto/client_spec.rb +++ b/spec/ddtrace/contrib/presto/client_spec.rb @@ -84,11 +84,6 @@ def suppress_warnings expect(span.get_tag('presto.model_version')).to eq(model_version) expect(span.get_tag('out.host')).to eq("#{host}:#{port}") end - - it_behaves_like 'analytics for integration' do - let(:analytics_enabled_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_ENABLED } - let(:analytics_sample_rate_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_SAMPLE_RATE } - end end shared_examples_for 'a configurable Presto trace' do @@ -199,12 +194,20 @@ def suppress_warnings end end + shared_examples_for 'analytics' do + it_behaves_like 'analytics for integration' do + let(:analytics_enabled_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_ENABLED } + let(:analytics_sample_rate_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_SAMPLE_RATE } + end + end + describe '#run operation' do before(:each) { client.run('SELECT 1') } it_behaves_like 'a Presto trace' it_behaves_like 'a configurable Presto trace' it_behaves_like 'a synchronous query trace' + it_behaves_like 'analytics' it 'has a query resource' do expect(span.resource).to eq('SELECT 1') @@ -259,6 +262,7 @@ def suppress_warnings it_behaves_like 'a configurable Presto trace' it_behaves_like 'a query trace' it_behaves_like 'a synchronous query trace' + it_behaves_like 'analytics' end context 'given a block parameter' do @@ -268,6 +272,7 @@ def suppress_warnings it_behaves_like 'a configurable Presto trace' it_behaves_like 'a query trace' it_behaves_like 'an asynchronous query trace' + it_behaves_like 'analytics' end end @@ -278,6 +283,7 @@ def suppress_warnings it_behaves_like 'a Presto trace' it_behaves_like 'a configurable Presto trace' + it_behaves_like 'analytics' it 'has a kill resource' do expect(span.resource).to eq(Datadog::Contrib::Presto::Ext::SPAN_KILL) @@ -297,6 +303,7 @@ def suppress_warnings it_behaves_like 'a Presto trace' it_behaves_like 'a configurable Presto trace' + it_behaves_like 'analytics' it 'has a query resource' do expect(span.resource).to eq('SELECT 1') From 974e571516d4bce417cb24e595966472cfb987c4 Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 12:03:09 +0100 Subject: [PATCH 4/7] [presto]improve naming for shared examples for analytics test --- spec/ddtrace/contrib/presto/client_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/ddtrace/contrib/presto/client_spec.rb b/spec/ddtrace/contrib/presto/client_spec.rb index 0e1edd71dae..99c8af4d0ef 100644 --- a/spec/ddtrace/contrib/presto/client_spec.rb +++ b/spec/ddtrace/contrib/presto/client_spec.rb @@ -194,7 +194,7 @@ def suppress_warnings end end - shared_examples_for 'analytics' do + shared_examples_for 'a sampled trace' do it_behaves_like 'analytics for integration' do let(:analytics_enabled_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_ENABLED } let(:analytics_sample_rate_var) { Datadog::Contrib::Presto::Ext::ENV_ANALYTICS_SAMPLE_RATE } @@ -207,7 +207,7 @@ def suppress_warnings it_behaves_like 'a Presto trace' it_behaves_like 'a configurable Presto trace' it_behaves_like 'a synchronous query trace' - it_behaves_like 'analytics' + it_behaves_like 'a sampled trace' it 'has a query resource' do expect(span.resource).to eq('SELECT 1') @@ -262,7 +262,7 @@ def suppress_warnings it_behaves_like 'a configurable Presto trace' it_behaves_like 'a query trace' it_behaves_like 'a synchronous query trace' - it_behaves_like 'analytics' + it_behaves_like 'a sampled trace' end context 'given a block parameter' do @@ -272,7 +272,7 @@ def suppress_warnings it_behaves_like 'a configurable Presto trace' it_behaves_like 'a query trace' it_behaves_like 'an asynchronous query trace' - it_behaves_like 'analytics' + it_behaves_like 'a sampled trace' end end @@ -283,7 +283,7 @@ def suppress_warnings it_behaves_like 'a Presto trace' it_behaves_like 'a configurable Presto trace' - it_behaves_like 'analytics' + it_behaves_like 'a sampled trace' it 'has a kill resource' do expect(span.resource).to eq(Datadog::Contrib::Presto::Ext::SPAN_KILL) @@ -303,7 +303,7 @@ def suppress_warnings it_behaves_like 'a Presto trace' it_behaves_like 'a configurable Presto trace' - it_behaves_like 'analytics' + it_behaves_like 'a sampled trace' it 'has a query resource' do expect(span.resource).to eq('SELECT 1') From 789937da579020bf794608d3e51226993a921c49 Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 13:04:41 +0100 Subject: [PATCH 5/7] [presto] only supports ruby 2.1 and up --- Appraisals | 1 - Rakefile | 1 - 2 files changed, 2 deletions(-) diff --git a/Appraisals b/Appraisals index 297ec1f386d..d76d2dfccb2 100644 --- a/Appraisals +++ b/Appraisals @@ -75,7 +75,6 @@ elsif Gem::Version.new('2.0.0') <= Gem::Version.new(RUBY_VERSION) \ gem 'hiredis' gem 'mongo', '< 2.5' gem 'mysql2', '0.3.21', platform: :ruby - gem 'presto-client', '>= 0.5.14' gem 'rack', '1.4.7' gem 'rack-cache', '1.7.1' gem 'rack-test', '0.7.0' diff --git a/Rakefile b/Rakefile index 32e067b8b32..24f501ffb91 100644 --- a/Rakefile +++ b/Rakefile @@ -189,7 +189,6 @@ task :ci do sh 'bundle exec appraisal contrib-old rake spec:http' sh 'bundle exec appraisal contrib-old rake spec:mongodb' sh 'bundle exec appraisal contrib-old rake spec:mysql2' - sh 'bundle exec appraisal contrib-old rake spec:presto' sh 'bundle exec appraisal contrib-old rake spec:rack' sh 'bundle exec appraisal contrib-old rake spec:rake' sh 'bundle exec appraisal contrib-old rake spec:redis' From d455e18de4e2367e6dc97b0ac0367060f48cfc3e Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 13:10:01 +0100 Subject: [PATCH 6/7] remove depends_on for presto for 2.0 container --- docker-compose.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 6e38a8311d8..3b1415f6f79 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -10,7 +10,6 @@ services: - mongodb - mysql - postgres - - presto - redis env_file: ./.env environment: @@ -21,9 +20,7 @@ services: - TEST_MEMCACHED_HOST=memcached - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - - TEST_POSTGRES_HOST=postgres - - TEST_PRESTO_HOST=presto - - TEST_PRESTO_PORT=8080 + - TEST_POSTGRES_HOST=postgres - TEST_REDIS_HOST=redis stdin_open: true tty: true From 505a290375ea5ce74b99509c8b60cf501f65ab5b Mon Sep 17 00:00:00 2001 From: ericmustin Date: Tue, 17 Mar 2020 13:10:24 +0100 Subject: [PATCH 7/7] linting --- docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index 3b1415f6f79..27c9cd87af4 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -20,7 +20,7 @@ services: - TEST_MEMCACHED_HOST=memcached - TEST_MONGODB_HOST=mongodb - TEST_MYSQL_HOST=mysql - - TEST_POSTGRES_HOST=postgres + - TEST_POSTGRES_HOST=postgres - TEST_REDIS_HOST=redis stdin_open: true tty: true