Convert mysql2 integration to Datadog::Instrumentation API #1657
Conversation
delner
added
integrations
| @@ -61,6 +61,8 @@ end | |||
| # @see https://github.com/DataDog/dogstatsd-ruby/issues/182 | |||
| gem 'dogstatsd-ruby', '>= 3.3.0', '!= 5.0.0', '!= 5.0.1', '!= 5.1.0' | |||
| gem 'opentracing', '>= 0.4.1' | |||
| # TODO: Promote this to ddtrace.gemspec. (Or contrib package? Only used by contrib.) | |||
There was a problem hiding this comment.
Should we? In my opinion this kind of "small base building block" functionality could live in a separate package without much effort. In my mind it would quickly reach stable status and then receive mostly bug fixes, which users can receive effortlessly with semver.
There was a problem hiding this comment.
In my opinion this kind of "small base building block" functionality could live in a separate package without much effort.
Can you clarify what you mean? In order to use this gem we need to add it to the gemspec, otherwise user applications won't import it and the integration won't work, right?
|
This looks nice to me as a first validation step. It will get better with the tracer instrumentation DSL we talked about though, which we should probably wait for before merging so as not to have yet another way to do things, since it's not time sensitive. |
| Contrib::Analytics.set_sample_rate(span, analytics_sample_rate) if analytics_enabled? | ||
| if Datadog.configuration[:mysql2][:analytics_enabled] | ||
| Contrib::Analytics.set_sample_rate(span, Datadog.configuration[:mysql2][:analytics_sample_rate]) | ||
| end |
There was a problem hiding this comment.
Not sure if it's "in scope", but have you considered taking this opportunity to move the configuration to be supplied as part of the env, rather than being global state that is accessed directly?
This is similar to how above we use datadog_pin.tracer rather than Datadog.tracer to get the correct tracer instance.
| def get_client(env) | ||
| return unless env[:self].instance_of?(::Mysql2::Client) |
There was a problem hiding this comment.
Should this be is_a?, rather than instance_of? to avoid coupling on the exact class, allowing some flexibility if we get a subclass? (see also https://stackoverflow.com/questions/3893278/ruby-kind-of-vs-instance-of-vs-is-a )
| ::Mysql2::Client.include(Instrumentation) | ||
| # ::Mysql2::Client.include(Instrumentation::Client) | ||
|
|
||
| Datadog::Instrumentation::Hook['Mysql2::Client#query'].add do |
There was a problem hiding this comment.
Would it make sense to use the class name directly here? E.g. Hook[Mysql2::Client, '#query']?
There was a problem hiding this comment.
This would cause an exception if the constant does not exist (e.g the gem is not in the bundle). Hook handles that internally and add (+install IIRC) becomes NOOP.
There was a problem hiding this comment.
Yes...ish. The current way that dd-trace-rb does it is that there's a separate check for that in integration.rb:
which is why the old code referenced the constant directly.
Using it as a string is good if we could get rid of that separate file, otherwise, we already get the validation for free.
| stack.call(trace_env) | ||
| end | ||
| end | ||
| end.install |
There was a problem hiding this comment.
Is it common to use Hook without performing install immediately afterwards? E.g. would it make sense to make this the default behavior?
There was a problem hiding this comment.
It has been the case in the past, adding multiple hooks on the same hookpoint in different locations, then installing in another part of the lifecycle.
There was a problem hiding this comment.
I think this would be largely solved with the upcoming tracer instrumentation API (being drafted by David), which sits atop of Hook.
This pull request converts the Mysql2 integration over to the
datadog-instrumentationAPI. The changes to this library are entirely internal and do not affect user implementation, or trace data.Work in progress prototype: DO NOT MERGE!
This is intended to be a proof-of-concept in the development of the new instrumentation API. Some things we may want to take into consideration before merging this:
ddtracethat makes the implementation of the API a little more friendly/specific to tracing? (We may only understand the usage patterns better after we convert more integrations and observe the overlap.)