From b8ee1c3e8778f9a8488beae6c01bd81a98100a2d Mon Sep 17 00:00:00 2001 From: Ian MacDonald Date: Tue, 22 Sep 2020 15:41:49 -0700 Subject: [PATCH 1/3] sequel-instrumentation-fail-for-sql-expression When instrumenting an application using the `sequel` integration, tracing was failing for a case like the following: ```ruby db.run( Sequel.lit("UPDATE foo SET bar = :wat WHERE bar = :var", wat: 2, var: 1) ) ``` When using `Sequel.lit` in this context, it returns a `Sequel::SQL::PlaceholderLiteralString`. This object does not respond to `prepared_sql`, the method expected by `Datadog::Contrib::Sequel::Utils.parse_opts`. To coerce a `Sequel::SQL::PlaceholderLiteralString` into a `String`, `Sequel::Database#literal` (`Sequel::Dataset#literal`) may be used. Unfortunately a duck-type approach for the variety of types the `sql` argument may take is not possible as when `sql` is a `String` because `#literal` escapes `'`s: ```ruby db.literal("SELECT * FROM tbl WHERE name = 'foo'") # => "'SELECT * FROM tbl WHERE name = ''foo'''" ``` --- lib/ddtrace/contrib/sequel/database.rb | 8 +++ .../contrib/sequel/instrumentation_spec.rb | 58 ++++++++++++------- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/lib/ddtrace/contrib/sequel/database.rb b/lib/ddtrace/contrib/sequel/database.rb index e32d395682d..a9561c1a8ac 100644 --- a/lib/ddtrace/contrib/sequel/database.rb +++ b/lib/ddtrace/contrib/sequel/database.rb @@ -52,6 +52,14 @@ def parse_opts(sql, opts) elsif instance_variable_defined?(:@pool) && @pool @pool.db.opts end + sql = case sql + when Symbol + ::Sequel.lit(sql).to_s + when ::Sequel::SQL::Expression + literal(sql) + else + sql + end Utils.parse_opts(sql, opts, db_opts) end end diff --git a/spec/ddtrace/contrib/sequel/instrumentation_spec.rb b/spec/ddtrace/contrib/sequel/instrumentation_spec.rb index bf93328babc..3e3ab05089f 100644 --- a/spec/ddtrace/contrib/sequel/instrumentation_spec.rb +++ b/spec/ddtrace/contrib/sequel/instrumentation_spec.rb @@ -35,6 +35,7 @@ around do |example| # Reset before and after each example; don't allow global state to linger. Datadog.registry[:sequel].reset_configuration! + Sequel::DATABASES.each(&:disconnect) example.run Datadog.registry[:sequel].reset_configuration! end @@ -48,31 +49,46 @@ let(:normalized_adapter) { defined?(super) ? super() : adapter } - describe 'when queried through a Sequel::Database object' do - before(:each) { sequel.run(query) } - let(:query) { "SELECT * FROM tbl WHERE name = 'foo'" } - let(:span) { spans.first } - - it 'traces the command' do - expect(span.name).to eq('sequel.query') - # Expect it to be the normalized adapter name. - expect(span.service).to eq(normalized_adapter) - expect(span.span_type).to eq('sql') - expect(span.get_tag('sequel.db.vendor')).to eq(normalized_adapter) - # Expect non-quantized query: agent does SQL quantization. - expect(span.resource).to eq(query) - expect(span.status).to eq(0) - expect(span.parent_id).to eq(0) - end + describe 'Sequel::Database.run' do + shared_context 'query executed through a Sequel::Database object' do + before(:each) { sequel.run(query) } + let(:span) { spans.first } - it_behaves_like 'analytics for integration' do - let(:analytics_enabled_var) { Datadog::Contrib::Sequel::Ext::ENV_ANALYTICS_ENABLED } - let(:analytics_sample_rate_var) { Datadog::Contrib::Sequel::Ext::ENV_ANALYTICS_SAMPLE_RATE } + it 'traces the command' do + expect(span.name).to eq('sequel.query') + # Expect it to be the normalized adapter name. + expect(span.service).to eq(normalized_adapter) + expect(span.span_type).to eq('sql') + expect(span.get_tag('sequel.db.vendor')).to eq(normalized_adapter) + # Expect non-quantized query: agent does SQL quantization. + expect(span.resource).to eq(expected_query) + expect(span.status).to eq(0) + expect(span.parent_id).to eq(0) + end + + it_behaves_like 'analytics for integration' do + let(:analytics_enabled_var) { Datadog::Contrib::Sequel::Ext::ENV_ANALYTICS_ENABLED } + let(:analytics_sample_rate_var) { Datadog::Contrib::Sequel::Ext::ENV_ANALYTICS_SAMPLE_RATE } + end + + it_behaves_like 'a peer service span' + + it_behaves_like 'measured span for integration', false end - it_behaves_like 'a peer service span' + context 'with query provided as a string' do + it_behaves_like 'query executed through a Sequel::Database object' do + let(:query) { "SELECT * FROM tbl WHERE name = 'foo'" } + let(:expected_query) { query } + end + end - it_behaves_like 'measured span for integration', false + context 'with query provided as complex expression' do + it_behaves_like 'query executed through a Sequel::Database object' do + let(:query) { Sequel.lit("SELECT * FROM tbl WHERE name = :var", var: 'foo') } + let(:expected_query) { sequel.literal(query) } + end + end end describe 'when queried through a Sequel::Dataset' do From 0e58d73beeff7291261337623196ce42870e1564 Mon Sep 17 00:00:00 2001 From: Ian MacDonald Date: Tue, 22 Sep 2020 16:04:10 -0700 Subject: [PATCH 2/3] sequel-instrumentation-fail-for-sql-expression refactor type check --- lib/ddtrace/contrib/sequel/database.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/ddtrace/contrib/sequel/database.rb b/lib/ddtrace/contrib/sequel/database.rb index a9561c1a8ac..eed4cae91e8 100644 --- a/lib/ddtrace/contrib/sequel/database.rb +++ b/lib/ddtrace/contrib/sequel/database.rb @@ -52,14 +52,8 @@ def parse_opts(sql, opts) elsif instance_variable_defined?(:@pool) && @pool @pool.db.opts end - sql = case sql - when Symbol - ::Sequel.lit(sql).to_s - when ::Sequel::SQL::Expression - literal(sql) - else - sql - end + sql = sql.is_a?(::Sequel::SQL::Expression) ? literal(sql) : sql.to_s + Utils.parse_opts(sql, opts, db_opts) end end From 0084de40f5ae8317ed08a0c58adab7cfe4ae2b62 Mon Sep 17 00:00:00 2001 From: Ian MacDonald Date: Tue, 22 Sep 2020 16:23:03 -0700 Subject: [PATCH 3/3] sequel-instrumentation-fail-for-sql-expression rubocop --- spec/ddtrace/contrib/sequel/instrumentation_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ddtrace/contrib/sequel/instrumentation_spec.rb b/spec/ddtrace/contrib/sequel/instrumentation_spec.rb index 3e3ab05089f..0982b98fc7b 100644 --- a/spec/ddtrace/contrib/sequel/instrumentation_spec.rb +++ b/spec/ddtrace/contrib/sequel/instrumentation_spec.rb @@ -85,7 +85,7 @@ context 'with query provided as complex expression' do it_behaves_like 'query executed through a Sequel::Database object' do - let(:query) { Sequel.lit("SELECT * FROM tbl WHERE name = :var", var: 'foo') } + let(:query) { Sequel.lit('SELECT * FROM tbl WHERE name = :var', var: 'foo') } let(:expected_query) { sequel.literal(query) } end end