Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument sequel gem #414

Merged
merged 12 commits into from
Sep 5, 2024
1 change: 1 addition & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ library_gemfile: &library_gemfile
- "./gemfiles/sinatra_22.gemfile"
- "./gemfiles/sinatra_30.gemfile"
- "./gemfiles/sinatra_40.gemfile"
- "./gemfiles/sequel_50.gemfile"
- "./gemfiles/shoryuken_50.gemfile"
- "./gemfiles/shoryuken_60.gemfile"
- "./gemfiles/mongo_216.gemfile"
Expand Down
15 changes: 15 additions & 0 deletions gemfiles/sequel_50.gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# This file was generated by Appraisal

# (c) Copyright IBM Corp. 2024

source "https://rubygems.org"

gem "minitest-reporters"
gem "webmock"
gem "puma"
gem "rack-test"
gem "simplecov", "~> 0.21.2"
gem "sequel", ">= 5.0"
gem "sqlite3", "~> 1.4"

gemspec path: "../"
21 changes: 21 additions & 0 deletions lib/instana/activators/sequel.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# (c) Copyright IBM Corp. 2021
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2024!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed!

# (c) Copyright Instana Inc. 2021
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need Instana Inc. anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed!


module Instana
module Activators
class Sequel < Activator
def can_instrument?
defined?(::Sequel::Database)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, tha added option in config.rb, would be used here already. In fact I don't see anywhere in production code that you read config[:sequel].

end

def instrument
require 'instana/instrumentation/sequel'

::Sequel::Database
.prepend(Instana::Instrumentation::Sequel)

true
end
end
end
end
1 change: 1 addition & 0 deletions lib/instana/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ def initialize(logger: ::Instana.logger, agent_host: ENV['INSTANA_AGENT_HOST'],
@config[:'resque-client'] = { :enabled => true, :propagate => true }
@config[:'resque-worker'] = { :enabled => true, :'setup-fork' => true }
@config[:'rest-client'] = { :enabled => true }
@config[:sequel] = { :enabled => true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we align the indentation here?

@config[:'sidekiq-client'] = { :enabled => true }
@config[:'sidekiq-worker'] = { :enabled => true }
end
Expand Down
41 changes: 41 additions & 0 deletions lib/instana/instrumentation/sequel.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# (c) Copyright IBM Corp. 2024

module Instana
module Instrumentation
module Sequel
IGNORED_SQL = %w[BEGIN COMMIT SET].freeze
SANITIZE_REGEXP = /('[\s\S][^']*'|\d*\.\d+|\d+|NULL)/i.freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why [\s\S] over just .? Do we expect a linebreak here?
In general why is it needed? Why not just the [^']*'?
Why does NULL need to be sanitized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex is present in active record. So i copied the same. We sanitise sql so that the ppl viewing the logs might not get the information like PII etc etc. So NULL can be used to filter data. Hence i think we are sanitising NULL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am familiar with the purpose of sanitation. My point is that the string NULL alone is not a secret, so without further reason, it makes no sense to sanitize it.
And I have also noticed, that active record has this pattern, and I don't think we should randomly copy code without understanding it.


def log_connection_yield(sql, conn, *args)
call_payload = {
sequel: {
adapter: opts[:adapter],
host: opts[:host],
username: opts[:user],
db: opts[:database],
sql: maybe_sanitize(sql)
}
}
maybe_trace(call_payload) { super(sql, conn, *args) }
end

private

def maybe_sanitize(sql)
::Instana.config[:sanitize_sql] ? sql.gsub(SANITIZE_REGEXP, '?') : sql
end

def maybe_trace(call_payload, &blk)
if ::Instana.tracer.tracing? && !ignored?(call_payload)
::Instana.tracer.trace(:sequel, call_payload, &blk)
else
yield
end
end

def ignored?(call_payload)
IGNORED_SQL.any? { |s| call_payload[:sequel][:sql].upcase.start_with?(s) }
end
end
end
end
4 changes: 2 additions & 2 deletions lib/instana/tracing/span.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ class Span
:memcache, :'net-http', :rack, :render, :'rpc-client',
:'rpc-server', :'sidekiq-client', :'sidekiq-worker',
:redis, :'resque-client', :'resque-worker', :'graphql.server', :dynamodb, :s3, :sns, :sqs, :'aws.lambda.entry', :activejob, :log, :"mail.actionmailer",
:"aws.lambda.invoke", :mongo ].freeze
:"aws.lambda.invoke", :mongo, :sequel ].freeze
ENTRY_SPANS = [ :rack, :'resque-worker', :'rpc-server', :'sidekiq-worker', :'graphql.server', :sqs,
:'aws.lambda.entry' ].freeze
EXIT_SPANS = [ :activerecord, :excon, :'net-http', :'resque-client',
:'rpc-client', :'sidekiq-client', :redis, :dynamodb, :s3, :sns, :sqs, :log, :"mail.actionmailer",
:"aws.lambda.invoke", :mongo ].freeze
:"aws.lambda.invoke", :mongo, :sequel ].freeze
HTTP_SPANS = [ :rack, :excon, :'net-http' ].freeze

attr_accessor :parent
Expand Down
105 changes: 105 additions & 0 deletions test/instrumentation/sequel_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# (c) Copyright IBM Corp. 2024

require 'test_helper'
require 'sequel'

class SequelTest < Minitest::Test
def setup
skip unless ENV['DATABASE_URL']
db_url = ENV['DATABASE_URL'].sub("sqlite3", "sqlite")
@db = Sequel.connect(db_url)

@db.create_table!(:blocks) do
String :name
String :color
end
@model = @db[:blocks]
end

def teardown
@db.drop_table(:blocks)
@db.disconnect
end

def test_config_defaults
assert ::Instana.config[:sanitize_sql] == true
assert ::Instana.config[:sequel].is_a?(Hash)
assert ::Instana.config[:sequel].key?(:enabled)
assert_equal true, ::Instana.config[:sequel][:enabled]
end

def test_create
Instana::Tracer.start_or_continue_trace(:sequel_test, {}) do
@model.insert(name: 'core', color: 'blue')
end
spans = ::Instana.processor.queued_spans
assert_equal 2, spans.length
span = find_first_span_by_name(spans, :sequel)
data = span[:data][:sequel]
assert data[:sql].start_with?('INSERT INTO')
end

def test_read
@model.insert(name: 'core', color: 'blue')
Instana::Tracer.start_or_continue_trace(:sequel_test, {}) do
@model.where(name: 'core').first
end
spans = ::Instana.processor.queued_spans
assert_equal 2, spans.length
span = find_first_span_by_name(spans, :sequel)
data = span[:data][:sequel]
assert data[:sql].start_with?('SELECT')
assert_nil span[:ec]
end

def test_update
@model.insert(name: 'core', color: 'blue')
Instana::Tracer.start_or_continue_trace(:sequel_test, {}) do
@model.where(name: 'core').update(color: 'red')
end
spans = ::Instana.processor.queued_spans
assert_equal 2, spans.length
span = find_first_span_by_name(spans, :sequel)
data = span[:data][:sequel]
assert data[:sql].start_with?('UPDATE')
assert_nil span[:ec]
end

def test_delete
@model.insert(name: 'core', color: 'blue')
Instana::Tracer.start_or_continue_trace(:sequel_test, {}) do
@model.where(name: 'core').delete
end
spans = ::Instana.processor.queued_spans
assert_equal 2, spans.length
span = find_first_span_by_name(spans, :sequel)
data = span[:data][:sequel]
assert data[:sql].start_with?('DELETE')
assert_nil span[:ec]
end

def test_raw
Instana::Tracer.start_or_continue_trace(:sequel_test, {}) do
@db.run('SELECT 1')
end
spans = ::Instana.processor.queued_spans
assert_equal 2, spans.length
span = find_first_span_by_name(spans, :sequel)
data = span[:data][:sequel]
assert 'SELECT 1', data[:sql]
assert_nil span[:ec]
end

def test_raw_error
assert_raises Sequel::DatabaseError do
Instana::Tracer.start_or_continue_trace(:sequel_test, {}) do
@db.run('INVALID')
end
end
spans = ::Instana.processor.queued_spans
assert_equal 2, spans.length
span = find_first_span_by_name(spans, :sequel)

assert_equal 1, span[:ec]
end
end
Loading