Fix sql comment span id being the parent span id instead of the SQL s…

…pan's.

contrib/database/sql/conn.go

@@ -208,6 +208,10 @@ func WithSpanTags(ctx context.Context, tags map[string]string) context.Context {
 // 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) (cquery string, injectedSpanID uint64) {
+	// The sql span only gets created after the call to the database because we need to be able to skip spans
+	// when a driver returns driver.ErrSkip. In order to work with those constraints, the parent span is used
+	// during SQL comment injection and a new span ID is generated for the sql span and used later when/if the span
+	// gets created.
 	var spanContext ddtrace.SpanContext
 	if span, ok := tracer.SpanFromContext(ctx); ok {
 		spanContext = span.Context()

ddtrace/ext/tags.go

@@ -46,12 +46,7 @@ const (
 
 	// Version is a tag that specifies the current application version.
 	Version = "version"
-
-	// ParentVersion is a tag that specifies the parent's application version
-	// (i.e. for database service spans, this would be the version of the running application rather than
-	// the database version).
-	ParentVersion = "parent.version"
-
+
 	// ResourceName defines the Resource name for the Span.
 	ResourceName = "resource.name"
 

ddtrace/tracer/sqlcomment.go

@@ -98,15 +98,18 @@ func (p *SQLCommentPropagator) injectWithCommentCarrier(spanCtx ddtrace.SpanCont
 	if p.mode == CommentInjectionDisabled {
 		return nil
 	}
+	spanID := random.Uint64()
+	carrier.AddSpanID(spanID)
+
 	if p.mode == StaticTagsSQLCommentInjection || p.mode == FullSQLCommentInjection {
 		ctx, ok := spanCtx.(*spanContext)
-		var env, pversion string
+		var env, version string
 		if ok {
 			if e, ok := ctx.meta(ext.Environment); ok {
 				env = e
 			}
-			if version, ok := ctx.meta(ext.ParentVersion); ok {
-				pversion = version
+			if v, ok := ctx.meta(ext.Version); ok {
+				version = v
 			}
 		}
 		if globalconfig.ServiceName() != "" {
@@ -115,14 +118,14 @@ func (p *SQLCommentPropagator) injectWithCommentCarrier(spanCtx ddtrace.SpanCont
 		if env != "" {
 			carrier.Set(ServiceEnvironmentSQLCommentKey, env)
 		}
-		if pversion != "" {
-			carrier.Set(ServiceVersionSQLCommentKey, pversion)
+		if version != "" {
+			carrier.Set(ServiceVersionSQLCommentKey, version)
 		}
 	}
 	if p.mode == FullSQLCommentInjection {
 		samplingPriority := 0
-		var traceID, spanID uint64
 
+		var traceID uint64
 		ctx, ok := spanCtx.(*spanContext)
 		if ok {
 			if sp, ok := ctx.samplingPriority(); ok {
@@ -131,13 +134,6 @@ func (p *SQLCommentPropagator) injectWithCommentCarrier(spanCtx ddtrace.SpanCont
 			if ctx.TraceID() > 0 {
 				traceID = ctx.TraceID()
 			}
-			if ctx.SpanID() > 0 {
-				spanID = ctx.SpanID()
-			}
-		}
-		if spanID == 0 {
-			spanID = random.Uint64()
-			carrier.AddSpanID(spanID)
 		}
 		if traceID == 0 {
 			traceID = spanID

ddtrace/tracer/sqlcomment_test.go

@@ -19,7 +19,7 @@ import (
 
 func TestSQLCommentPropagator(t *testing.T) {
 	prepareSpanContextWithSpanID := func(tracer *tracer) ddtrace.SpanContext {
-		root := tracer.StartSpan("db.call", WithSpanID(10), ServiceName("whiskey-db")).(*span)
+		root := tracer.StartSpan("service.calling.db", WithSpanID(10)).(*span)
 		root.SetTag(ext.SamplingPriority, 2)
 		return root.Context()
 	}
@@ -39,8 +39,8 @@ func TestSQLCommentPropagator(t *testing.T) {
 			mode:               FullSQLCommentInjection,
 			carrierOpts:        nil,
 			prepareSpanContext: prepareSpanContextWithSpanID,
-			expectedQuery:      "/*dde='test-env',ddsid='10',ddsn='whiskey-service',ddsp='2',ddsv='1.0.0',ddtid='10'*/ SELECT * from FOO",
-			expectedSpanIDGen:  false,
+			expectedQuery:      "/*dde='test-env',ddsid='<span_id>',ddsn='whiskey-service',ddsp='2',ddsv='1.0.0',ddtid='10'*/ SELECT * from FOO",
+			expectedSpanIDGen:  true,
 		},
 		{
 			name:        "no existing trace",
@@ -59,17 +59,17 @@ func TestSQLCommentPropagator(t *testing.T) {
 			mode:               FullSQLCommentInjection,
 			carrierOpts:        nil,
 			prepareSpanContext: prepareSpanContextWithSpanID,
-			expectedQuery:      "/*dde='test-env',ddsid='10',ddsn='whiskey-service',ddsp='2',ddsv='1.0.0',ddtid='10'*/",
-			expectedSpanIDGen:  false,
+			expectedQuery:      "/*dde='test-env',ddsid='<span_id>',ddsn='whiskey-service',ddsp='2',ddsv='1.0.0',ddtid='10'*/",
+			expectedSpanIDGen:  true,
 		},
 		{
 			name:               "query with existing comment",
 			query:              "SELECT * from FOO -- test query",
 			mode:               FullSQLCommentInjection,
 			carrierOpts:        nil,
 			prepareSpanContext: prepareSpanContextWithSpanID,
-			expectedQuery:      "/*dde='test-env',ddsid='10',ddsn='whiskey-service',ddsp='2',ddsv='1.0.0',ddtid='10'*/ SELECT * from FOO -- test query",
-			expectedSpanIDGen:  false,
+			expectedQuery:      "/*dde='test-env',ddsid='<span_id>',ddsn='whiskey-service',ddsp='2',ddsv='1.0.0',ddtid='10'*/ SELECT * from FOO -- test query",
+			expectedSpanIDGen:  true,
 		},
 		{
 			name:               "discard dynamic tags",
@@ -102,14 +102,10 @@ func TestSQLCommentPropagator(t *testing.T) {
 			require.NoError(t, err)
 
 			commented, spanID := carrier.CommentQuery(tc.query)
-			if tc.expectedSpanIDGen {
-				assert.Greater(t, spanID, uint64(0))
-				expected := strings.ReplaceAll(tc.expectedQuery, "<span_id>", strconv.FormatUint(spanID, 10))
-				assert.Equal(t, expected, commented)
-			} else {
-				assert.Equal(t, uint64(0), spanID)
-				assert.Equal(t, tc.expectedQuery, commented)
-			}
+
+			assert.Greater(t, spanID, uint64(0))
+			expected := strings.ReplaceAll(tc.expectedQuery, "<span_id>", strconv.FormatUint(spanID, 10))
+			assert.Equal(t, expected, commented)
 		})
 	}
 }

ddtrace/tracer/tracer.go

@@ -426,11 +426,6 @@ func (t *tracer) StartSpan(operationName string, options ...ddtrace.StartSpanOpt
 		if t.config.universalVersion || (!t.config.universalVersion && span.Service == t.config.serviceName) {
 			span.setMeta(ext.Version, t.config.version)
 		}
-		// For SQL spans which have a different database-inferred service name, set the service version as
-		// the parent version to avoid confusion with the database service's version
-		if span.Service != t.config.serviceName {
-			span.setMeta(ext.ParentVersion, t.config.version)
-		}
 	}
 	if t.config.env != "" {
 		span.setMeta(ext.Environment, t.config.env)