Instrument sequel gem #414
Conversation
arjun-rajappa
force-pushed
the
instrument-sequel-gem
branch
from
b9bf65b
to
e2a0337
Compare
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
Signed-off-by: arjun-rajappa <[email protected]>
arjun-rajappa
force-pushed
the
instrument-sequel-gem
branch
from
e2a0337
to
9eacd24
Compare
lib/instana/activators/sequel.rb
Outdated
| @@ -0,0 +1,21 @@ | |||
| # (c) Copyright IBM Corp. 2021 | |||
| # (c) Copyright Instana Inc. 2021 | |||
There was a problem hiding this comment.
You don't need Instana Inc. anymore.
lib/instana/activators/sequel.rb
Outdated
| @@ -0,0 +1,21 @@ | |||
| # (c) Copyright IBM Corp. 2021 | |||
| module Activators | ||
| class Sequel < Activator | ||
| def can_instrument? | ||
| defined?(::Sequel::Database) |
There was a problem hiding this comment.
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].
| module Instrumentation | ||
| module Sequel | ||
| IGNORED_SQL = %w[BEGIN COMMIT SET].freeze | ||
| SANITIZE_REGEXP = /('[\s\S][^']*'|\d*\.\d+|\d+|NULL)/i.freeze |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Arjun Rajappa <[email protected]>
Signed-off-by: Arjun Rajappa <[email protected]>
Signed-off-by: Arjun Rajappa <[email protected]>
lib/instana/config.rb
Outdated
| @@ -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 } | |||
There was a problem hiding this comment.
Could we align the indentation here?
Signed-off-by: Arjun Rajappa <[email protected]>
Instrument sequel gem