Skip to content

Commit

Permalink
fix: Introduce a new Span.Name field (#605)
Browse files Browse the repository at this point in the history
  • Loading branch information
cleptric committed Mar 21, 2023
1 parent dc573d7 commit 85b380d
Show file tree
Hide file tree
Showing 9 changed files with 17 additions and 105 deletions.
19 changes: 3 additions & 16 deletions _examples/http/main.go
Expand Up @@ -70,24 +70,11 @@ func run() error {
TracesSampleRate: 1.0,
// ... or a TracesSampler
TracesSampler: sentry.TracesSampler(func(ctx sentry.SamplingContext) float64 {
// As an example, this custom sampler does not send some
// transactions to Sentry based on their name.
hub := sentry.GetHubFromContext(ctx.Span.Context())
name := hub.Scope().Transaction()
if name == "GET /favicon.ico" {
// Don't sample health checks.
if ctx.Span.Name == "GET /health" {
return 0.0
}
if strings.HasPrefix(name, "HEAD") {
return 0.0
}
// As an example, sample some transactions with a uniform rate.
if strings.HasPrefix(name, "POST") {
return 0.2
}
// Sample all other transactions for testing. On
// production, use TracesSampleRate with a rate adequate
// for your traffic, or use the SamplingContext to
// customize sampling per-transaction.

return 1.0
}),
})
Expand Down
4 changes: 2 additions & 2 deletions dynamic_sampling_context.go
Expand Up @@ -75,8 +75,8 @@ func DynamicSamplingContextFromTransaction(span *Span) DynamicSamplingContext {

// Only include the transaction name if it's of good quality (not empty and not SourceURL)
if span.Source != "" && span.Source != SourceURL {
if transactionName := scope.Transaction(); transactionName != "" {
entries["transaction"] = transactionName
if span.IsTransaction() {
entries["transaction"] = span.Name
}
}

Expand Down
5 changes: 0 additions & 5 deletions http/sentryhttp_test.go
Expand Up @@ -43,7 +43,6 @@ func TestIntegration(t *testing.T) {
"User-Agent": "Go-http-client/1.1",
},
},
Transaction: "GET /panic",
},
},
{
Expand Down Expand Up @@ -72,7 +71,6 @@ func TestIntegration(t *testing.T) {
"User-Agent": "Go-http-client/1.1",
},
},
Transaction: "POST /post",
},
},
{
Expand All @@ -93,7 +91,6 @@ func TestIntegration(t *testing.T) {
"User-Agent": "Go-http-client/1.1",
},
},
Transaction: "GET /get",
},
},
{
Expand Down Expand Up @@ -123,7 +120,6 @@ func TestIntegration(t *testing.T) {
"User-Agent": "Go-http-client/1.1",
},
},
Transaction: "POST /post/large",
},
},
{
Expand All @@ -149,7 +145,6 @@ func TestIntegration(t *testing.T) {
"User-Agent": "Go-http-client/1.1",
},
},
Transaction: "POST /post/body-ignored",
},
},
}
Expand Down
8 changes: 1 addition & 7 deletions otel/span_processor.go
Expand Up @@ -117,11 +117,6 @@ func getTraceParentContext(ctx context.Context) sentry.TraceParentContext {
}

func updateTransactionWithOtelData(transaction *sentry.Span, s otelSdkTrace.ReadOnlySpan) {
hub := sentry.GetHubFromContext(transaction.Context())
if hub == nil {
return
}

// TODO(michi) This is crazy inefficient
attributes := map[attribute.Key]string{}
resource := map[attribute.Key]string{}
Expand All @@ -141,10 +136,9 @@ func updateTransactionWithOtelData(transaction *sentry.Span, s otelSdkTrace.Read
spanAttributes := utils.ParseSpanAttributes(s)

transaction.Status = utils.MapOtelStatus(s)
transaction.Name = spanAttributes.Description
transaction.Op = spanAttributes.Op
transaction.Source = spanAttributes.Source
// TODO(michi) We might need to set this somewhere else than on the scope
hub.Scope().SetTransaction(spanAttributes.Description)
}

func updateSpanWithOtelData(span *sentry.Span, s otelSdkTrace.ReadOnlySpan) {
Expand Down
18 changes: 3 additions & 15 deletions otel/span_processor_test.go
Expand Up @@ -35,18 +35,6 @@ func setupSpanProcessorTest() (otelSdkTrace.SpanProcessor, *otelSdkTrace.TracerP
return spanProcessor, tp, tracer
}

func transactionName(span *sentry.Span) string {
hub := sentry.GetHubFromContext(span.Context())
if hub == nil {
log.Fatal("Cannot extract transaction name: Hub is nil")
}
scope := hub.Scope()
if scope == nil {
log.Fatal("Cannot extract transaction name: Hub is nil")
}
return scope.Transaction()
}

func emptyContextWithSentry() context.Context {
client, _ := sentry.NewClient(sentry.ClientOptions{
Dsn: "https://abc@example.com/123",
Expand Down Expand Up @@ -131,7 +119,7 @@ func TestOnStartRootSpan(t *testing.T) {
assertEqual(t, sentrySpan.ParentSpanID, sentry.SpanID{})
assertTrue(t, sentrySpan.IsTransaction())
assertEqual(t, sentrySpan.Sampled, sentry.SampledTrue)
assertEqual(t, transactionName(sentrySpan), "spanName")
assertEqual(t, sentrySpan.Name, "spanName")

testutils.AssertBaggageStringsEqual(
t,
Expand Down Expand Up @@ -185,7 +173,7 @@ func TestOnStartWithTraceParentContext(t *testing.T) {
assertEqual(t, sentrySpan.ParentSpanID, SpanIDFromHex("b72fa28504b07285"))
assertTrue(t, sentrySpan.IsTransaction())
assertEqual(t, sentrySpan.Sampled, sentry.SampledFalse)
assertEqual(t, transactionName(sentrySpan), "spanName")
assertEqual(t, sentrySpan.Name, "spanName")
assertEqual(t, sentrySpan.Status, sentry.SpanStatusUndefined)
assertEqual(t, sentrySpan.Source, sentry.SourceCustom)

Expand Down Expand Up @@ -230,7 +218,7 @@ func TestOnStartWithExistingParentSpan(t *testing.T) {
assertEqual(t, sentryChildSpan.SpanID.String(), otelChildSpan.SpanContext().SpanID().String())
assertEqual(t, sentryChildSpan.TraceID.String(), "bc6d53f15eb88f4320054569b8c553d4")
assertFalse(t, sentryChildSpan.IsTransaction())
assertEqual(t, transactionName(sentryChildSpan), "rootSpan")
assertEqual(t, sentryChildSpan.GetTransaction().Name, "rootSpan")
assertEqual(t, sentryChildSpan.Op, "childSpan")
}

Expand Down
22 changes: 0 additions & 22 deletions scope.go
Expand Up @@ -31,7 +31,6 @@ type Scope struct {
extra map[string]interface{}
fingerprint []string
level Level
transaction string
request *http.Request
// requestBody holds a reference to the original request.Body.
requestBody interface {
Expand Down Expand Up @@ -275,22 +274,6 @@ func (scope *Scope) SetLevel(level Level) {
scope.level = level
}

// SetTransaction sets the transaction name for the current transaction.
func (scope *Scope) SetTransaction(name string) {
scope.mu.Lock()
defer scope.mu.Unlock()

scope.transaction = name
}

// Transaction returns the transaction name for the current transaction.
func (scope *Scope) Transaction() (name string) {
scope.mu.RLock()
defer scope.mu.RUnlock()

return scope.transaction
}

// Clone returns a copy of the current scope with all data copied over.
func (scope *Scope) Clone() *Scope {
scope.mu.RLock()
Expand All @@ -312,7 +295,6 @@ func (scope *Scope) Clone() *Scope {
clone.fingerprint = make([]string, len(scope.fingerprint))
copy(clone.fingerprint, scope.fingerprint)
clone.level = scope.level
clone.transaction = scope.transaction
clone.request = scope.request
clone.requestBody = scope.requestBody
clone.eventProcessors = scope.eventProcessors
Expand Down Expand Up @@ -395,10 +377,6 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event {
event.Level = scope.level
}

if scope.transaction != "" {
event.Transaction = scope.transaction
}

if event.Request == nil && scope.request != nil {
event.Request = NewRequest(scope.request)
// NOTE: The SDK does not attempt to send partial request body data.
Expand Down
1 change: 0 additions & 1 deletion scope_concurrency_test.go
Expand Up @@ -52,7 +52,6 @@ func touchScope(scope *sentry.Scope, x int) {
scope.SetContext("foo", sentry.Context{"foo": "bar"})
scope.SetExtra("foo", "bar")
scope.SetLevel(sentry.LevelDebug)
scope.SetTransaction("foo")
scope.SetFingerprint([]string{"foo"})
scope.AddBreadcrumb(&sentry.Breadcrumb{Message: "foo"}, 100)
scope.SetUser(sentry.User{ID: "foo"})
Expand Down
30 changes: 0 additions & 30 deletions scope_test.go
Expand Up @@ -22,7 +22,6 @@ func fillScopeWithData(scope *Scope) *Scope {
scope.extra = map[string]interface{}{"scopeExtraKey": "scopeExtraValue"}
scope.fingerprint = []string{"scopeFingerprintOne", "scopeFingerprintTwo"}
scope.level = LevelDebug
scope.transaction = "wat"
scope.request = httptest.NewRequest("GET", "/wat", nil)
return scope
}
Expand Down Expand Up @@ -316,21 +315,6 @@ func TestScopeSetLevelOverrides(t *testing.T) {
assertEqual(t, scope.level, LevelFatal)
}

func TestScopeSetTransaction(t *testing.T) {
scope := NewScope()
scope.SetTransaction("abc")

assertEqual(t, scope.transaction, "abc")
}

func TestScopeSetTransactionOverrides(t *testing.T) {
scope := NewScope()
scope.SetTransaction("abc")
scope.SetTransaction("def")

assertEqual(t, scope.transaction, "def")
}

func TestAddBreadcrumbAddsBreadcrumb(t *testing.T) {
scope := NewScope()
scope.AddBreadcrumb(&Breadcrumb{Timestamp: testNow, Message: "test"}, maxBreadcrumbs)
Expand Down Expand Up @@ -395,7 +379,6 @@ func TestScopeParentChangedInheritance(t *testing.T) {
clone.SetContext("foo", Context{"foo": "bar"})
clone.SetExtra("foo", "bar")
clone.SetLevel(LevelDebug)
clone.SetTransaction("foo")
clone.SetFingerprint([]string{"foo"})
clone.AddBreadcrumb(&Breadcrumb{Timestamp: testNow, Message: "foo"}, maxBreadcrumbs)
clone.SetUser(User{ID: "foo"})
Expand All @@ -406,7 +389,6 @@ func TestScopeParentChangedInheritance(t *testing.T) {
scope.SetContext("foo", Context{"foo": "baz"})
scope.SetExtra("foo", "baz")
scope.SetLevel(LevelFatal)
scope.SetTransaction("bar")
scope.SetFingerprint([]string{"bar"})
scope.AddBreadcrumb(&Breadcrumb{Timestamp: testNow, Message: "bar"}, maxBreadcrumbs)
scope.SetUser(User{ID: "bar"})
Expand All @@ -417,7 +399,6 @@ func TestScopeParentChangedInheritance(t *testing.T) {
assertEqual(t, map[string]Context{"foo": {"foo": "bar"}}, clone.contexts)
assertEqual(t, map[string]interface{}{"foo": "bar"}, clone.extra)
assertEqual(t, LevelDebug, clone.level)
assertEqual(t, "foo", clone.transaction)
assertEqual(t, []string{"foo"}, clone.fingerprint)
assertEqual(t, []*Breadcrumb{{Timestamp: testNow, Message: "foo"}}, clone.breadcrumbs)
assertEqual(t, User{ID: "foo"}, clone.user)
Expand All @@ -427,7 +408,6 @@ func TestScopeParentChangedInheritance(t *testing.T) {
assertEqual(t, map[string]Context{"foo": {"foo": "baz"}}, scope.contexts)
assertEqual(t, map[string]interface{}{"foo": "baz"}, scope.extra)
assertEqual(t, LevelFatal, scope.level)
assertEqual(t, "bar", scope.transaction)
assertEqual(t, []string{"bar"}, scope.fingerprint)
assertEqual(t, []*Breadcrumb{{Timestamp: testNow, Message: "bar"}}, scope.breadcrumbs)
assertEqual(t, User{ID: "bar"}, scope.user)
Expand All @@ -441,7 +421,6 @@ func TestScopeChildOverrideInheritance(t *testing.T) {
scope.SetContext("foo", Context{"foo": "baz"})
scope.SetExtra("foo", "baz")
scope.SetLevel(LevelFatal)
scope.SetTransaction("bar")
scope.SetFingerprint([]string{"bar"})
scope.AddBreadcrumb(&Breadcrumb{Timestamp: testNow, Message: "bar"}, maxBreadcrumbs)
scope.SetUser(User{ID: "bar"})
Expand All @@ -456,7 +435,6 @@ func TestScopeChildOverrideInheritance(t *testing.T) {
clone.SetContext("foo", Context{"foo": "bar"})
clone.SetExtra("foo", "bar")
clone.SetLevel(LevelDebug)
clone.SetTransaction("foo")
clone.SetFingerprint([]string{"foo"})
clone.AddBreadcrumb(&Breadcrumb{Timestamp: testNow, Message: "foo"}, maxBreadcrumbs)
clone.SetUser(User{ID: "foo"})
Expand All @@ -470,7 +448,6 @@ func TestScopeChildOverrideInheritance(t *testing.T) {
assertEqual(t, map[string]Context{"foo": {"foo": "bar"}}, clone.contexts)
assertEqual(t, map[string]interface{}{"foo": "bar"}, clone.extra)
assertEqual(t, LevelDebug, clone.level)
assertEqual(t, "foo", clone.transaction)
assertEqual(t, []string{"foo"}, clone.fingerprint)
assertEqual(t, []*Breadcrumb{
{Timestamp: testNow, Message: "bar"},
Expand All @@ -483,7 +460,6 @@ func TestScopeChildOverrideInheritance(t *testing.T) {
assertEqual(t, map[string]Context{"foo": {"foo": "baz"}}, scope.contexts)
assertEqual(t, map[string]interface{}{"foo": "baz"}, scope.extra)
assertEqual(t, LevelFatal, scope.level)
assertEqual(t, "bar", scope.transaction)
assertEqual(t, []string{"bar"}, scope.fingerprint)
assertEqual(t, []*Breadcrumb{{Timestamp: testNow, Message: "bar"}}, scope.breadcrumbs)
assertEqual(t, User{ID: "bar"}, scope.user)
Expand All @@ -504,7 +480,6 @@ func TestClear(t *testing.T) {
assertEqual(t, map[string]interface{}{}, scope.extra)
assertEqual(t, []string{}, scope.fingerprint)
assertEqual(t, Level(""), scope.level)
assertEqual(t, "", scope.transaction)
assertEqual(t, (*http.Request)(nil), scope.request)
}

Expand All @@ -516,7 +491,6 @@ func TestClearAndReconfigure(t *testing.T) {
scope.SetContext("foo", Context{"foo": "bar"})
scope.SetExtra("foo", "bar")
scope.SetLevel(LevelDebug)
scope.SetTransaction("foo")
scope.SetFingerprint([]string{"foo"})
scope.AddBreadcrumb(&Breadcrumb{Timestamp: testNow, Message: "foo"}, maxBreadcrumbs)
scope.SetUser(User{ID: "foo"})
Expand All @@ -527,7 +501,6 @@ func TestClearAndReconfigure(t *testing.T) {
assertEqual(t, map[string]Context{"foo": {"foo": "bar"}}, scope.contexts)
assertEqual(t, map[string]interface{}{"foo": "bar"}, scope.extra)
assertEqual(t, LevelDebug, scope.level)
assertEqual(t, "foo", scope.transaction)
assertEqual(t, []string{"foo"}, scope.fingerprint)
assertEqual(t, []*Breadcrumb{{Timestamp: testNow, Message: "foo"}}, scope.breadcrumbs)
assertEqual(t, User{ID: "foo"}, scope.user)
Expand All @@ -553,7 +526,6 @@ func TestApplyToEventWithCorrectScopeAndEvent(t *testing.T) {
assertEqual(t, event.Contexts[sharedContextsKey], event.Contexts[sharedContextsKey], "should not override event context")
assertEqual(t, len(processedEvent.Extra), 2, "should merge extra")
assertEqual(t, processedEvent.Level, scope.level, "should use scope level if its set")
assertEqual(t, processedEvent.Transaction, scope.transaction, "should use scope transaction if its set")
assertNotEqual(t, processedEvent.User, scope.user, "should use event user if one exist")
assertNotEqual(t, processedEvent.Request, scope.request, "should use event request if one exist")
assertNotEqual(t, processedEvent.Fingerprint, scope.fingerprint, "should use event fingerprints if they exist")
Expand All @@ -572,7 +544,6 @@ func TestApplyToEventUsingEmptyScope(t *testing.T) {
assertNotEqual(t, processedEvent.User, scope.user, "should use event user")
assertNotEqual(t, processedEvent.Fingerprint, scope.fingerprint, "should use event fingerprint")
assertNotEqual(t, processedEvent.Level, scope.level, "should use event level")
assertNotEqual(t, processedEvent.Transaction, scope.transaction, "should use event transaction")
assertNotEqual(t, processedEvent.Request, scope.request, "should use event request")
}

Expand All @@ -589,7 +560,6 @@ func TestApplyToEventUsingEmptyEvent(t *testing.T) {
assertEqual(t, processedEvent.User, scope.user, "should use scope user")
assertEqual(t, processedEvent.Fingerprint, scope.fingerprint, "should use scope fingerprint")
assertEqual(t, processedEvent.Level, scope.level, "should use scope level")
assertEqual(t, processedEvent.Transaction, scope.transaction, "should use scope transaction")
assertEqual(t, processedEvent.Request, NewRequest(scope.request), "should use scope request")
}

Expand Down

0 comments on commit 85b380d

Please sign in to comment.