Skip to content
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

fix: mark spans representing calls to external services as exit spans #1317

Merged
merged 9 commits into from
Sep 23, 2022
10 changes: 10 additions & 0 deletions apmtest/withtransaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ func WithTransaction(f func(ctx context.Context)) (model.Transaction, []model.Sp
return WithTransactionOptions(apm.TransactionOptions{}, f)
}

// WithUncompressedTransaction is equivalent to calling WithTransactionOptions with compression disabled.
func WithUncompressedTransaction(f func(ctx context.Context)) (model.Transaction, []model.Span, []model.Error) {
tracer := NewRecordingTracer()
// Do not drop short exit spans by default.
tracer.SetExitSpanMinDuration(0)
tracer.SetSpanCompressionEnabled(false)
defer tracer.Close()
return tracer.WithTransactionOptions(apm.TransactionOptions{}, f)
}

// WithTransactionOptions calls f with a new context containing a transaction
// and transaction options, flushes the transaction to a test server, and returns
// the decoded transaction and any associated spans and errors.
Expand Down
2 changes: 1 addition & 1 deletion module/apmawssdkgo/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func build(req *request.Request) {

// The span name is added in the `send()` function, after other
// handlers have generated the necessary information on the request.
span := tx.StartSpan("", spanType, apm.SpanFromContext(ctx))
span := tx.StartExitSpan("", spanType, apm.SpanFromContext(ctx))
if span.Dropped() {
span.End()
return
Expand Down
6 changes: 4 additions & 2 deletions module/apmazure/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,11 @@ func (p *apmPipeline) Do(
r := req.Request.WithContext(ctx)
req.Request = r
defer tx.End()
span = tx.StartSpan(rpc.name(), rpc._type(), apm.SpanFromContext(ctx))
span = tx.StartExitSpan(rpc.name(), rpc._type(), apm.SpanFromContext(ctx))
} else {
span, ctx = apm.StartSpan(ctx, rpc.name(), rpc._type())
span, ctx = apm.StartSpanOptions(ctx, rpc.name(), rpc._type(), apm.SpanOptions{
ExitSpan: true,
})
}

defer span.End()
Expand Down
2 changes: 1 addition & 1 deletion module/apmelasticsearch/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {

propagateLegacyHeader := tx.ShouldPropagateLegacyHeader()
name := requestName(req)
span := tx.StartSpan(name, "db.elasticsearch", apm.SpanFromContext(ctx))
span := tx.StartExitSpan(name, "db.elasticsearch", apm.SpanFromContext(ctx))

if span.Dropped() {
span.End()
Expand Down
12 changes: 12 additions & 0 deletions module/apmgocql/apmgocql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,18 @@ func TestBatchObserver(t *testing.T) {
Instance: "quay ",
Statement: "INSERT INTO foo.bar(id) VALUES(1)",
},
Destination: &model.DestinationSpanContext{
Service: &model.DestinationServiceSpanContext{
Type: "db",
Resource: "cassandra",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "cassandra",
Name: "quay ",
},
},
}, spans[0].Context)

require.Len(t, errors, 1)
Expand Down
3 changes: 2 additions & 1 deletion module/apmgocql/observer.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ func (o *Observer) ObserveBatch(ctx context.Context, batch gocql.ObservedBatch)

for _, statement := range batch.Statements {
span, _ := apm.StartSpanOptions(ctx, querySignature(statement), "db.cassandra.query", apm.SpanOptions{
Start: batch.Start,
Start: batch.Start,
ExitSpan: true,
})
span.Duration = batchSpan.Duration
span.Context.SetDatabase(apm.DatabaseSpanContext{
Expand Down
4 changes: 3 additions & 1 deletion module/apmgopg/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ func (qh *queryHook) BeforeQuery(evt *pg.QueryEvent) {
sql = fmt.Sprintf("[go-pg] error: %s", err.Error())
}

span, _ := apm.StartSpan(evt.DB.Context(), apmsql.QuerySignature(sql), "db.postgresql.query")
span, _ := apm.StartSpanOptions(evt.DB.Context(), apmsql.QuerySignature(sql), "db.postgresql.query", apm.SpanOptions{
ExitSpan: true,
})
span.Context.SetDatabase(apm.DatabaseSpanContext{
Statement: sql,

Expand Down
4 changes: 3 additions & 1 deletion module/apmgopgv10/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ func (qh *queryHook) BeforeQuery(ctx context.Context, evt *pg.QueryEvent) (conte
return ctx, errors.Wrap(err, "failed to generate sql")
}

span, ctx := apm.StartSpan(ctx, apmsql.QuerySignature(string(sql)), "db.postgresql.query")
span, ctx := apm.StartSpanOptions(ctx, apmsql.QuerySignature(string(sql)), "db.postgresql.query", apm.SpanOptions{
ExitSpan: true,
})
span.Context.SetDatabase(apm.DatabaseSpanContext{
Statement: string(sql),

Expand Down
12 changes: 5 additions & 7 deletions module/apmgormv2/apmgorm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func TestWithContext(t *testing.T) {
}

func testWithContext(t *testing.T, dsnInfo apmsql.DSNInfo, dialect gorm.Dialector, config *gorm.Config) {
_, spans, errors := apmtest.WithTransaction(func(ctx context.Context) {
_, spans, errors := apmtest.WithUncompressedTransaction(func(ctx context.Context) {
db, err := gorm.Open(dialect, config)
require.NoError(t, err)
ddb, _ := db.DB()
Expand Down Expand Up @@ -121,12 +121,10 @@ func testWithContext(t *testing.T, dsnInfo apmsql.DSNInfo, dialect gorm.Dialecto
assert.NotEmpty(t, span.Context.Database.Statement)
assert.Equal(t, "sql", span.Context.Database.Type)
assert.Equal(t, dsnInfo.User, span.Context.Database.User)
if dsnInfo.Address == "" {
assert.Nil(t, span.Context.Destination)
} else {
assert.Equal(t, dsnInfo.Address, span.Context.Destination.Address)
assert.Equal(t, dsnInfo.Port, span.Context.Destination.Port)
}
assert.Equal(t, dsnInfo.Address, span.Context.Destination.Address)
assert.Equal(t, dsnInfo.Port, span.Context.Destination.Port)
assert.Equal(t, dsnInfo.User, span.Context.Destination.Service.Name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.Equal(t, dsnInfo.User, span.Context.Destination.Service.Name)
assert.Equal(t, dsnInfo.Database, span.Context.Destination.Service.Name)

Right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually destination service name, the old deprecated field. It should use the driver name.

assert.Equal(t, "db", span.Context.Destination.Service.Type)
}
assert.Equal(t, []string{
"INSERT INTO products",
Expand Down
2 changes: 1 addition & 1 deletion module/apmgrpc/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func startSpan(ctx context.Context, name string) (*apm.Span, context.Context) {
if !traceContext.Options.Recorded() {
return nil, outgoingContextWithTraceContext(ctx, traceContext, propagateLegacyHeader)
}
span := tx.StartSpan(name, "external.grpc", apm.SpanFromContext(ctx))
span := tx.StartExitSpan(name, "external.grpc", apm.SpanFromContext(ctx))
if !span.Dropped() {
traceContext = span.TraceContext()
ctx = apm.ContextWithSpan(ctx, span)
Expand Down
6 changes: 6 additions & 0 deletions module/apmgrpc/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ func testClientSpan(t *testing.T, traceparentHeaders ...string) {
Resource: tcpAddr.String(),
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "grpc",
Name: tcpAddr.String(),
},
},
}, clientSpans[0].Context)

serverTracer.Flush(nil)
Expand Down
10 changes: 9 additions & 1 deletion module/apmhttp/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,15 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
}

name := r.requestName(req)
span := tx.StartSpan(name, r.spanType, apm.SpanFromContext(ctx))
var span *apm.Span
if r.traceRequests {
// Trace requests are enabled, use this as the parent span and mark the trace requests as
// exit spans.
span = tx.StartSpan(name, r.spanType, apm.SpanFromContext(ctx))
} else {
// No trace requests, mark the span as exit span.
span = tx.StartExitSpan(name, r.spanType, apm.SpanFromContext(ctx))
}
var rt *requestTracer
if !span.Dropped() {
traceContext = span.TraceContext()
Expand Down
6 changes: 6 additions & 0 deletions module/apmhttp/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ func TestClient(t *testing.T) {
Resource: serverAddr.String(),
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "http",
Name: serverAddr.String(),
},
},
HTTP: &model.HTTPSpanContext{
// Note no user info included in server.URL.
URL: serverURL,
Expand Down
10 changes: 5 additions & 5 deletions module/apmhttp/clienttrace.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func withClientTrace(ctx context.Context, tx *apm.Transaction, parent *apm.Span)
r.mu.Lock()
defer r.mu.Unlock()
if !r.ended {
r.DNS = tx.StartSpan(fmt.Sprintf("DNS %s", i.Host), "external.http.dns", parent)
r.DNS = tx.StartExitSpan(fmt.Sprintf("DNS %s", i.Host), "external.http.dns", parent)
}
},

Expand All @@ -78,7 +78,7 @@ func withClientTrace(ctx context.Context, tx *apm.Transaction, parent *apm.Span)
defer r.mu.Unlock()
if !r.ended {
key := connectKey{network: network, addr: addr}
span := tx.StartSpan(fmt.Sprintf("Connect %s", addr), "external.http.connect", parent)
span := tx.StartExitSpan(fmt.Sprintf("Connect %s", addr), "external.http.connect", parent)
r.Connects[key] = span
}
},
Expand All @@ -100,15 +100,15 @@ func withClientTrace(ctx context.Context, tx *apm.Transaction, parent *apm.Span)
r.mu.Lock()
defer r.mu.Unlock()
if !r.ended {
r.Request = tx.StartSpan("Request", "external.http.request", parent)
r.Request = tx.StartExitSpan("Request", "external.http.request", parent)
}
},

TLSHandshakeStart: func() {
r.mu.Lock()
defer r.mu.Unlock()
if !r.ended {
r.TLS = tx.StartSpan("TLS", "external.http.tls", parent)
r.TLS = tx.StartExitSpan("TLS", "external.http.tls", parent)
}
},

Expand All @@ -131,7 +131,7 @@ func withClientTrace(ctx context.Context, tx *apm.Transaction, parent *apm.Span)
r.Request = nil
}
if !r.ended {
r.Response = tx.StartSpan("Response", "external.http.response", parent)
r.Response = tx.StartExitSpan("Response", "external.http.response", parent)
}
},
}), &r
Expand Down
4 changes: 3 additions & 1 deletion module/apmmongo/monitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ func (c *commandMonitor) started(ctx context.Context, event *event.CommandStarte
if collectionName, ok := collectionName(event.CommandName, event.Command); ok {
spanName = collectionName + "." + spanName
}
span, _ := apm.StartSpan(ctx, spanName, "db.mongodb.query")
span, _ := apm.StartSpanOptions(ctx, spanName, "db.mongodb.query", apm.SpanOptions{
ExitSpan: true,
})
if span.Dropped() {
return
}
Expand Down
12 changes: 12 additions & 0 deletions module/apmmongo/monitor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,18 @@ func testCommandMonitorFinished(t *testing.T, failure string) {
Type: "mongodb",
Statement: `{"find":"test_coll"}`,
},
Destination: &model.DestinationSpanContext{
Service: &model.DestinationServiceSpanContext{
Type: "db",
Resource: "mongodb",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "mongodb",
Name: "test_db",
},
},
}, spans[0].Context)
}

Expand Down
4 changes: 3 additions & 1 deletion module/apmredigo/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ func Do(ctx context.Context, conn redis.Conn, commandName string, args ...interf
if spanName == "" {
spanName = "(flush pipeline)"
}
span, _ := apm.StartSpan(ctx, spanName, "db.redis")
span, _ := apm.StartSpanOptions(ctx, spanName, "db.redis", apm.SpanOptions{
ExitSpan: true,
})
defer span.End()
return conn.Do(commandName, args...)
}
Expand Down
50 changes: 49 additions & 1 deletion module/apmsql/apmsql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func TestExecContext(t *testing.T) {

db.Ping() // connect
const N = 5
_, spans, _ := apmtest.WithTransaction(func(ctx context.Context) {
_, spans, _ := apmtest.WithUncompressedTransaction(func(ctx context.Context) {
_, err := db.ExecContext(ctx, "CREATE TABLE foo (bar INT)")
require.NoError(t, err)
for i := 0; i < N; i++ {
Expand Down Expand Up @@ -109,6 +109,18 @@ func TestExecContext(t *testing.T) {
Statement: "CREATE TABLE foo (bar INT)",
Type: "sql",
},
Destination: &model.DestinationSpanContext{
Service: &model.DestinationServiceSpanContext{
Type: "db",
Resource: "sqlite3",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "sqlite3",
Name: ":memory:",
},
},
}, spans[0].Context)

for i := 0; i < N; i++ {
Expand All @@ -121,6 +133,18 @@ func TestExecContext(t *testing.T) {
Statement: "INSERT INTO foo VALUES (?)",
Type: "sql",
},
Destination: &model.DestinationSpanContext{
Service: &model.DestinationServiceSpanContext{
Type: "db",
Resource: "sqlite3",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "sqlite3",
Name: ":memory:",
},
},
}, span.Context)
}

Expand All @@ -132,6 +156,18 @@ func TestExecContext(t *testing.T) {
Statement: "DELETE FROM foo",
Type: "sql",
},
Destination: &model.DestinationSpanContext{
Service: &model.DestinationServiceSpanContext{
Type: "db",
Resource: "sqlite3",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "sqlite3",
Name: ":memory:",
},
},
}, spans[N+1].Context)
}

Expand Down Expand Up @@ -162,6 +198,18 @@ func TestQueryContext(t *testing.T) {
Statement: "SELECT * FROM foo",
Type: "sql",
},
Destination: &model.DestinationSpanContext{
Service: &model.DestinationServiceSpanContext{
Type: "db",
Resource: "sqlite3",
},
},
Service: &model.ServiceSpanContext{
Target: &model.ServiceTargetSpanContext{
Type: "sqlite3",
Name: ":memory:",
},
},
}, spans[0].Context)
}

Expand Down
4 changes: 3 additions & 1 deletion module/apmsql/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ func (c *conn) startStmtSpan(ctx context.Context, stmt, spanType string) (*apm.S
}

func (c *conn) startSpan(ctx context.Context, name, spanType, stmt string) (*apm.Span, context.Context) {
span, ctx := apm.StartSpan(ctx, name, spanType)
span, ctx := apm.StartSpanOptions(ctx, name, spanType, apm.SpanOptions{
ExitSpan: true,
})
if !span.Dropped() {
if c.dsnInfo.Address != "" {
span.Context.SetDestinationAddress(c.dsnInfo.Address, c.dsnInfo.Port)
Expand Down
21 changes: 21 additions & 0 deletions span.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,27 @@ func (tx *Transaction) StartSpan(name, spanType string, parent *Span) *Span {
})
}

// StartExitSpan starts and returns a new Span within the transaction,
// with the specified name, type, and optional parent span, and
// with the start time set to the current time.
//
// StartExitSpan always returns a non-nil Span, with a non-nil SpanData
// field. Its End method must be called when the span completes.
//
// If the span type contains two dots, they are assumed to separate
// the span type, subtype, and action; a single dot separates span
// type and subtype, and the action will not be set.
//
// StartExitSpan is equivalent to calling StartSpanOptions with
// SpanOptions.Parent set to the trace context of parent if
// parent is non-nil and the span being marked as an exit span.
func (tx *Transaction) StartExitSpan(name, spanType string, parent *Span) *Span {
return tx.StartSpanOptions(name, spanType, SpanOptions{
parent: parent,
ExitSpan: true,
})
}

// StartSpanOptions starts and returns a new Span within the transaction,
// with the specified name, type, and options.
//
Expand Down