contrib/database/sql: sql comment tag injection experimental feature #1226

alexandre-normand · 2022-03-29T20:49:30Z

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:

Disabled (default): Doesn't inject any sql comments on queries
Static tags only: Injects static tags as sql comments. This mode can be used with databases/deployments where dynamic tags would incur a cost by essentially making every query unique and losing benefits of caching. Examples of this would be sqlserver query plan cache or client side caching middlewares.
- Service name (the application/DD_SERVER and not the database service)
- Environment
- Service version (again, the application version and not the database version)
Standard mode: Injects both static tags that are shared for the duration of an application (listed above) but also dynamic ones that are specific to a span make each query unique when comments are considered. This mode is the full featured one and can enable more product functionality. Note that for prepared statements, the tracer can't include dynamic tags and reverts back to what is essentially the static tags only mode.
- Trace ID
- Span ID
- Sampling priority

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 the commentInjectionMode which can be set by using one of the WithStaticTagsCommentInjection / WithCommentInjection on sqltrace.Register or sqltrace.Open.

In order for the tracer to do that injection, I introduced a new separate propagator-like interface called ExperimentalInjector with method InjectWithOptions. It could have been a new method on the existing Propagator interface but this would have broken existing external implementations of the interface. The InjectWithOptions is needed because the client (contrib/database/sql) calling InjectWithOptions 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).

contrib/database/sql/conn.go

README.md

ajgajg1134

Overall I think this is looking pretty good 😄 Just a couple open questions and mostly minor changes

contrib/database/sql/conn.go

contrib/database/sql/option.go

contrib/database/sql/stmt.go

ddtrace/tracer/propagator.go

ddtrace/tracer/sqlcomment.go

ddtrace/tracer/option.go

contrib/database/sql/conn.go

contrib/database/sql/stmt.go

alexandre-normand · 2022-05-10T23:52:52Z

contrib/database/sql/conn.go

+// 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


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?

ddtrace/tracer/option.go

gbbr

Just my usual nits :)

ddtrace/tracer/textmap.go

gbbr

Left a few comments and suggestions. Not sure how "ready for review" the PR is. The description says POC.

gbbr · 2022-05-16T09:21:04Z

contrib/database/sql/conn.go

+	if span, ok := tracer.SpanFromContext(ctx); ok {
+		spanContext = span.Context()
+	}
+


Can we remove empty lines around curly braces?

gbbr · 2022-05-16T09:21:14Z

contrib/database/sql/conn.go

+// 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) {


Suggested change

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?

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?

I think so. 0 or a 3rd return argument (ok or spanok)

gbbr · 2022-05-16T09:22:28Z

contrib/database/sql/conn.go

+	if tp.cfg.commentInjectionMode != commentInjectionDisabled {
+		sqlCommentCarrier := tracer.SQLCommentCarrier{}
+		spanID := random.Uint64()
+		injectionOpts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags)


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

Suggested change

injectionOpts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags)

opts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags)

I think opts should be enough here.

gbbr · 2022-05-16T09:22:38Z

contrib/database/sql/conn.go

+	}
+
+	if tp.cfg.commentInjectionMode != commentInjectionDisabled {
+		sqlCommentCarrier := tracer.SQLCommentCarrier{}


Suggested change

sqlCommentCarrier := tracer.SQLCommentCarrier{}

var carrier tracer.SQLCommentCarrier

gbbr · 2022-05-16T09:25:03Z

contrib/database/sql/conn.go

@@ -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)


Suggested change

commentedQuery, spanID := tc.withSQLCommentsInjected(ctx, query, true)

cquery, spanID := tc.withSQLCommentsInjected(ctx, query, true)

gbbr · 2022-05-16T09:30:06Z

contrib/database/sql/option.go

+	}
+}
+
+func injectionOptionsForMode(mode commentInjectionMode, discardDynamicTags bool) (opts []tracer.InjectionOption) {


Suggested change

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.

gbbr · 2022-05-16T09:31:52Z

contrib/database/sql/rand.go

@@ -0,0 +1,55 @@
+// Unless explicitly stated otherwise all files in this repository are licensed


Please don't copy ddtrace/tracer/rand.go. Rather move it into the internal package and reuse it.

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.

gbbr · 2022-05-16T09:32:45Z

contrib/internal/sqltest/sqltest.go

+}
+
+func expectedInjectedTags(cfg *Config, discardDynamicTags bool) map[string]string {
+	expectedInjectedTags := make(map[string]string)


Suggested change

expectedInjectedTags := make(map[string]string)

tags := make(map[string]string)

gbbr · 2022-05-16T09:35:12Z

ddtrace/ddtrace.go

@@ -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


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?

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.

gbbr · 2022-05-16T09:37:14Z

contrib/database/sql/option.go

+// 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 are too many new options. Why not simply?

Suggested change

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).

ajgajg1134

looking pretty good! Just some small comments here and there

contrib/internal/sqltest/sqltest.go

ddtrace/tracer/option.go

ddtrace/tracer/spancontext.go

ddtrace/tracer/tracer.go

gbbr · 2022-05-31T15:18:21Z

ddtrace/tracer/textmap.go

@@ -165,13 +169,19 @@ func getPropagators(cfg *PropagatorConfig, env string) []Propagator {
 	if cfg.B3 {
 		defaultPs = append(defaultPs, &propagatorB3{})
 	}
+	if cfg.SQLCommentInjectionMode > CommentInjectionDisabled {


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.

You're like my own internal voice, @gbbr. That was one thought I was wondering about when writing this. I'll change.

ajgajg1134

Looks good to me overall just a few small nits left from me :)

contrib/database/sql/conn.go

ddtrace/tracer/sqlcomment.go

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

This reverts commit ea84e8c.

alexandre-normand · 2022-06-15T20:23:49Z

I think it's good to merge any time you like. If possible, it'd be nice if we could increase the code coverage a bit on tests, but it's not mandatory. You'll also need to rebase on main to be able to merge.

I'm a little puzzled about the codecov failure because the new code I added is pretty well covered (93% of sqlcomment.go and the new logic in contrib/database/sql is covered as well). Looking at the report is not very helpful as it's showing uncovered lines in code unaffected by this PR 🤔 .

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 SQLCommentCarrier.Extract returns nil but that seems a little like cheating just to get numbers higher?

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.

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch from cdebf6b to 00d3a32 Compare March 30, 2022 01:41

gbbr changed the title ~~SQL commenter proof of concept~~ contrib/database/sql: commenter proof of concept Mar 30, 2022

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch 2 times, most recently from ec225b3 to db2d4e7 Compare May 5, 2022 02:58

alexandre-normand changed the base branch from dbm-apm to v1 May 5, 2022 03:42

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch 2 times, most recently from 2b556be to a962c9c Compare May 5, 2022 18:58

alexandre-normand commented May 5, 2022

View reviewed changes

contrib/database/sql/conn.go Show resolved Hide resolved

README.md Show resolved Hide resolved

alexandre-normand marked this pull request as ready for review May 5, 2022 22:48

alexandre-normand requested a review from a team May 5, 2022 22:48

alexandre-normand requested a review from a team as a code owner May 5, 2022 22:48

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch from 137c3dd to fd1cd55 Compare May 10, 2022 18:29

ajgajg1134 requested changes May 10, 2022

View reviewed changes

alexandre-normand commented May 11, 2022

View reviewed changes

ajgajg1134 reviewed May 11, 2022

View reviewed changes

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

gbbr reviewed May 13, 2022

View reviewed changes

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

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

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

gbbr suggested changes May 16, 2022

View reviewed changes

alexandre-normand changed the title ~~contrib/database/sql: commenter proof of concept~~ contrib/database/sql: sql comment tag injection experimental feature May 18, 2022

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch 2 times, most recently from 2c955fd to 903890b Compare May 26, 2022 05:16

alexandre-normand added this to the 1.39.0 milestone May 26, 2022

alexandre-normand requested a review from ajgajg1134 May 26, 2022 17:15

ajgajg1134 requested changes May 26, 2022

View reviewed changes

contrib/internal/sqltest/sqltest.go Outdated Show resolved Hide resolved

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

ddtrace/tracer/spancontext.go Show resolved Hide resolved

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

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch from 2ad59a6 to 4878ab6 Compare May 27, 2022 02:20

alexandre-normand requested review from ajgajg1134 and gbbr May 27, 2022 02:51

gbbr reviewed May 31, 2022

View reviewed changes

alexandre-normand requested a review from gbbr May 31, 2022 15:57

ajgajg1134 requested changes Jun 2, 2022

View reviewed changes

contrib/database/sql/conn.go Outdated Show resolved Hide resolved

contrib/database/sql/conn.go Outdated Show resolved Hide resolved

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

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

alexandre-normand requested a review from ajgajg1134 June 2, 2022 18:27

alexandre-normand and others added 22 commits June 15, 2022 13:11

Inline sqlcommenter implementation

e22347a

Fix mode handling bug

3576b56

Add godoc to mockdriver

39b6a53

Fix tests

d3605f0

Update contrib/database/sql/conn.go

452d890

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

Update contrib/database/sql/conn.go

8415c84

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

Update ddtrace/tracer/sqlcomment.go

7e742dc

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

Use span id instead of allowing options

bd54cb3

Address some but not all comments

b351637

Update contrib/database/sql/internal/mockdriver.go

ffc7af4

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

Update contrib/database/sql/internal/mockdriver.go

42f27e9

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

Update ddtrace/tracer/sqlcomment.go

2a0dbed

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

Rename sqlcomment test cases

cb5a1b8

Gofmt mockdriver.go

8a2d5d4

More Renaming

04b59ca

Add Benchmark and Include an Optimized Implementation of CommentQuery

3cfb8d0

Revert "Use span id instead of allowing options"

037c0f7

This reverts commit ea84e8c.

Address some more comments

685ee1f

Tweak test to increase confidence that sql comment span ids are new

d39e3fb

Address a few more

6b53e19

Make SQLCommentInjection mode a string

b92c0ef

Reorder imports

3e87cec

alexandre-normand dismissed gbbr’s stale review via 3e87cec June 15, 2022 20:13

alexandre-normand force-pushed the alex.normand/sql-commenter-proof-of-concept branch from c3ced37 to 3e87cec Compare June 15, 2022 20:13

gbbr approved these changes Jun 15, 2022

View reviewed changes

gbbr merged commit e2a9297 into main Jun 15, 2022

gbbr deleted the alex.normand/sql-commenter-proof-of-concept branch June 15, 2022 20:30

alexandre-normand mentioned this pull request Aug 18, 2022

Use span read lock when reading span meta from span context #1437

Merged

alexandre-normand mentioned this pull request Aug 26, 2022

contrib/database/sql: add db service name tag and _dd.dbm_trace_injected span tag #1448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/database/sql: sql comment tag injection experimental feature #1226

contrib/database/sql: sql comment tag injection experimental feature #1226

alexandre-normand commented Mar 29, 2022 •

edited

ajgajg1134 left a comment

alexandre-normand May 10, 2022

gbbr left a comment

gbbr left a comment

gbbr May 16, 2022

gbbr May 16, 2022 •

edited

alexandre-normand May 18, 2022

gbbr May 18, 2022

gbbr May 16, 2022

gbbr May 16, 2022

gbbr May 16, 2022

gbbr May 16, 2022

gbbr May 16, 2022

alexandre-normand May 18, 2022

gbbr May 16, 2022

gbbr May 16, 2022

alexandre-normand May 18, 2022

gbbr May 16, 2022

ajgajg1134 left a comment

gbbr May 31, 2022

alexandre-normand May 31, 2022

ajgajg1134 left a comment

alexandre-normand commented Jun 15, 2022

	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) {

	injectionOpts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags)
	opts := injectionOptionsForMode(tp.cfg.commentInjectionMode, discardDynamicTags)

	sqlCommentCarrier := tracer.SQLCommentCarrier{}
	var carrier tracer.SQLCommentCarrier

	commentedQuery, spanID := tc.withSQLCommentsInjected(ctx, query, true)
	cquery, spanID := tc.withSQLCommentsInjected(ctx, query, true)

	func injectionOptionsForMode(mode commentInjectionMode, discardDynamicTags bool) (opts []tracer.InjectionOption) {
	func injectionOptionsForMode(mode commentInjectionMode, discardDynamicTags bool) []tracer.InjectionOption {

		@@ -0,0 +1,55 @@
		// Unless explicitly stated otherwise all files in this repository are licensed

	expectedInjectedTags := make(map[string]string)
	tags := make(map[string]string)

	func WithCommentInjection() Option {
	func WithCommentInjection(mode CommentInjectionMode) Option {

contrib/database/sql: sql comment tag injection experimental feature #1226

contrib/database/sql: sql comment tag injection experimental feature #1226

Conversation

alexandre-normand commented Mar 29, 2022 • edited

Implementation notes

ajgajg1134 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr May 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

alexandre-normand commented Jun 15, 2022

alexandre-normand commented Mar 29, 2022 •

edited

gbbr May 16, 2022 •

edited