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

feat: Add a method to start a transaction #482

Merged
merged 4 commits into from Oct 25, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions http/sentryhttp.go
Expand Up @@ -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
Expand Down
32 changes: 32 additions & 0 deletions tracing.go
Expand Up @@ -5,6 +5,7 @@ import (
"crypto/rand"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
"net/http"
"regexp"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I like the idea of panicing here, we shouldn't cause possible runtime issues with our SDK methods.

Could we return err instead here? And have users handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I can return an error instead here. I'll be back to asking if I can panic in the HTTP handler though, and so, maybe you can shed some light on how other SDKs are dealing with this if that's a case that can happen?

Could we silently return the existing transaction with a bool to say it already was in progress instead? Could we just return nil and the user would have test for the transaction or get the existing one themselves?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah so there are the two options

  1. We return err, so the users have to grab the existing transaction themselves.
  2. We return the active transaction and don't create one. This does mean that the transaction won't have the context supplied in the constructor.

Option 2 is more "magic" behavior, but I think it's correct since two transactions running at once is basically useless to a person, so let us maybe do that.

Could we silently return the existing transaction with a bool to say it already was in progress instead

Sounds good to me, I think that's cleaner API

}
hub := GetHubFromContext(ctx)
if hub == nil {
hub = CurrentHub().Clone()
ctx = SetHubOnContext(ctx, hub)
}
options = append(options, TransactionName(name))
return StartSpan(
ctx,
"",
options...,
)
}
45 changes: 28 additions & 17 deletions tracing_test.go
Expand Up @@ -88,63 +88,58 @@ 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 {
t.Fatalf("sent %d events, want 1", got)
}
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,
}
Expand All @@ -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{
Expand Down