feat: collect db.collection_name attribute - pg instrumentation #4
Conversation
| @@ -86,11 +86,13 @@ def lru_cache | |||
| # module size limit! We can't win here unless we want to start | |||
| # abstracting things into a million pieces. | |||
| def span_attrs(kind, *args) | |||
| text = args[0] | |||
There was a problem hiding this comment.
I've received feedback before discouraging small refactors like this for object allocation concerns. I don't think you need to change anything, just a head's up.
There was a problem hiding this comment.
Good to know thank you ◡̈
| collection_name = collection_name(text) | ||
| attrs['db.collection.name'] = collection_name unless collection_name.nil? |
There was a problem hiding this comment.
Same here. If collection_name is only used to set that value, you might have it recommended to write this as:
attrs['db.collection.name'] = collection_name(text)Also, the key/value pairs with nil values will get removed on line 112 with attrs.compact!, so it's okay to set the key even if the return value is nil.
There was a problem hiding this comment.
Great feedback! I didn't even realize compact! removes nil values.
| }, | ||
| { | ||
| "name": "insert_into", | ||
| "sql": "INSERT INTO X values('', 'a''b c',0, 1 , 'd''e f''s h')" |
| # 1. Build the opentelemetry/opentelemetry-ruby-contrib image | ||
| # - docker-compose build | ||
| # 2. Bundle install | ||
| # - docker-compose run ex-instrumentation-pg-test bundle install | ||
| # 3. Run test suite | ||
| # - docker-compose run ex-instrumentation-pg-test bundle exec rake test | ||
| # 3. Install the Appraisal gem (https://github.com/thoughtbot/appraisal) |
There was a problem hiding this comment.
The apprasial gem should already be installed from step 2, maybe something like: "Install the dependencies for each Appraisal" or "Install the Appraisals"? I like your link to the gem :)
There was a problem hiding this comment.
Yes that is better!
| name = test_case['name'] | ||
| query = test_case['sql'] | ||
|
|
||
| define_method(:"test_sql_obfuscation_#{name}") do |
There was a problem hiding this comment.
I think this might've been a copy-paste miss! Maybe "test_sql_table_name_" instead?
No description provided.