Stop creating root spans without existing spans (#13)

* Add AllowRoot option * Add tests * Update description about AllowRoot option * Update CHANGELOG * Add more tests for recordSpanError
XSAM · May 13, 2021 · 0987eb9 · 0987eb9
1 parent 8c94007
commit 0987eb9
Show file tree

Hide file tree

Showing 13 changed files with 601 additions and 331 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,9 +8,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Added
+
+- Add AllowRoot option to prevent backward incompatible. (#13)
+
 ### Changed
 
 - Upgrade to v0.20.0 of `go.opentelemetry.io/otel`. (#8)
+- otelsql will not create root spans in absence of existing spans by default. (#13)
 
 ## [0.2.1] - 2021-03-28
 

diff --git a/README.md b/README.md
@@ -22,9 +22,9 @@ $ go get github.com/XSAM/otelsql
 | Ping                       | If set to true, will enable the creation of spans on Ping requests.                                        | Implemented                    | Ping has context argument, but it might no needs to record.                                                                                                                                 |
 | RowsNext                   | If set to true, will enable the creation of events on corresponding calls. This can result in many events. | Implemented                    | It provides more visibility.                                                                                                                                                                |
 | DisableErrSkip             | If set to true, will suppress driver.ErrSkip errors in spans.                                              | Implemented                    | ErrSkip error might annoying                                                                                                                                                                |
+| AllowRoot                  | If set to true, will allow otelsql to create root spans in absence of existing spans or even context.        | Implemented                     | It might helpful while debugging missing operations. |
 | RowsAffected, LastInsertID | If set to true, will enable the creation of spans on RowsAffected/LastInsertId calls.                      | Dropped                     | Don't know its use cases. We might add this later based on the users' feedback.                                                                                                             |
 | QueryParams                | If set to true, will enable recording of parameters used with parametrized queries.                        | Dropped                     | It will cause high cardinality values and security problems.                                                                                                                                |
-| AllowRoot                  | If set to true, will allow ocsql to create root spans in absence of existing spans or even context.        | Dropped                     | I don't think traces data have meaning without context.                                                                                                                                     |
 
 ## Example
 

diff --git a/config.go b/config.go
@@ -63,6 +63,9 @@ type SpanOptions struct {
 
 	// DisableErrSkip, if set to true, will suppress driver.ErrSkip errors in spans.
 	DisableErrSkip bool
+
+	// AllowRoot, if set to true, will create root spans in absence of existing spans or even context.
+	AllowRoot bool
 }
 
 type defaultSpanNameFormatter struct{}

diff --git a/conn.go b/conn.go
@@ -53,7 +53,7 @@ func (c *otConn) Ping(ctx context.Context) (err error) {
 		return driver.ErrSkip
 	}
 
-	if c.cfg.SpanOptions.Ping {
+	if c.cfg.SpanOptions.Ping && (c.cfg.SpanOptions.AllowRoot || trace.SpanContextFromContext(ctx).IsValid()) {
 		var span trace.Span
 		ctx, span = c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnPing, ""),
 			trace.WithSpanKind(trace.SpanKindClient),
@@ -85,14 +85,17 @@ func (c *otConn) ExecContext(ctx context.Context, query string, args []driver.Na
 		return nil, driver.ErrSkip
 	}
 
-	ctx, span := c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnExec, query),
-		trace.WithSpanKind(trace.SpanKindClient),
-		trace.WithAttributes(
-			append(c.cfg.Attributes,
-				semconv.DBStatementKey.String(query),
-			)...),
-	)
-	defer span.End()
+	var span trace.Span
+	if c.cfg.SpanOptions.AllowRoot || trace.SpanContextFromContext(ctx).IsValid() {
+		ctx, span = c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnExec, query),
+			trace.WithSpanKind(trace.SpanKindClient),
+			trace.WithAttributes(
+				append(c.cfg.Attributes,
+					semconv.DBStatementKey.String(query),
+				)...),
+		)
+		defer span.End()
+	}
 
 	res, err = execer.ExecContext(ctx, query, args)
 	if err != nil {
@@ -116,14 +119,18 @@ func (c *otConn) QueryContext(ctx context.Context, query string, args []driver.N
 		return nil, driver.ErrSkip
 	}
 
-	queryCtx, span := c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnQuery, query),
-		trace.WithSpanKind(trace.SpanKindClient),
-		trace.WithAttributes(
-			append(c.cfg.Attributes,
-				semconv.DBStatementKey.String(query),
-			)...),
-	)
-	defer span.End()
+	var span trace.Span
+	queryCtx := ctx
+	if c.cfg.SpanOptions.AllowRoot || trace.SpanContextFromContext(ctx).IsValid() {
+		queryCtx, span = c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnQuery, query),
+			trace.WithSpanKind(trace.SpanKindClient),
+			trace.WithAttributes(
+				append(c.cfg.Attributes,
+					semconv.DBStatementKey.String(query),
+				)...),
+		)
+		defer span.End()
+	}
 
 	rows, err = queryer.QueryContext(queryCtx, query, args)
 	if err != nil {
@@ -139,14 +146,17 @@ func (c *otConn) PrepareContext(ctx context.Context, query string) (stmt driver.
 		return nil, driver.ErrSkip
 	}
 
-	ctx, span := c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnPrepare, query),
-		trace.WithSpanKind(trace.SpanKindClient),
-		trace.WithAttributes(
-			append(c.cfg.Attributes,
-				semconv.DBStatementKey.String(query),
-			)...),
-	)
-	defer span.End()
+	var span trace.Span
+	if c.cfg.SpanOptions.AllowRoot || trace.SpanContextFromContext(ctx).IsValid() {
+		ctx, span = c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnPrepare, query),
+			trace.WithSpanKind(trace.SpanKindClient),
+			trace.WithAttributes(
+				append(c.cfg.Attributes,
+					semconv.DBStatementKey.String(query),
+				)...),
+		)
+		defer span.End()
+	}
 
 	stmt, err = preparer.PrepareContext(ctx, query)
 	if err != nil {
@@ -162,11 +172,15 @@ func (c *otConn) BeginTx(ctx context.Context, opts driver.TxOptions) (tx driver.
 		return nil, driver.ErrSkip
 	}
 
-	beginTxCtx, span := c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnBeginTx, ""),
-		trace.WithSpanKind(trace.SpanKindClient),
-		trace.WithAttributes(c.cfg.Attributes...),
-	)
-	defer span.End()
+	var span trace.Span
+	beginTxCtx := ctx
+	if c.cfg.SpanOptions.AllowRoot || trace.SpanContextFromContext(ctx).IsValid() {
+		beginTxCtx, span = c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnBeginTx, ""),
+			trace.WithSpanKind(trace.SpanKindClient),
+			trace.WithAttributes(c.cfg.Attributes...),
+		)
+		defer span.End()
+	}
 
 	tx, err = connBeginTx.BeginTx(beginTxCtx, opts)
 	if err != nil {
@@ -182,11 +196,14 @@ func (c *otConn) ResetSession(ctx context.Context) (err error) {
 		return driver.ErrSkip
 	}
 
-	ctx, span := c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnResetSession, ""),
-		trace.WithSpanKind(trace.SpanKindClient),
-		trace.WithAttributes(c.cfg.Attributes...),
-	)
-	defer span.End()
+	var span trace.Span
+	if c.cfg.SpanOptions.AllowRoot || trace.SpanContextFromContext(ctx).IsValid() {
+		ctx, span = c.cfg.Tracer.Start(ctx, c.cfg.SpanNameFormatter.Format(ctx, MethodConnResetSession, ""),
+			trace.WithSpanKind(trace.SpanKindClient),
+			trace.WithAttributes(c.cfg.Attributes...),
+		)
+		defer span.End()
+	}
 
 	err = sessionResetter.ResetSession(ctx)
 	if err != nil {