Skip to content

Conversation

mrdg
Copy link
Contributor

@mrdg mrdg commented Dec 14, 2021

This was introduced in #247 to allow applications to
use the datadog tracing api directly instead of an opentracing compatible version.

The issue is that when UseDatadog is enabled, opentracing.GlobalTracer is set to a noop tracer
so other packages in netlify-commons that emit traces (e.g router) don't work as expected.

We don't actually need this option because opentracer.New already calls tracer.Start so
applications should be able to use both the opentracing and datadog tracing api.

This was introduced in #247 to allow applications to
use the datadog tracing api directly instead of an opentracing compatible version.

The issue is that when `UseDatadog` is enabled, opentracing.GlobalTracer is set to a noop tracer
so other packages in netlify-commons that emit traces (e.g router) don't work as expected.

We don't actually need this option because `opentracer.New` already calls `tracer.Start` so
applications should be able to use both the opentracing and datadog tracing api.
@mrdg mrdg added the type: chore work needed to keep the product and development running smoothly label Dec 14, 2021
@mrdg mrdg requested a review from a team as a code owner December 14, 2021 14:22
Copy link
Contributor

@paulo paulo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@mheffner
Copy link
Contributor

We don't actually need this option because opentracer.New already calls tracer.Start so

Does it call the tracer from ddtrace?

ddtrace and opentracing aren't compatible, are we sure this will propagate traces to a ddtrace only app?

@mrdg
Copy link
Contributor Author

mrdg commented Dec 14, 2021

Does it call the tracer from ddtrace?

Yeah: https://github.com/DataDog/dd-trace-go/blob/v1/ddtrace/opentracer/tracer.go#L35

ddtrace and opentracing aren't compatible, are we sure this will propagate traces to a ddtrace only app?

This change is running on nfsvr staging right now and I don't see any missing traces. Is there something specific I should look for?

@mheffner
Copy link
Contributor

So mongotrace that we use for tracing mongo queries is a ddtrace package. It requires us to use DD tracing or else the mongo spans aren't connected to parent request spans.

@mrdg
Copy link
Contributor Author

mrdg commented Dec 15, 2021

So mongotrace that we use for tracing mongo queries is a ddtrace package. It requires us to use DD tracing or else the mongo spans aren't connected to parent request spans.

Right, yeah as long as those parent spans are created with the ddtrace package that will still work. I just tested it and and the mongo spans are still connected to the http request spans. The only effect of this change is that traces created via the opentracing package (such as the ones created in netlify-commons/router) are now also emitted.

Side note: longer term we should really pick one approach

@mrdg mrdg merged commit 8bc165b into master Dec 15, 2021
@mrdg mrdg deleted the mrdg/opentracer-fix branch December 15, 2021 10:13
@mheffner
Copy link
Contributor

Yeah, I guess when I tested in this in the past the parent http spans were created with OT so they didn't connect since DD uses a different context key. It sounds like we are still using DD for those http spans but just using OT for commons router?

@mrdg
Copy link
Contributor Author

mrdg commented Dec 15, 2021

Yeah, I guess when I tested in this in the past the parent http spans were created with OT so they didn't connect since DD uses a different context key. It sounds like we are still using DD for those http spans but just using OT for commons router?

yes exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants