feat: Add Trilogy Auto Instrumentation #1063
Conversation
arielvalentin
requested review from
ahayworth,
dazuma,
ericmustin,
fbogsany,
mwear and
robertlaurin
as code owners
Provides auto instrumentation for the [trilogy database driver](https://github.com/github/trilogy)
arielvalentin
force-pushed
the
arielvalentin/trilogy-instrumentation
branch
from
f942308
to
dfd803b
Compare
|
just ack'ing this, will try to give this a thorough review otw. |
There was a problem hiding this comment.
Good stuff! A few very small nits, only thing that's blocking is I think we need to add semantic-conventions as a dependancy.
I should mention that I do have some hesitation about :obfuscate being the default setting here, since it creates a delta between the mysql2 and the trilogy instrumentation, and could add a performance overhead by default that may be unexpected. But as you're the only real user of this at the moment I'd prefer to do what's best for our actual user's use case, which apparently is to :obfuscate by default.
Anyway, again awesome stuff ty for this, and your many other, contributions.
| multi_line_comments | ||
| ].freeze | ||
|
|
||
| FULL_SQL_REGEXP = Regexp.union(MYSQL_COMPONENTS.map { |component| COMPONENTS_REGEX_MAP[component] }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I created an issue based on your comment.
| end | ||
|
|
||
| compatible do | ||
| Gem::Requirement.create('>= 2.0', '< 3.0').satisfied_by?(Gem::Version.new(::Trilogy::VERSION)) |
There was a problem hiding this comment.
❤️ +1 to max version, thank you for this. i'd say let's make it a constant on the class but, i suppose we should let the related proposals/PR about this shake out before informally creating any conventions here.
instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb
Outdated
Show resolved
Hide resolved
|
|
||
| def client_attributes | ||
| attributes = { ::OpenTelemetry::SemanticConventions::Trace::DB_SYSTEM => 'mysql' } | ||
| attributes[::OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME] = @connected_host if defined?(@connected_host) |
There was a problem hiding this comment.
I peaked into https://github.com/github/trilogy/blob/9380a5844c844dd13ddd6dc761895898b17186a8/contrib/ruby/lib/trilogy.rb a bit and indeed it doesn't look like too much is available via public ruby methods for us to grab extra metadata at present. So it goes (awesome gem tho, thank you to all the great GH folks for open sourcing it! 🎉 ).
That being said all that's required according to the specification (which itself is experimental so it leaves us wiggle room) is db.system and either net.peer.name || net.peer.ip. So I think we're covered here. My only incredibly small bikeshedding thing is maybe we should set this to something like unknown in the event that @connected_host isn't defined, since downstream processors may assume that either net.peer.name or net.peer.ip will always exist? Idk, not blocking at all, and given that your org is literally the only user of this instrumentation at the moment, I don't think it matters too much and defer to you on this.
There was a problem hiding this comment.
db.system and either net.peer.name || net.peer.ip
TIL! Let me see what I can do. Perhaps I can use the original host provided to the connection and switch to the connected host when it is set.
|
Thanks @ericmustin although I think that I'm the case of the UDS the host name will be nil and I don't want to trigger and error so I'm going to set a default there. Although not as common, I think that we need some additional coverage for those cases for all of the drivers. |
arielvalentin
force-pushed
the
arielvalentin/trilogy-instrumentation
branch
from
a195f6c
to
027cba1
Compare
Provides auto instrumentation for the trilogy database driver