From d1042efe0c88c2e70b12ea503db98389447ec991 Mon Sep 17 00:00:00 2001 From: Pierre Massat Date: Wed, 19 Oct 2022 15:17:13 -0400 Subject: [PATCH 1/4] feat: Add a method to start a transaction --- http/sentryhttp.go | 12 ++++++++---- tracing.go | 32 ++++++++++++++++++++++++++++++++ tracing_test.go | 45 ++++++++++++++++++++++++++++----------------- 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/http/sentryhttp.go b/http/sentryhttp.go index a41b04a9d..1d426d6bd 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -86,15 +86,19 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { hub = sentry.CurrentHub().Clone() ctx = sentry.SetHubOnContext(ctx, hub) } - span := sentry.StartSpan(ctx, "http.server", - sentry.TransactionName(fmt.Sprintf("%s %s", r.Method, r.URL.Path)), + options := []sentry.SpanOption{ + sentry.OpName("http.server"), sentry.ContinueFromRequest(r), + } + transaction := sentry.StartTransaction(ctx, + fmt.Sprintf("%s %s", r.Method, r.URL.Path), + options..., ) - defer span.Finish() + defer transaction.Finish() // TODO(tracing): if the next handler.ServeHTTP panics, store // information on the transaction accordingly (status, tag, // level?, ...). - r = r.WithContext(span.Context()) + r = r.WithContext(transaction.Context()) hub.Scope().SetRequest(r) defer h.recoverWithSentry(hub, r) // TODO(tracing): use custom response writer to intercept diff --git a/tracing.go b/tracing.go index 6c992fa30..f123f3bc6 100644 --- a/tracing.go +++ b/tracing.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/hex" "encoding/json" + "errors" "fmt" "net/http" "regexp" @@ -567,6 +568,13 @@ func TransactionName(name string) SpanOption { } } +// OpName sets the operation name for a given span +func OpName(name string) SpanOption { + return func(s *Span) { + s.Op = name + } +} + // ContinueFromRequest returns a span option that updates the span to continue // an existing trace. If it cannot detect an existing trace in the request, the // span will be left unchanged. @@ -626,3 +634,27 @@ func spanFromContext(ctx context.Context) *Span { } return nil } + +// ErrTransactionAlreadyInProgress is returne when we try to start a transaction +// when another one is in progress +var ErrTransactionAlreadyInProgress = errors.New("transaction already in progress") + +// StartTransaction will create a transaction (root span) if there's no existing +// transaction in the context +func StartTransaction(ctx context.Context, name string, options ...SpanOption) *Span { + currentTransaction := ctx.Value(spanContextKey{}) + if currentTransaction != nil { + panic(ErrTransactionAlreadyInProgress) + } + hub := GetHubFromContext(ctx) + if hub == nil { + hub = CurrentHub().Clone() + ctx = SetHubOnContext(ctx, hub) + } + options = append(options, TransactionName(name)) + return StartSpan( + ctx, + "", + options..., + ) +} diff --git a/tracing_test.go b/tracing_test.go index 69819da72..5f875324a 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -88,40 +88,37 @@ func testMarshalJSONOmitEmptyParentSpanID(t *testing.T, v interface{}) { } } -func TestStartSpan(t *testing.T) { +func TestStartTransaction(t *testing.T) { transport := &TransportMock{} ctx := NewTestContext(ClientOptions{ Transport: transport, }) - op := "test.op" - transaction := "Test Transaction" + transactionName := "Test Transaction" description := "A Description" status := SpanStatusOK - parentSpanID := SpanIDFromHex("f00db33f") sampled := SampledTrue startTime := time.Now() endTime := startTime.Add(3 * time.Second) data := map[string]interface{}{ "k": "v", } - span := StartSpan(ctx, op, - TransactionName(transaction), + transaction := StartTransaction(ctx, + transactionName, func(s *Span) { s.Description = description s.Status = status - s.ParentSpanID = parentSpanID s.Sampled = sampled s.StartTime = startTime s.EndTime = endTime s.Data = data }, ) - span.Finish() + transaction.Finish() SpanCheck{ Sampled: sampled, RecorderLen: 1, - }.Check(t, span) + }.Check(t, transaction) events := transport.Events() if got := len(events); got != 1 { @@ -129,22 +126,20 @@ func TestStartSpan(t *testing.T) { } want := &Event{ Type: transactionType, - Transaction: transaction, + Transaction: transactionName, Contexts: map[string]Context{ "trace": TraceContext{ - TraceID: span.TraceID, - SpanID: span.SpanID, - ParentSpanID: parentSpanID, - Op: op, - Description: description, - Status: status, + TraceID: transaction.TraceID, + SpanID: transaction.SpanID, + Description: description, + Status: status, }.Map(), }, Tags: nil, // TODO(tracing): the root span / transaction data field is // mapped into Event.Extra for now, pending spec clarification. // https://github.com/getsentry/develop/issues/244#issuecomment-778694182 - Extra: span.Data, + Extra: transaction.Data, Timestamp: endTime, StartTime: startTime, } @@ -165,6 +160,22 @@ func TestStartSpan(t *testing.T) { } } +func TestStartTransactionAlreadyInProgress(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Errorf("StartTransaction should panic if a transaction is already in progress") + } + }() + transport := &TransportMock{} + ctx := NewTestContext(ClientOptions{ + Transport: transport, + }) + // Simulate a transaction already in progress + ctx = context.WithValue(ctx, spanContextKey{}, &Span{}) + transactionName := "Test Transaction" + _ = StartTransaction(ctx, transactionName) +} + func TestStartChild(t *testing.T) { transport := &TransportMock{} ctx := NewTestContext(ClientOptions{ From 9dcf4d75de8df9bd9292167dbce702d56eda810a Mon Sep 17 00:00:00 2001 From: Pierre Massat Date: Wed, 19 Oct 2022 20:46:31 -0400 Subject: [PATCH 2/4] Fix linter issues --- tracing.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tracing.go b/tracing.go index f123f3bc6..4c2d9e05a 100644 --- a/tracing.go +++ b/tracing.go @@ -568,7 +568,7 @@ func TransactionName(name string) SpanOption { } } -// OpName sets the operation name for a given span +// OpName sets the operation name for a given span. func OpName(name string) SpanOption { return func(s *Span) { s.Op = name @@ -636,11 +636,11 @@ func spanFromContext(ctx context.Context) *Span { } // ErrTransactionAlreadyInProgress is returne when we try to start a transaction -// when another one is in progress +// when another one is in progress. var ErrTransactionAlreadyInProgress = errors.New("transaction already in progress") // StartTransaction will create a transaction (root span) if there's no existing -// transaction in the context +// transaction in the context. func StartTransaction(ctx context.Context, name string, options ...SpanOption) *Span { currentTransaction := ctx.Value(spanContextKey{}) if currentTransaction != nil { From 68faf86219058cfa684e806b334c6507de53c4fc Mon Sep 17 00:00:00 2001 From: Pierre Massat Date: Fri, 21 Oct 2022 11:26:40 -0400 Subject: [PATCH 3/4] Return the existing transaction if there's already one running --- http/sentryhttp.go | 4 +- tracing.go | 14 ++--- tracing_test.go | 131 +++++++++++++++++++++++++++++++++++---------- 3 files changed, 111 insertions(+), 38 deletions(-) diff --git a/http/sentryhttp.go b/http/sentryhttp.go index 1d426d6bd..b2a7351cf 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -90,7 +90,9 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { sentry.OpName("http.server"), sentry.ContinueFromRequest(r), } - transaction := sentry.StartTransaction(ctx, + // We don't mind getting an existing transaction back so we don't need to + // check if it is. + transaction, _ := sentry.StartTransaction(ctx, fmt.Sprintf("%s %s", r.Method, r.URL.Path), options..., ) diff --git a/tracing.go b/tracing.go index 4c2d9e05a..4af7e5308 100644 --- a/tracing.go +++ b/tracing.go @@ -5,7 +5,6 @@ import ( "crypto/rand" "encoding/hex" "encoding/json" - "errors" "fmt" "net/http" "regexp" @@ -635,16 +634,13 @@ func spanFromContext(ctx context.Context) *Span { return nil } -// ErrTransactionAlreadyInProgress is returne when we try to start a transaction -// when another one is in progress. -var ErrTransactionAlreadyInProgress = errors.New("transaction already in progress") - // StartTransaction will create a transaction (root span) if there's no existing -// transaction in the context. -func StartTransaction(ctx context.Context, name string, options ...SpanOption) *Span { +// transaction in the context otherwise, it will return the existing transaction. +// The boolean indicates if it returned the existing transaction or not. +func StartTransaction(ctx context.Context, name string, options ...SpanOption) (*Span, bool) { currentTransaction := ctx.Value(spanContextKey{}) if currentTransaction != nil { - panic(ErrTransactionAlreadyInProgress) + return currentTransaction.(*Span), true } hub := GetHubFromContext(ctx) if hub == nil { @@ -656,5 +652,5 @@ func StartTransaction(ctx context.Context, name string, options ...SpanOption) * ctx, "", options..., - ) + ), false } diff --git a/tracing_test.go b/tracing_test.go index 5f875324a..f5fb6f4bd 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -88,37 +88,40 @@ func testMarshalJSONOmitEmptyParentSpanID(t *testing.T, v interface{}) { } } -func TestStartTransaction(t *testing.T) { +func TestStartSpan(t *testing.T) { transport := &TransportMock{} ctx := NewTestContext(ClientOptions{ Transport: transport, }) - transactionName := "Test Transaction" + op := "test.op" + transaction := "Test Transaction" description := "A Description" status := SpanStatusOK + parentSpanID := SpanIDFromHex("f00db33f") sampled := SampledTrue startTime := time.Now() endTime := startTime.Add(3 * time.Second) data := map[string]interface{}{ "k": "v", } - transaction := StartTransaction(ctx, - transactionName, + span := StartSpan(ctx, op, + TransactionName(transaction), func(s *Span) { s.Description = description s.Status = status + s.ParentSpanID = parentSpanID s.Sampled = sampled s.StartTime = startTime s.EndTime = endTime s.Data = data }, ) - transaction.Finish() + span.Finish() SpanCheck{ Sampled: sampled, RecorderLen: 1, - }.Check(t, transaction) + }.Check(t, span) events := transport.Events() if got := len(events); got != 1 { @@ -126,20 +129,22 @@ func TestStartTransaction(t *testing.T) { } want := &Event{ Type: transactionType, - Transaction: transactionName, + Transaction: transaction, Contexts: map[string]Context{ "trace": TraceContext{ - TraceID: transaction.TraceID, - SpanID: transaction.SpanID, - Description: description, - Status: status, + TraceID: span.TraceID, + SpanID: span.SpanID, + ParentSpanID: parentSpanID, + Op: op, + Description: description, + Status: status, }.Map(), }, Tags: nil, // TODO(tracing): the root span / transaction data field is // mapped into Event.Extra for now, pending spec clarification. // https://github.com/getsentry/develop/issues/244#issuecomment-778694182 - Extra: transaction.Data, + Extra: span.Data, Timestamp: endTime, StartTime: startTime, } @@ -160,22 +165,6 @@ func TestStartTransaction(t *testing.T) { } } -func TestStartTransactionAlreadyInProgress(t *testing.T) { - defer func() { - if r := recover(); r == nil { - t.Errorf("StartTransaction should panic if a transaction is already in progress") - } - }() - transport := &TransportMock{} - ctx := NewTestContext(ClientOptions{ - Transport: transport, - }) - // Simulate a transaction already in progress - ctx = context.WithValue(ctx, spanContextKey{}, &Span{}) - transactionName := "Test Transaction" - _ = StartTransaction(ctx, transactionName) -} - func TestStartChild(t *testing.T) { transport := &TransportMock{} ctx := NewTestContext(ClientOptions{ @@ -237,6 +226,92 @@ func TestStartChild(t *testing.T) { } } +func TestStartTransaction(t *testing.T) { + transport := &TransportMock{} + ctx := NewTestContext(ClientOptions{ + Transport: transport, + }) + transactionName := "Test Transaction" + description := "A Description" + status := SpanStatusOK + sampled := SampledTrue + startTime := time.Now() + endTime := startTime.Add(3 * time.Second) + data := map[string]interface{}{ + "k": "v", + } + transaction, _ := StartTransaction(ctx, + transactionName, + func(s *Span) { + s.Description = description + s.Status = status + s.Sampled = sampled + s.StartTime = startTime + s.EndTime = endTime + s.Data = data + }, + ) + transaction.Finish() + + SpanCheck{ + Sampled: sampled, + RecorderLen: 1, + }.Check(t, transaction) + + events := transport.Events() + if got := len(events); got != 1 { + t.Fatalf("sent %d events, want 1", got) + } + want := &Event{ + Type: transactionType, + Transaction: transactionName, + Contexts: map[string]Context{ + "trace": TraceContext{ + TraceID: transaction.TraceID, + SpanID: transaction.SpanID, + Description: description, + Status: status, + }.Map(), + }, + Tags: nil, + // TODO(tracing): the root span / transaction data field is + // mapped into Event.Extra for now, pending spec clarification. + // https://github.com/getsentry/develop/issues/244#issuecomment-778694182 + Extra: transaction.Data, + Timestamp: endTime, + StartTime: startTime, + } + opts := cmp.Options{ + cmpopts.IgnoreFields(Event{}, + "Contexts", "EventID", "Level", "Platform", + "Release", "Sdk", "ServerName", "Modules", + ), + cmpopts.EquateEmpty(), + } + if diff := cmp.Diff(want, events[0], opts); diff != "" { + t.Fatalf("Event mismatch (-want +got):\n%s", diff) + } + // Check trace context explicitly, as we ignored all contexts above to + // disregard other contexts. + if diff := cmp.Diff(want.Contexts["trace"], events[0].Contexts["trace"]); diff != "" { + t.Fatalf("TraceContext mismatch (-want +got):\n%s", diff) + } +} + +func TestStartTransactionAlreadyInProgress(t *testing.T) { + transport := &TransportMock{} + ctx := NewTestContext(ClientOptions{ + Transport: transport, + }) + // Simulate a transaction already in progress + ctx = context.WithValue(ctx, spanContextKey{}, &Span{}) + transactionName := "Test Transaction" + _, existing := StartTransaction(ctx, transactionName) + if !existing { + t.Fatal("StartTransaction should return true if a transaction is already in progress") + } +} + // testContextKey is used to store a value in a context so that we can check // that SDK operations on that context preserve the original context values. type testContextKey struct{} From 80272434a92c29337f0ba7c78c37b4f54a89e9dd Mon Sep 17 00:00:00 2001 From: Pierre Massat Date: Fri, 21 Oct 2022 11:42:42 -0400 Subject: [PATCH 4/4] Remove the boolean and go for maximum magic --- http/sentryhttp.go | 2 +- tracing.go | 11 +++++------ tracing_test.go | 16 +--------------- 3 files changed, 7 insertions(+), 22 deletions(-) diff --git a/http/sentryhttp.go b/http/sentryhttp.go index b2a7351cf..86c5df380 100644 --- a/http/sentryhttp.go +++ b/http/sentryhttp.go @@ -92,7 +92,7 @@ func (h *Handler) handle(handler http.Handler) http.HandlerFunc { } // We don't mind getting an existing transaction back so we don't need to // check if it is. - transaction, _ := sentry.StartTransaction(ctx, + transaction := sentry.StartTransaction(ctx, fmt.Sprintf("%s %s", r.Method, r.URL.Path), options..., ) diff --git a/tracing.go b/tracing.go index 4af7e5308..77666d4b5 100644 --- a/tracing.go +++ b/tracing.go @@ -636,11 +636,10 @@ func spanFromContext(ctx context.Context) *Span { // StartTransaction will create a transaction (root span) if there's no existing // transaction in the context otherwise, it will return the existing transaction. -// The boolean indicates if it returned the existing transaction or not. -func StartTransaction(ctx context.Context, name string, options ...SpanOption) (*Span, bool) { - currentTransaction := ctx.Value(spanContextKey{}) - if currentTransaction != nil { - return currentTransaction.(*Span), true +func StartTransaction(ctx context.Context, name string, options ...SpanOption) *Span { + currentTransaction, exists := ctx.Value(spanContextKey{}).(*Span) + if exists { + return currentTransaction } hub := GetHubFromContext(ctx) if hub == nil { @@ -652,5 +651,5 @@ func StartTransaction(ctx context.Context, name string, options ...SpanOption) ( ctx, "", options..., - ), false + ) } diff --git a/tracing_test.go b/tracing_test.go index f5fb6f4bd..347a6dded 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -240,7 +240,7 @@ func TestStartTransaction(t *testing.T) { data := map[string]interface{}{ "k": "v", } - transaction, _ := StartTransaction(ctx, + transaction := StartTransaction(ctx, transactionName, func(s *Span) { s.Description = description @@ -298,20 +298,6 @@ func TestStartTransaction(t *testing.T) { } } -func TestStartTransactionAlreadyInProgress(t *testing.T) { - transport := &TransportMock{} - ctx := NewTestContext(ClientOptions{ - Transport: transport, - }) - // Simulate a transaction already in progress - ctx = context.WithValue(ctx, spanContextKey{}, &Span{}) - transactionName := "Test Transaction" - _, existing := StartTransaction(ctx, transactionName) - if !existing { - t.Fatal("StartTransaction should return true if a transaction is already in progress") - } -} - // testContextKey is used to store a value in a context so that we can check // that SDK operations on that context preserve the original context values. type testContextKey struct{}