fix: update pg instrumentation to handle non primitive argument

[ericmustin]

Summary

Addresses #1139

the pg connection_adapter .execute method can be invoked with arguments that are not a String class, like a Arel::Nodes::SqlLiteral.

While Arel::Nodes::SqlLiteral is a String subclass, Our otlp exporter doesn't gracefully handle Subclasses of accepted attribute types (see here), and instead just returns an error and drops the attribute when attempting to encode a non valid type.

I'm hesitant to fiddle with the portion of our OTLP exporter code that handles encoding, as we'd have to be careful not to introduce a regression.

Instead, since we know which instrumentation is problematic and what attributes, it's more straightforward to just coerce that specific attribute to aString class, which is what this PR does (it calls .to_s defensively).

[ahayworth]

I had a question re: the defensive programming, but I don't think it's anything worth blocking the PR over.

This is a good fix - and thank you to @danielmbarlow for finding the issue! As an avid Arel.sql fan myself, we should have been testing for this from the beginning. 🤦 😆

[plantfansam]

👌 lovely! Had a in-line nit that you can take or leave as you wish 😄

[plantfansam]

Nit, but: thoughts on changing to attrs['db.statement'] = sql&.to_s if config[:db_statement] != :omit && !sql.nil? or just attrs['db.statement'] = sql&.to_s if config[:db_statement] != :omit

[ericmustin]

i've updated this to attrs['db.statement'] = sql&.to_s unless config[:db_statement] == :omit

I think this is as concise as i can get and handles all the edge cases and doesnt anger rubocop

[fbogsany]

Is there a circumstance where sql would be nil? That would have been problematic previously, since attribute values have to be non-nil, and validation would have dropped it and logged an error.

[fbogsany]

Note: I realize this has been beaten to death, but I really don't think this level of defensiveness is warranted here.

[ericmustin]

This test case

opentelemetry-ruby/instrumentation/pg/test/opentelemetry/instrumentation/pg/instrumentation_test.rb

Lines 137 to 138 in 06b36a6

	# We should have evicted the statement from the cache
	_(last_span.attributes['db.statement']).must_be_nil

WIthout the defensiveness it seems it's possible that lru_cache returns nil, which then gets encoded to "" by .to_s. This failed test case is what prompted the defensiveness in the first place https://github.com/open-telemetry/opentelemetry-ruby/runs/5524602649?check_suite_focus=true

I hadn't really looked into the lru cache details or instrumentation implementation other than that it does seem possible, in some cases, for sql to be nil, we are currently testing for that case for some reason, and so we should have a nil check.

[fbogsany]

It feels like that's more a case of the cache not working correctly. If the lru_cache returns nil, we should handle that cache miss and update the cache rather than papering over the problem with a &..

[fbogsany]

The lru_cache is a little unusual here - the expectation is that we write into the cache in the PREPARE case and read from it in the EXECUTE case. I think we should handle the sql.nil? case on line 81, where we read from the cache. By the time we get here, sql should be non-nil.

[ericmustin]

I think the idea, (@ahayworth correct me if i'm wrong) is that this specific case is for handling prepared statements. Reviewing the test here, w/an lru cache of size 50:

• user prepares 51 statements
• each time prepare is called, we perform the obfuscation and cache <statement-name>:<obfuscated-prepared-statement> k:v pair.
• The 51st call evicts the least recently used <statement-name>:<obfuscated-prepared-statement> k:v pair
• when the user later attempts to execute that already-evicted <statement-name>:<obfuscated-prepared-statement> k:v pair via .execute(<statement-name>), it causes a cache miss and we can't update the cache because we no longer have access to the <obfuscated-prepared-statement> to set as a value, so the lru_cache returns nil.

I don't know what the improvement would be here, this seems like an expected behavior, albeit one with tradeoffs.

[fbogsany]

🤦 we handle nil attributes on the next line. This is complicated and confusing IMO. I think we should .to_s the result from obfuscate_sql, either in that method or on lines 72 and 77. Then we always cache strings.

[ericmustin]

ok

[ahayworth]

@ericmustin your understanding of the LRU cache is correct, both in functionality and in purpose.

[plantfansam]

LGTM