Add option to disable db.statement in DB instrumentation

[fbogsany]

See open-telemetry/opentelemetry-specification#1659

We should use the same configuration option in all affected instrumentation.

Affected instrumentation:

• Dalli
• LMDB
• Mongo
• MySQL2
• Postgres
• Redis

[robertlaurin]

As mentioned in the description we should use the same config option.

Currently we're a bit inconsistent with the configuration naming here see examples:

opentelemetry-ruby/instrumentation/pg/lib/opentelemetry/instrumentation/pg/instrumentation.rb

Lines 28 to 29 in 9dc42f1

	option :enable_sql_obfuscation, default: false, validate: :boolean
	option :enable_statement_attribute, default: true, validate: :boolean

opentelemetry-ruby/instrumentation/redis/lib/opentelemetry/instrumentation/redis/instrumentation.rb

Line 24 in 9dc42f1

option :enable_statement_obfuscation, default: true, validate: :boolean

opentelemetry-ruby/instrumentation/mysql2/lib/opentelemetry/instrumentation/mysql2/instrumentation.rb

Line 23 in 9dc42f1

option :enable_sql_obfuscation, default: false, validate: :boolean

Settling on a consistent name will introduce breaking changes but I think its valuable in the long term.

I have the opinion that the enable/disable prefix is a bit wordy and could be dropped (this is true of other config options, some I introduced myself).

option :db_statement,             default: true, validate: :boolean
option :db_statement_obfuscation, default: true, validate: :boolean

[SomalianIvan]

👀

[fbogsany]

I have the opinion that the enable/disable prefix is a bit wordy and could be dropped

I'm not sure I agree 😄. I tend to think boolean config options benefit from being clearly a predicate, e.g. include_db_statement: false is clearer than db_statement: false. On the other hand, obfuscate_db_statement: false is clearer than either enable_db_statement_obfuscation: false or db_statement_obfuscation: false. So my vote would be for:

option :include_db_statement,   default: true, validate: :boolean
option :obfuscate_db_statement, default: true, validate: :boolean

Alternatively, these could be unified:

option :db_statement, default: :include, validate: ->(opt) { %I[include omit obfuscate].include?(opt) }

[robertlaurin]

I'm not sure I agree

I tend to think boolean config options benefit from being clearly a predicate

Looking at your examples I agree, I think my issue was with enable_ / disable_ prefixes. Having the option names clearly be predicates is the better route.

Alternatively, these could be unified:

option :db_statement, default: :include, validate: ->(opt) { %I[include omit obfuscate].include?(opt) }

I think I prefer the alternative overall when thinking about this from the perspective of an application that may have multiple instrumentation libraries that have this option.

{
  'OpenTelemetry::Instrumentation::Redis'  => { db_statement: :include },
  'OpenTelemetry::Instrumentation::Mysql2' => { db_statement: :obfuscate },
  'OpenTelemetry::Instrumentation::LMDB'   => { db_statement: :omit },
}

It just seems clearer to understand. In the example where they are split which one takes priority? If I do not include the db.statement but enable obfuscation what should happen? With the alternative this is no longer a concern.