ddtrace/opentracer: implement Context Extension #855

sjmtan · 2021-02-23T19:11:47Z

Solves #813.

This implements the OpenTracing context extension. If a span was started by a Datadog OpenTracer, then the underlying object should be a pointer to a opentracer.span. If that is the case, we're able to use the context with the underlying Datadog Span (of the opentracer.span).

I've tested this locally to connect opentracing spans with Datadog spans assuming I set the opentracing.SetGlobalTracer(datadogOpenTracer).

sjmtan · 2021-02-23T19:35:05Z

I can't set the milestone btw.

gbbr

This is very interesting. We'll have to run some tests. I'm hoping to get a chance to look at it soon. This could potentially solve a big limitation if it does what it claims it does 🙂

gbbr

Thanks. I think it's fine to do this. AFAIU this simply adds more context into the uhm... context 🙂

ddtrace/opentracer/tracer.go

sjmtan · 2021-02-26T18:55:54Z

@gbbr for what its worth, this only solves part of the problem. This allows using opentracing as a parent for DD spans, but no ability to use a DD span as a parent for opentracing spans.

This is because DD span needs to call opentracing.ContextWithSpan in order for that work.

sjmtan · 2021-02-26T18:56:50Z

This actually means that barring a significant change to support that (I tried and failed without having to rewrite a bunch of stuff), we'll be moving off ddtrace completely and adopting opentracing only.

nishkala-square · 2021-02-26T22:06:22Z

I still find this to be a useful change and hope we can merge it even though it only solves half the issue. While it would be great if we could also implement something to work in the other direction, this still allows us to use some of the extensions (such as database tracing)

gbbr

This is fine by me to merge. We are currently in code freeze so I'll leave it here until that ends, and merge it afterwards.

@gbbr for what its worth, this only solves part of the problem. This allows using opentracing as a parent for DD spans, but no ability to use a DD span as a parent for opentracing spans.

This is because DD span needs to call opentracing.ContextWithSpan in order for that work.

@shannontan-bolt I see. I'm happy for us to fix that, if we can work around some restrictions. For us, it was a hard requirement that the Datadog tracer (ddtrace package) never imports opentracing. If we can solve this somehow and keep this requirement, I'm open.

ddtrace/opentracer/tracer.go

sjmtan · 2021-03-01T18:07:32Z

@gbbr - I don't see any good way of doing that since somehow, you'll need to set the context within opentracing when starting a span from within ddtrace. Therefore, you'll eventually need to call opentracing.ContextWithSpan.

One thought I had, but no good way to do it is: when ddtrace is used with opentracing, actually use the opentracer instead of using a global tracer. opentracer can call opentracing.ContextWithSpan before returning a started span.

sjmtan · 2021-03-04T04:33:59Z

@gbbr - when is the code freeze over? Since this is a blocker for me a bit, I might need to fork this temporarily if the code freeze/time to release is a while.

gbbr · 2021-03-04T09:36:19Z

@gbbr - when is the code freeze over? Since this is a blocker for me a bit, I might need to fork this temporarily if the code freeze/time to release is a while.

If you don't mind, please go ahead and fork it. It will be a good way to test. Code freeze is over in about a week generally if all goes well, but then we'll still have to wait again until the next version.

gbbr

Thanks for this! I assume you've tested it and it does what you expect it to :)

ddtrace/opentracer/tracer.go

gbbr

Thanks! Just a few nits.

ddtrace/opentracer/tracer_test.go

gbbr

@shannontan-bolt would love to get this merged for you, are you still interested in doing the last few final changes?

ddtrace/opentracer/tracer.go

Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>

gbbr · 2021-04-19T08:39:50Z

@shannontan-bolt since this was a popular PR and haven't heard back from you, I took the liberty to commit into your PR. I hope that's ok.

sjmtan · 2021-04-19T18:24:32Z

@gbbr that's awesome, thanks!

* 'v1' of https://github.com/DataDog/dd-trace-go: Implement DD_PROFILING_OUTPUT_DIR for dev/debug (DataDog#924) contrib/go-pg/pg.v10: add INTEGRATION flag check for tests (DataDog#921) contrib/go-redis/redis.v8: optimize BeforeProcess and BeforeProcessPipeline (DataDog#920) CI: check go.sum is up-to-date (DataDog#918) README: Fix go get instructions (DataDog#913) internal/log: Improve API for testing (DataDog#916) ddtrace/tracer: add support for agent discovery endpoint, feature flags, stats & drops (DataDog#859) internal/version: bump to v1.31.0 (DataDog#910) CONTRIBUTING.md: add link to contrib/README.md (DataDog#802) profiler: Disable agentless mode for WithAPIKey (DataDog#906) contrib/Shopify/sarama: fix possible deadlock in WrapAsyncProducer (DataDog#907) contrib/database/sql.tracedConn implement driver.SessionResetter (DataDog#908) ddtrace/opentracer: consider FollowsFrom references as children (DataDog#905) ddtrace/opentracer: add support for opentracing.TracerContextWithSpanExtension (DataDog#855) ddtrace/tracer: follow noDebugStack setting when using SetTag with an error (DataDog#900) contrib/net/http: add a getter to retrieve original round tripper (DataDog#903) ddtrace/tracer: ensure Flush call is synchronous (DataDog#901)

sjmtan mentioned this pull request Feb 24, 2021

question: opentracing.SpanFromContext(request.Context()) returns nil inside a datadog span #813

Closed

gbbr changed the title ~~Implement Context Extension~~ ddtrace/opentracer: implement Context Extension Feb 24, 2021

gbbr reviewed Feb 24, 2021

View reviewed changes

gbbr reviewed Feb 26, 2021

View reviewed changes

ddtrace/opentracer/tracer.go Outdated Show resolved Hide resolved

ddtrace/opentracer/tracer.go Outdated Show resolved Hide resolved

ddtrace/opentracer/tracer.go Show resolved Hide resolved

gbbr added this to the 1.30.0 milestone Feb 26, 2021

gbbr previously approved these changes Mar 1, 2021

View reviewed changes

gbbr reviewed Mar 1, 2021

View reviewed changes

ddtrace/opentracer/tracer.go Show resolved Hide resolved

sjmtan dismissed gbbr’s stale review via 047de40 March 1, 2021 17:58

sjmtan mentioned this pull request Mar 10, 2021

Implement Context Extension + Allow FollowsFromRef BoltApp/dd-trace-go#1

Merged

gbbr previously approved these changes Mar 12, 2021

View reviewed changes

gbbr reviewed Mar 12, 2021

View reviewed changes

ddtrace/opentracer/tracer.go Show resolved Hide resolved

sjmtan dismissed gbbr’s stale review via 2d0ba8a March 29, 2021 23:31

sjmtan force-pushed the shannontan/openTracingContextExtension branch from c68fa21 to e576811 Compare March 29, 2021 23:57

sjmtan mentioned this pull request Mar 30, 2021

opentracer: add support for FollowsFromRef #883

Closed

gbbr reviewed Mar 30, 2021

View reviewed changes

gbbr reviewed Apr 2, 2021

View reviewed changes

alebabai reviewed Apr 6, 2021

View reviewed changes

ddtrace/opentracer/tracer.go Show resolved Hide resolved

KarthikNayak mentioned this pull request Apr 19, 2021

question: opentracing: is the chi middleware compatible with opentracing? #904

Closed

Shannon Tan and others added 5 commits April 19, 2021 11:36

Implement context extension

5c9e753

Implement context extension

c38337a

Update ddtrace/opentracer/tracer.go

764a0fe

Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>

Update ddtrace/opentracer/tracer.go

6651b6a

Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>

Fix for comment

39fc7d9

Shannon Tan and others added 6 commits April 19, 2021 11:36

Fix

fc46458

Add test

3cea37c

Add test

5bc3ecc

Fix test

31a4661

Fix test

6509ca3

Auto-address comments

7239b9c

gbbr force-pushed the shannontan/openTracingContextExtension branch from ca55feb to 7239b9c Compare April 19, 2021 08:36

gbbr approved these changes Apr 19, 2021

View reviewed changes

gbbr merged commit 58da3e4 into DataDog:v1 Apr 19, 2021

sjmtan deleted the shannontan/openTracingContextExtension branch April 19, 2021 18:24

ddtrace/opentracer: implement Context Extension #855

ddtrace/opentracer: implement Context Extension #855

Uh oh!

Conversation

sjmtan commented Feb 23, 2021

Uh oh!

sjmtan commented Feb 23, 2021

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sjmtan commented Feb 26, 2021

Uh oh!

sjmtan commented Feb 26, 2021

Uh oh!

nishkala-square commented Feb 26, 2021

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjmtan commented Mar 1, 2021

Uh oh!

sjmtan commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gbbr commented Mar 4, 2021

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gbbr commented Apr 19, 2021

Uh oh!

sjmtan commented Apr 19, 2021

Uh oh!

Uh oh!

sjmtan commented Mar 4, 2021 •

edited

Loading