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

Infer destination service resource adapt public api #1003

Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions gocontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,6 @@ func StartSpan(ctx context.Context, name, spanType string) (*Span, context.Conte
return StartSpanOptions(ctx, name, spanType, SpanOptions{})
}

// StartExitSpan starts an exit span. If the provided context already contains
// an exit span, this function returns a dropped span.
func StartExitSpan(ctx context.Context, name, spanType string) (*Span, context.Context) {
span, ctx := StartSpanOptions(ctx, name, spanType, SpanOptions{})
if span.Dropped() {
return span, ctx
}
span.setExitSpan(name, spanType)
return span, ctx
}

// StartSpanOptions starts and returns a new Span within the sampled transaction
// and parent span in the context, if any. If the span isn't dropped, it will be
// stored in the resulting context.
Expand Down
2 changes: 1 addition & 1 deletion model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ type DestinationServiceSpanContext struct {
Type string `json:"type,omitempty"`

// Name holds the destination service name. Deprecated.
Name string `json:"name,omitempty"`
Name string `json:"name"`

// Resource identifies the destination service
// resource, e.g. a URI or message queue name.
Expand Down
35 changes: 20 additions & 15 deletions span.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,6 @@ func (tx *Transaction) StartSpan(name, spanType string, parent *Span) *Span {
})
}

// StartExitSpan starts an exit span. If the provided parent is already an exit
// span, this function returns a dropped span.
func (tx *Transaction) StartExitSpan(name, spanType string, parent *Span) *Span {
span := tx.StartSpan(name, spanType, parent)
if span.Dropped() {
return span
}
span.setExitSpan(name, spanType)
return span
}

// StartSpanOptions starts and returns a new Span within the transaction,
// with the specified name, type, and options.
//
Expand All @@ -77,7 +66,6 @@ func (tx *Transaction) StartExitSpan(name, spanType string, parent *Span) *Span
// subtype, and the action will not be set.
func (tx *Transaction) StartSpanOptions(name, spanType string, opts SpanOptions) *Span {
if tx == nil || opts.parent.IsExitSpan() {
// TODO: Is this the equivalent of a noop span?
return newDroppedSpan()
}

Expand All @@ -93,15 +81,20 @@ func (tx *Transaction) StartSpanOptions(name, spanType string, opts SpanOptions)
// Lock the parent first to avoid deadlocks in breakdown metrics calculation.
if opts.parent != nil {
opts.parent.mu.Lock()
defer opts.parent.mu.Unlock()
}

// Prevent tx from being ended while we're starting a span.
tx.mu.RLock()
defer tx.mu.RUnlock()
if tx.ended() {
if opts.parent != nil {
opts.parent.mu.Unlock()
}
Copy link
Member

@axw axw Aug 12, 2021

Choose a reason for hiding this comment

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

Moving the unlock away from the lock scares me a bit. TBH I'm really not a fan of this implicit exit span status, so I've raised the question with agent devs whether it is necessary to implement that part of the spec.

EDIT: point being that if we don't need to do the implicit check, and we only have explicitly identified exit spans, then "s.exit" is immutable from span start and requires no locking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 makes sense. I'm not a fan of this locking either, so let's see what happens.

return tx.tracer.StartSpan(name, spanType, transactionID, opts)
}
if opts.parent != nil {
defer opts.parent.mu.Unlock()
}

// Calculate the span time relative to the transaction timestamp so
// that wall-clock adjustments occurring after the transaction start
Expand All @@ -114,6 +107,9 @@ func (tx *Transaction) StartSpanOptions(name, spanType string, opts SpanOptions)
span := tx.tracer.startSpan(name, spanType, transactionID, opts)
span.tx = tx
span.parent = opts.parent
if opts.ExitSpan {
span.setExitSpan(name, spanType)
}

// Guard access to spansCreated, spansDropped, rand, and childrenTimer.
tx.TransactionData.mu.Lock()
Expand Down Expand Up @@ -195,6 +191,10 @@ type SpanOptions struct {
// will be generated and used instead.
SpanID SpanID

// Indicates whether a span is an exit span or not. All child spans
// will be noop spans.
ExitSpan bool

// parent, if non-nil, holds the parent span.
//
// This is only used if Parent is zero, and is only available to internal
Expand Down Expand Up @@ -255,6 +255,7 @@ type Span struct {
traceContext TraceContext
transactionID SpanID
parentID SpanID
exit bool

mu sync.RWMutex

Expand Down Expand Up @@ -413,7 +414,12 @@ func (s *Span) setExitSpan(name, spanType string) {

// IsExitSpan returns true if the span is an exit span.
func (s *Span) IsExitSpan() bool {
if s == nil || s.SpanData == nil {
if s == nil {
return false
}
s.mu.RLock()
defer s.mu.RUnlock()
if s.SpanData == nil {
return false
}
ctx := s.Context
Expand All @@ -432,7 +438,6 @@ type SpanData struct {
stackTraceLimit int
timestamp time.Time
childrenTimer childrenTimer
exit bool

// Name holds the span name, initialized with the value passed to StartSpan.
Name string
Expand Down
4 changes: 2 additions & 2 deletions span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func TestSpanType(t *testing.T) {

func TestStartExitSpan(t *testing.T) {
_, spans, _ := apmtest.WithTransaction(func(ctx context.Context) {
span, _ := apm.StartExitSpan(ctx, "name", "type")
span, _ := apm.StartSpanOptions(ctx, "name", "type", apm.SpanOptions{ExitSpan: true})
assert.True(t, span.IsExitSpan())
span.End()
})
Expand All @@ -176,7 +176,7 @@ func TestStartExitSpan(t *testing.T) {
defer tracer.Close()

tx := tracer.StartTransaction("name", "type")
span := tx.StartExitSpan("name", "type", nil)
span := tx.StartSpanOptions("name", "type", apm.SpanOptions{ExitSpan: true})
assert.True(t, span.IsExitSpan())
// when the parent span is an exit span, any children should be noops.
span2 := tx.StartSpan("name", "type", span)
Expand Down
5 changes: 5 additions & 0 deletions spancontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ type DatabaseSpanContext struct {

// DestinationServiceSpanContext holds destination service span span.
type DestinationServiceSpanContext struct {
// Name holds a name for the destination service, which may be used
// for grouping and labeling in service maps. Deprecated.
Name string

// Resource holds an identifier for a destination service resource,
// such as a message queue.
Resource string
Expand Down Expand Up @@ -220,6 +224,7 @@ func (c *SpanContext) SetDestinationService(service DestinationServiceSpanContex
if service.Resource == "" {
return
}
c.destinationService.Name = truncateString(service.Name)
c.destinationService.Resource = truncateString(service.Resource)
c.destination.Service = &c.destinationService
c.model.Destination = &c.destination
Expand Down
3 changes: 2 additions & 1 deletion validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,12 @@ func TestValidateDatabaseSpanContext(t *testing.T) {

func TestValidateDestinationSpanContext(t *testing.T) {
overlong := strings.Repeat("x", 1025)
for range []string{"", overlong} {
for _, name := range []string{"", overlong} {
for _, resource := range []string{"", overlong} {
validateSpan(t, func(s *apm.Span) {
s.Context.SetDestinationAddress(overlong, 0)
s.Context.SetDestinationService(apm.DestinationServiceSpanContext{
Name: name,
Resource: resource,
})
})
Expand Down