Implement method tracing API #2241
Conversation
jennchenn
added
integrations
There was a problem hiding this comment.
This is looking very nice. Clean, easy to understand, docs are good... solid work.
Path to merge involves:
- Moving some internals around, to make sure we're not introducing co-dependencies and are preserving the relationship between namespaces e.g.
TracingandContrib. - Describe this as a "beta" feature: that it's subject to breaking changes.
- Adding some kind of check & warning when
trace_methodis used withoutdatadog-instrumentation(as it currently must be included manually as a soft dependency by users).
Let me know if I can clarify the asks/needs here at all!
| module Tracing | ||
| module Contrib | ||
| # Class defining hook used to implement method tracing | ||
| class Hook |
There was a problem hiding this comment.
Overall looks very clean! May want to add some YARD doc comments if possible.
lib/datadog/tracing/tracer.rb
Outdated
| # @param [String] target Defines the method to be traced | ||
| # @param [Hash] span_options Optional value to be added to the produced span | ||
| # @return [Datadog::Tracing::Contrib::Hook] The newly defined Datadog::Tracing::Contrib::Hook object | ||
| def trace_method(target, name, span_options = {}) |
There was a problem hiding this comment.
We probably need to migrate this to Contrib::Extensions or declare this within module Tracing from a file within the contrib directory.
Reason being we would like the tracing core to be independent of the integrations. Thus Contrib can depend on things in Tracing but not vice versa.
Also, given this function doesn't actually use the Tracer at all, I think extracting this method will help keep this class a bit simpler (fewer responsibilities).
docs/GettingStarted.md
Outdated
|
|
||
| To generate traces, use the `Datadog::Tracing.trace_method` method: | ||
| ```ruby | ||
| Datadog::Tracing.trace_method(name, target, span_options).around do |env, span, trace, &block| |
There was a problem hiding this comment.
Does this need to be updated to reflect target, name instead of name, target?
ddtrace.gemspec
Outdated
| @@ -69,5 +69,8 @@ Gem::Specification.new do |spec| | |||
| # Used by profiling (and possibly others in the future) | |||
| spec.add_dependency 'libdatadog', '~> 0.7.0.1.1' | |||
|
|
|||
| # Used by method tracing API | |||
| spec.add_dependency 'datadog-instrumentation' | |||
There was a problem hiding this comment.
Because this package is not published to RubyGems we can't merge this with this dependency as-is.
Eventually we should publish this, but in the interim, we could introduce it as a soft dependency: remove it from this file, and add a check to trace_method that looks for the presence of Datadog::Instrumentation.
- If present, use the package and proceed as normal.
- If not present, log a warning and suggestion to add
datadog-instrumentationto theGemfile.
This way, users could opt-in to use this as a "beta" feature while we polish up datadog-instrumentation for it's debut, without causing errors.
jennchenn
force-pushed
the
method-tracing/poc
branch
from
e63d133
to
44a3818
Compare
jennchenn
force-pushed
the
method-tracing/poc
branch
from
44a3818
to
9b8b97a
Compare
WIP: Some tasks to be done
3.2at the momentdisable/enablefunctionality does not seem to be working as intended on instrumentation gem side (i.e. even when hook is disabled, it still produces traces)What does this PR do?
This PR adds a method tracing API that can be used by:
Motivation
Method tracing has been requested for a long time by the community. This implementation makes use of the instrumentation API to provide users with an easy interface to implement custom instrumentation.
Additional Notes
The number of changed files is inflated because of the changes to the gemfile. Main change is in the
tracing/contrib/hook.rbfile.How to test the change?
Unit tests have been added to test the changes.
pgandmysql2instrumentations were modified as a POC that the API works (the original instrumentation code was not deleted in this branch).