-
Notifications
You must be signed in to change notification settings - Fork 377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OpenTracing:Use Datadog configuration for the OpenTracer instance #2782
Conversation
attr_reader \ | ||
:datadog_tracer | ||
|
||
# (see Datadog::Tracer#initialize) | ||
def initialize(**options) | ||
# Create an OpenTracing::Trace instance that is bound to the active | ||
# Datadog tracer instance. | ||
# To configure this tracer, use the global tracing configuration. | ||
# | ||
# DEV-2.0: Remove options as configuration is done through the global tracing configuration. | ||
def initialize(**ignored_options) | ||
super() | ||
@datadog_tracer = Datadog::Tracing::Tracer.new(**options) | ||
@datadog_tracer = Datadog.send(:components).tracer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the global_tracer
instance is a bit of a singleton that is supposed to live for the whole lifetime of the process, I'm not sure it's correct to try to store a reference to the Datadog::Tracing::Tracer
that was available when it gets initialized.
In fact, I don't think this PR would fix my example from #1563, which I've updated to work with dd-trace-rb 1.x:
require 'opentracing'
require 'ddtrace'
require 'datadog/opentracer'
# # Activate the Datadog tracer for OpenTracing
OpenTracing.global_tracer = Datadog::OpenTracer::Tracer.new
Datadog.configure do |c|
c.diagnostics.debug = true
c.agent.host = 'foo'
end
OpenTracing.start_active_span('operation_name') do |scope|
# do nothing
end
This is because the Datadog::OpenTracer::Tracer
has a reference to the tracer that existed before the configuration block run.
I'm thinking that perhaps the right way to go is to replace the attr_reader :datadog_tracer
with a
def datadog_tracer
Datadog::Tracing.send(:tracer)
end
which would almost ensure that the latest tracer is always the one that gets used...
I was actually testing it out and it still wasn't picking up the configuration because of this monkey patch which I claim also doesn't seem correct with the way that dd-trace-rb 1.x expects components to be used.
After disabling that monkey patch, my example seems to work but -- I may be missing something so take my suggestions with a big grain of salt :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually testing it out and it still wasn't picking up the configuration because of this monkey patch which I claim also doesn't seem correct with the way that dd-trace-rb 1.x expects components to be used.
I think it make sense that if we delegate datadog_tracer
to the current tracer, we should avoid injecting tracer instance for datadog tracing configuration like the monkey patch. This way, the dependency relationships would be more streamlined.
Fixes #1563
We currently create a new instance of
Datadog::Tracing::Tracer
for the OpenTracing integration.This a byproduct of previous implementation details, where creating a new instance of the tracer for distinct use cases was appropriate.
Today, using the global Tracer instance is the correct way to integrate OpenTracing with Datadog tracing: this ensures the OpenTracer respects any configured Datadog configuration. Currently, this configuration is wiped clean when the OpenTracer is created.
There are no documentation changes because the current documentation actually matches the behaviour of this PR.