fix: handle encoding errors in mysql obfuscation

[robertlaurin]

😭

ArgumentError: invalid byte sequence in UTF-8
gems/opentelemetry-instrumentation-mysql2-0.21.0/lib/opentelemetry/instrumentation/mysql2/patches/client.rb:76 gsub	
gems/opentelemetry-instrumentation-mysql2-0.21.0/lib/opentelemetry/instrumentation/mysql2/patches/client.rb:76 obfuscate_sql	
gems/opentelemetry-instrumentation-mysql2-0.21.0/lib/opentelemetry/instrumentation/mysql2/patches/client.rb:59 query

[plantfansam]

👍

[ahayworth]

Is it possible to make a more specific error message here?

[robertlaurin]

I wasn't sure how specific to make it, this is what will get plunked into the the db.statement attribute field. I'm being prudent about capturing any information about what was failed to be obfuscated.

What did you have in mind? Like the error class or something?

[ahayworth]

Ah, I didn't realize that this would get put into the db.statement field, which is my bad for not looking closely enough. I think this is fine. 👍

[fbogsany]

Probably not something you need to address here, but it'd be useful to use a common prefix for all the "error" type substitutions (this and the two above) - it'd make it easier for Observability teams to monitor this behaviour. Metrics might be nice as well, but a competent engineer can build metrics from the db.statement in a span processor or in the collector.

[robertlaurin]

Ya'll need to stop approving my failing PR lol

  1) Failure:
OpenTelemetry::Instrumentation::Mysql2::Instrumentation::tracing::when enable_sql_obfuscation is enabled#test_0002_encodes invalid byte sequences for db.statement [/home/runner/work/opentelemetry-ruby-contrib/opentelemetry-ruby-contrib/instrumentation/mysql2/test/opentelemetry/instrumentation/mysql2/instrumentation_test.rb:222]:
Expected: "select"
  Actual: true

[robertlaurin]

Can anyone guess why the span.name is true in my failing test 😭

opentelemetry-ruby-contrib/instrumentation/mysql2/lib/opentelemetry/instrumentation/mysql2/patches/client.rb

Lines 144 to 148 in cbcad76

	def extract_statement_type(sql)
	QUERY_NAME_RE.match(sql) { |match| match[1].downcase } unless sql.nil?
	rescue StandardError => e
	OpenTelemetry.logger.debug("Error extracting sql statement type: #{e.message}")
	end

[robertlaurin]

I mean technically true is not nil.

opentelemetry-ruby-contrib/instrumentation/mysql2/lib/opentelemetry/instrumentation/mysql2/patches/client.rb

Lines 98 to 112 in cbcad76

	def database_span_name(sql)
	# Setting span name to the SQL query without obfuscation would
	# result in PII + cardinality issues.
	# First attempt to infer the statement type then fallback to
	# current Otel approach {database.component_name}.{database_instance_name}
	# https://github.com/open-telemetry/opentelemetry-python/blob/39fa078312e6f41c403aa8cad1868264011f7546/ext/opentelemetry-ext-dbapi/tests/test_dbapi_integration.py#L53
	# This creates span names like mysql.default, mysql.replica, postgresql.staging etc.
	statement_type = extract_statement_type(sql)
	return statement_type unless statement_type.nil?
	# fallback
	database_name ? "mysql.#{database_name}" : 'mysql'
	end

[robertlaurin]

This error is covered by my new test.

[robertlaurin]

We're setting binary to true in redis, and dalli.

opentelemetry-ruby-contrib/instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb

Line 70 in 6df0122

OpenTelemetry::Common::Utilities.utf8_encode(serialized_commands, binary: true)

opentelemetry-ruby-contrib/instrumentation/dalli/lib/opentelemetry/instrumentation/dalli/utils.rb

Line 57 in 6df0122

command = OpenTelemetry::Common::Utilities.utf8_encode(command, binary: true, placeholder: placeholder)

So I think it makes sense here.
https://github.com/open-telemetry/opentelemetry-ruby/blob/18bfd391f2bda2c958d5d6935886c8cba61414dd/common/lib/opentelemetry/common/utilities.rb#L40-L63

[ericmustin]

Ya'll need to stop approving my failing PR lol

@robertlaurin this is merely a reflection of how high your trust battery charge is with me

[arielvalentin]

Are other SQL instrumentations broken in the same way?

[robertlaurin]

Are other SQL instrumentations broken in the same way?

It looks like Trilogy and PG may be vulnerable to the same error. I'll get this out and it see how it does in production before porting it over to the other two.