Fix sql comment propagation full mode when tracing is disabled
#2866
Conversation
| tags[Ext::KEY_TRACEPARENT] = | ||
| Tracing::Distributed::TraceContext.new(fetcher: nil).send(:build_traceparent, trace_op.to_digest) | ||
| # When tracing is disabled, trace_operation is a dummy object that does not contain data to build traceparent | ||
| if datadog_configuration.tracing.enabled |
There was a problem hiding this comment.
Propagation could happen even if tracing is disabled?
There was a problem hiding this comment.
Yes, when tracing is disabled. Our API returns dummy objects, see
https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/tracing/tracer.rb#L136
https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/tracing/tracer.rb#L518-L527
There was a problem hiding this comment.
Interesting.
But if tracing is disabled, do we call the tracing library for instrumenting MySQL and PG?
It feels weird to see the places prepend_comment is called and have tracing disabled.
I would assume that if tracing is disabled, we wouldn't even instrument MySQL and PG. Is that not correct?
There was a problem hiding this comment.
@GustavoCaso , when tracing is disabled, we are still instrumenting application, but the trace operation and span operation are dummy data to comply with our public API for user using manual instrumentation.
There was a problem hiding this comment.
If we return a dummy trace and span operation when tracing is disabled, we should make them behave like a dummy object that responds to the same methods as a regular trace or span operation. Those methods must probably be no-op. Basically, create an object duck type like a span or trace, rather than check if tracing is enabled through the codebase.
That change would be a more involved one, that should probably be tackled on a separate PR.
I will approve the PR, but I want to make sure that we try to tackle this in the future in a more sustainable way.
Codecov Report
@@ Coverage Diff @@
## master #2866 +/- ##
==========================================
+ Coverage 98.09% 98.10% +0.01%
==========================================
Files 1269 1264 -5
Lines 69969 70030 +61
Branches 3161 3174 +13
==========================================
+ Hits 68633 68702 +69
+ Misses 1336 1328 -8 see 30 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
TonyCTHsu
deleted the
tonycthsu/fix-type-error-for-sql-comment-propagation
branch
What does this PR do?
When sql comment propagation with
fullmode, but tracing is disabled, ends up withTypeErrorduring buildtraceparent.When tracing is disabled, there are not sufficient data to construct
traceparent.