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
contrib/database/sql: sql comment tag injection experimental feature #1226
Conversation
cdebf6b
to
00d3a32
Compare
ec225b3
to
db2d4e7
Compare
2b556be
to
a962c9c
Compare
137c3dd
to
fd1cd55
Compare
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.
Overall I think this is looking pretty good 😄 Just a couple open questions and mostly minor changes
contrib/database/sql/conn.go
Outdated
// with a span id injected into sql comments. If a span ID is returned, the caller should make sure to use it when creating | ||
// the span following the traced database call. | ||
func (tp *traceParams) withSQLCommentsInjected(ctx context.Context, query string, discardDynamicTags bool) (commentedQuery string, injectedSpanID *uint64) { | ||
// TODO: figure out why tests are failing because the span from context is nil or if there's a different way to |
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.
That's the main thing left for me to figure out. It looks like the span from context is nil
which causes the injection to be skipped. @ajgajg1134 : Was that what you had in mind to get access to the trace id without generating a new span?
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.
Just my usual nits :)
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.
Left a few comments and suggestions. Not sure how "ready for review" the PR is. The description says POC.
contrib/database/sql/conn.go
Outdated
if span, ok := tracer.SpanFromContext(ctx); ok { | ||
spanContext = span.Context() | ||
} | ||
|
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.
Can we remove empty lines around curly braces?
contrib/database/sql/conn.go
Outdated
// withSQLCommentsInjected will return the query with sql comments injected according to the comment injection mode along | ||
// with a span id injected into sql comments. If a span ID is returned, the caller should make sure to use it when creating | ||
// the span following the traced database call. | ||
func (tp *traceParams) withSQLCommentsInjected(ctx context.Context, query string, discardDynamicTags bool) (commentedQuery string, injectedSpanID *uint64) { |
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.
func (tp *traceParams) withSQLCommentsInjected(ctx context.Context, query string, discardDynamicTags bool) (commentedQuery string, injectedSpanID *uint64) { | |
func (tp *traceParams) withSQLCommentsInjected(ctx context.Context, query string, discardDynamicTags bool) (commentedQuery string, injectedSpanID uint64) { |
Why the pointer?
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.
We only conditionally return a span id so I was using nil
as a way to indicate that no span ID was injected. Would you prefer if we just returned the zero value in that case?
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 think so. 0
or a 3rd return argument (ok
or spanok
)
contrib/database/sql/conn.go
Outdated
if tp.cfg.commentInjectionMode != commentInjectionDisabled { | ||
sqlCommentCarrier := tracer.SQLCommentCarrier{} | ||
spanID := random.Uint64() | ||
injectionOpts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags) |
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.
The naming in this PR doesn't really match the style in this repository. If you have a few minutes, do you mind taking a look at the naming guidelines in our style document? https://github.com/DataDog/dd-trace-go/wiki/Style-guidelines#naming-things
injectionOpts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags) | |
opts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags) |
I think opts
should be enough here.
contrib/database/sql/conn.go
Outdated
} | ||
|
||
if tp.cfg.commentInjectionMode != commentInjectionDisabled { | ||
sqlCommentCarrier := tracer.SQLCommentCarrier{} |
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.
sqlCommentCarrier := tracer.SQLCommentCarrier{} | |
var carrier tracer.SQLCommentCarrier |
contrib/database/sql/conn.go
Outdated
@@ -125,8 +157,13 @@ func (tc *tracedConn) QueryContext(ctx context.Context, query string, args []dri | |||
return nil, ctx.Err() | |||
default: | |||
} | |||
rows, err = queryer.Query(query, dargs) | |||
tc.tryTrace(ctx, queryTypeQuery, query, start, err) | |||
commentedQuery, spanID := tc.withSQLCommentsInjected(ctx, query, true) |
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.
commentedQuery, spanID := tc.withSQLCommentsInjected(ctx, query, true) | |
cquery, spanID := tc.withSQLCommentsInjected(ctx, query, true) |
contrib/database/sql/option.go
Outdated
} | ||
} | ||
|
||
func injectionOptionsForMode(mode commentInjectionMode, discardDynamicTags bool) (opts []tracer.InjectionOption) { |
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.
func injectionOptionsForMode(mode commentInjectionMode, discardDynamicTags bool) (opts []tracer.InjectionOption) { | |
func injectionOptionsForMode(mode commentInjectionMode, discardDynamicTags bool) []tracer.InjectionOption { |
I don't think we need named return options here.
contrib/database/sql/rand.go
Outdated
@@ -0,0 +1,55 @@ | |||
// Unless explicitly stated otherwise all files in this repository are licensed |
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.
Please don't copy ddtrace/tracer/rand.go
. Rather move it into the internal
package and reuse it.
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.
Left a few comments and suggestions. Not sure how "ready for review" the PR is. The description says POC.
I updated the PR title to reflect the state. This has been open for a while now but it's been tested internally and we're hoping to do more internal testing soon so we'd like to get this merged in some form that's acceptable.
contrib/internal/sqltest/sqltest.go
Outdated
} | ||
|
||
func expectedInjectedTags(cfg *Config, discardDynamicTags bool) map[string]string { | ||
expectedInjectedTags := make(map[string]string) |
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.
expectedInjectedTags := make(map[string]string) | |
tags := make(map[string]string) |
ddtrace/ddtrace.go
Outdated
@@ -33,6 +33,9 @@ type Tracer interface { | |||
// Inject injects a span context into the given carrier. | |||
Inject(context SpanContext, carrier interface{}) error | |||
|
|||
// InjectWithOptions injects a span context into the given carrier with options. | |||
InjectWithOptions(context SpanContext, carrier interface{}, opts ...InjectionOption) error |
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 have pretty strong feelings about modifying this interface. It is technically a breaking change. Please try another way around it. You could maybe put all this stuff in the span context instead?
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.
Wouldn't adding to the SpanContext
also be updating a public interface and be a breaking change? That said, I would indeed love to make this change less impactful. I followed initial suggestions from the team to use the propagator injection and carrier interfaces but the constraints are limiting. It would be easier to do some things if contrib/database/sql
could pull data from the span context like the service name, env, version in addition to the span id, trace id and sampling priority.
I'd be interested in chatting about how to potentially rethink how the new feature is implemented while offering the same functionality. I've been discovering the code base with this change but I still don't have the full context that you have.
contrib/database/sql/option.go
Outdated
// WithCommentInjection enables injection of tags as sql comments on traced queries. | ||
// This includes dynamic values like span id, trace id and sampling priority which can make queries | ||
// unique for some cache implementations. Use WithStaticTagsCommentInjection if this is a concern. | ||
func WithCommentInjection() Option { |
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.
There are too many new options. Why not simply?
func WithCommentInjection() Option { | |
func WithCommentInjection(mode CommentInjectionMode) Option { |
And then mode
can be any of the existing modes which already have their type and variations (but they are not exported).
2c955fd
to
903890b
Compare
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.
looking pretty good! Just some small comments here and there
2ad59a6
to
4878ab6
Compare
ddtrace/tracer/textmap.go
Outdated
@@ -165,13 +169,19 @@ func getPropagators(cfg *PropagatorConfig, env string) []Propagator { | |||
if cfg.B3 { | |||
defaultPs = append(defaultPs, &propagatorB3{}) | |||
} | |||
if cfg.SQLCommentInjectionMode > CommentInjectionDisabled { |
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.
This approach assumes a specific order of definition of those constants, or certain values. I'd rather use !=
to avoid surprises. The outcome should be the same.
Same thing below on line 182.
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.
You're like my own internal voice, @gbbr. That was one thought I was wondering about when writing this. I'll change.
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.
Looks good to me overall just a few small nits left from me :)
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
This reverts commit ea84e8c.
c3ced37
to
3e87cec
Compare
I'm a little puzzled about the codecov failure because the new code I added is pretty well covered (93% of I do get satisfaction from having great test coverage but it looks like the reporting might be showing a flawed picture in this case. I could probably get a few more percentage points by adding a test like Also, it looks like even with the rebase, I'm still not authorized to merge: Also, do you have links/docs on how when things get a proper release? I can just use the commit hash once this is merged for some things but a proper release version is going to unlock a few more things. |
This is an experimental feature
This is a first implementation using sql comment injection to propagate tracing information to the database query activity. The feature has three modes:
Implementation notes
This is implemented as a new carrier:
SQLCommentCarrier
which encodes a set of tags according to the sqlcommenter spec. The feature is driven by thecommentInjectionMode
which can be set by using one of theWithStaticTagsCommentInjection
/WithCommentInjection
onsqltrace.Register
orsqltrace.Open
.In order for the tracer to do that injection, I introduced a new separate propagator-like interface called
ExperimentalInjector
with methodInjectWithOptions
. It could have been a new method on the existingPropagator
interface but this would have broken existing external implementations of the interface. TheInjectWithOptions
is needed because the client (contrib/database/sql
) callingInjectWithOptions
needs more control over which keys are injected and be able to provide some guarantees on those key names (which need to be short because there are space constraints in databases' collected query text).