SQL Instrumentation Improvements

[arielvalentin]

not blocking bc it's a huge change and out of scope for this PR but, along with some broad http helpers that oughta be abstracted into a gem, to be DRY we should also probably move the mysql obfuscation regex into a db helper gem of some sort eventually/ideally as an early todo in 2022.

Originally posted by @ericmustin in open-telemetry/opentelemetry-ruby#1063 (comment)

There is structural duplication in SQL instrumentation gems, e.g. obfuscation and semconv, as well as knowledge duplication between the mysql2 and triology instrumentations. To minimize maintenance burden of these libraries it will likely be best to extract reusable code and provides more focused test coverage.

In addition to that there is not any test coverage for UDS connections and should update semantic conventions to reflect these values.

[arielvalentin]

@ahayworth @ericmustin I started to look at this and realized we have a little bit of re-inventing the wheel going on here.

It looks like the original code was based off the NewRelic agent, which has an all-in-one set of Database utilities that supports multiple dialects. It's unclear to me why we didn't migrate this code into this repo, which I imagine has quite a bit of production usage.

Instead of refactoring out common functionality from our classes I would like to propose we revisit this and import the code from the NewRelic agent and credit them with authorship.

https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/new_relic/agent/database/obfuscation_helpers.rb

https://github.com/newrelic/newrelic-ruby-agent/tree/dev/test/fixtures/cross_agent_tests/sql_obfuscation

Apache 2 License https://github.com/newrelic/newrelic-ruby-agent/blob/dev/LICENSE

wdyt?

[ericmustin]

Happy to see this refactored into something reusable and more dry, i forget what the resolution was previously but my understanding is if we have concerns about OSS crediting we can just include a reference to the NR library in a license-3rdparty.csv file rather than adding a dependency, which id prefer to avoid. there were some New Relic folks who were involved in the SIG at the time this was contributed and didn’t raise any objections to our crediting approach (just a code comment iirc)

Example: https://github.com/DataDog/dd-trace-rb/blob/master/LICENSE-3rdparty.csv

[arielvalentin]

I'll clarify what I meant by reusing the NewRelic agent code, it would be a copy/paste/modify of the existing package and we'd credit NewRelic as the source in the Readme and Yardoc.

I'll look into the 3rd party CSV suggestion.

[ahayworth]

The reasons we didn't do this before were:

• We asked if NR would be willing to formally donate the code, but by "asked" I mean "we mentioned it on github somewhere and didn't really pursue it further".
• We added database instrumentation in pieces (pg coming last, if I recall the ordering). I think each PR added what they needed rather than bringing in "unnecessary code". Whoops. 😆

But I think at this point, having a unified set of maintainable code would be an excellent win. We should do it! 😄

[arielvalentin]

Related: open-telemetry/opentelemetry-specification#3127

[arielvalentin]

TIL: https://github.com/open-telemetry/opentelemetry-sqlcommenter

[arielvalentin]

Related: open-telemetry/semantic-conventions#717

[github-actions]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.

[ericmustin]

i'll try to pick this up this week

[kaylareopelle]

Hi @ericmustin! Could I collaborate with you on this issue? I work on New Relic's Ruby agent and am familiar with our SQL instrumentation.

[ericmustin]

Hey @kaylareopelle yea for sure. I had some wip branches here awhile back that I think are probably better to scrap and start from scratch. But I'll try to dig up any relevant branches/code here and link in this issue when I get a chance this wknd, I'm also happy to just assign to you and review any work you do here, either way I'll try to attend Tuesday's SIG mtg prepared to actually either provide guidance or feedback on anything here.

I think the goal was to sortve have a helper class or module that the individual mysql2/trilogy/pg/etc gems could simply inherit from, but iirc the implementation wasn't as clean as I'd have liked and we sortve dropped it and moved on. So it goes

[robbkidd]

During today's SIG, @kaylareopelle, @ericmustin, and I concluded that opentelemetry-instrumentation-base is a reasonable place to put shared logic for SQL instrumentation.

• We acknowledged that the shared logic will not be used by all instrumentation gems, but think -base is a good place anyway.
• The -base gem will be a bit bigger, but we think the trade of disk space for simpler dependency resolution is worth it.
• Gives us an existing place to make progress on the shared logic now without the additional effort and dependency complexity of adding another gem to the contrib mono-repo.