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 11 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
6 changes: 3 additions & 3 deletions model/model.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,11 @@ type DestinationSpanContext struct {
// DestinationServiceSpanContext holds contextual information about a
// destination service.
type DestinationServiceSpanContext struct {
// Type holds the destination service type.
// Type holds the destination service type. Deprecated.
Type string `json:"type,omitempty"`

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

// Resource identifies the destination service
// resource, e.g. a URI or message queue name.
Expand Down
1 change: 0 additions & 1 deletion module/apmawssdkgo/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ func send(req *request.Request) {
span.Action = req.Operation.Name

span.Context.SetDestinationService(apm.DestinationServiceSpanContext{
Name: spanSubtype,
Resource: svc.resource(),
})

Expand Down
52 changes: 49 additions & 3 deletions span.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"sync"
"time"

"go.elastic.co/apm/model"
"go.elastic.co/apm/stacktrace"
)

Expand Down Expand Up @@ -64,7 +65,7 @@ func (tx *Transaction) StartSpan(name, spanType string, parent *Span) *Span {
// span type, subtype, and action; a single dot separates span type and
// subtype, and the action will not be set.
func (tx *Transaction) StartSpanOptions(name, spanType string, opts SpanOptions) *Span {
if tx == nil {
if tx == nil || opts.parent.IsExitSpan() {
return newDroppedSpan()
}

Expand All @@ -80,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 @@ -101,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 @@ -143,7 +152,10 @@ func (tx *Transaction) StartSpanOptions(name, spanType string, opts SpanOptions)
// way will not have the "max spans" configuration applied, nor will they be
// considered in any transaction's span count.
func (t *Tracer) StartSpan(name, spanType string, transactionID SpanID, opts SpanOptions) *Span {
if opts.Parent.Trace.Validate() != nil || opts.Parent.Span.Validate() != nil || transactionID.Validate() != nil {
if opts.Parent.Trace.Validate() != nil ||
opts.Parent.Span.Validate() != nil ||
transactionID.Validate() != nil ||
opts.parent.IsExitSpan() {
return newDroppedSpan()
}
if !opts.Parent.Options.Recorded() {
Expand Down Expand Up @@ -179,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 @@ -239,6 +255,7 @@ type Span struct {
traceContext TraceContext
transactionID SpanID
parentID SpanID
exit bool

mu sync.RWMutex

Expand Down Expand Up @@ -384,6 +401,35 @@ func (s *Span) ended() bool {
return s.SpanData == nil
}

func (s *Span) setExitSpan(name, spanType string) {
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the spec, I believe the resource is meant to be set to subtype || type.

Subtype and Type can be set after starting a span, so I think we need to do that when ending the span. What we can do is keep track of whether SpanContext.SetDestinationService has been called (even with empty strings), and if it is never called for an exit span, then do this implicit call when ending the exit span.

resource := spanType
if resource == "" {
resource = name
}
s.exit = true
s.Context.SetDestinationService(DestinationServiceSpanContext{
Resource: resource,
})
}

// IsExitSpan returns true if the span is an exit span.
func (s *Span) IsExitSpan() bool {
if s == nil {
return false
}
s.mu.RLock()
defer s.mu.RUnlock()
if s.SpanData == nil {
return false
}
ctx := s.Context
return s.exit ||
ctx.destination != model.DestinationSpanContext{} ||
ctx.database != model.DatabaseSpanContext{} ||
ctx.message != model.MessageSpanContext{} ||
ctx.http != model.HTTPSpanContext{}
stuartnelson3 marked this conversation as resolved.
Show resolved Hide resolved
}

// SpanData holds the details for a span, and is embedded inside Span.
// When a span is ended or discarded, its SpanData field will be set
// to nil.
Expand Down
66 changes: 66 additions & 0 deletions span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,72 @@ func TestSpanType(t *testing.T) {
check(spans[3], "type", "subtype", "action.figure")
}

func TestStartExitSpan(t *testing.T) {
_, spans, _ := apmtest.WithTransaction(func(ctx context.Context) {
span, _ := apm.StartSpanOptions(ctx, "name", "type", apm.SpanOptions{ExitSpan: true})
assert.True(t, span.IsExitSpan())
span.End()
})
require.Len(t, spans, 1)
assert.Equal(t, spans[0].Context.Destination.Service.Resource, "type")

tracer := apmtest.NewRecordingTracer()
defer tracer.Close()

tx := tracer.StartTransaction("name", "type")
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)
assert.True(t, span2.Dropped())
span.End()
span2.End()

// when a span has the correct fields set to qualify as an exit span,
// it should be considered as such.
span = tx.StartSpan("name", "type", nil)
assert.False(t, span.IsExitSpan())
span.Context.SetDestinationService(apm.DestinationServiceSpanContext{
Resource: "my-resource",
})
assert.True(t, span.IsExitSpan())
span2 = tx.StartSpan("name", "type", span)
assert.True(t, span2.Dropped())
span.End()
span2.End()

span = tx.StartSpan("name", "type", nil)
assert.False(t, span.IsExitSpan())
span.Context.SetMessage(apm.MessageSpanContext{
QueueName: "my-queue",
})
assert.True(t, span.IsExitSpan())
span2 = tx.StartSpan("name", "type", span)
assert.True(t, span2.Dropped())
span.End()
span2.End()

span = tx.StartSpan("name", "type", nil)
assert.False(t, span.IsExitSpan())
span.Context.SetDatabase(apm.DatabaseSpanContext{
Instance: "my-instance",
})
assert.True(t, span.IsExitSpan())
span2 = tx.StartSpan("name", "type", span)
assert.True(t, span2.Dropped())
span.End()
span2.End()

span = tx.StartSpan("name", "type", nil)
assert.False(t, span.IsExitSpan())
span.Context.SetHTTPStatusCode(200)
assert.True(t, span.IsExitSpan())
span2 = tx.StartSpan("name", "type", span)
assert.True(t, span2.Dropped())
span.End()
span2.End()
}

func TestTracerStartSpanIDSpecified(t *testing.T) {
spanID := apm.SpanID{0, 1, 2, 3, 4, 5, 6, 7}
_, spans, _ := apmtest.WithTransaction(func(ctx context.Context) {
Expand Down
5 changes: 2 additions & 3 deletions spancontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ 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.
// for grouping and labeling in service maps. Deprecated.
Name string

// Resource holds an identifier for a destination service resource,
Expand Down Expand Up @@ -178,7 +178,6 @@ func (c *SpanContext) SetHTTPRequest(req *http.Request) {
}
}
c.SetDestinationService(DestinationServiceSpanContext{
Name: destinationServiceURL.String(),
Resource: destinationServiceResource,
})
}
Expand Down Expand Up @@ -222,7 +221,7 @@ func (c *SpanContext) SetMessage(message MessageSpanContext) {
// Both service.Name and service.Resource are required. If either is empty,
// then SetDestinationService is a no-op.
func (c *SpanContext) SetDestinationService(service DestinationServiceSpanContext) {
if service.Name == "" || service.Resource == "" {
if service.Resource == "" {
return
}
c.destinationService.Name = truncateString(service.Name)
stuartnelson3 marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
16 changes: 1 addition & 15 deletions spancontext_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,37 +65,31 @@ func TestSpanContextSetHTTPRequest(t *testing.T) {
url: "http://localhost/foo/bar",
addr: "localhost",
port: 80,
name: "http://localhost",
resource: "localhost:80",
}, {
url: "http://localhost:80/foo/bar",
addr: "localhost",
port: 80,
name: "http://localhost",
resource: "localhost:80",
}, {
url: "https://[::1]/foo/bar",
addr: "::1",
port: 443,
name: "https://[::1]",
resource: "[::1]:443",
}, {
url: "https://[::1]:8443/foo/bar",
addr: "::1",
port: 8443,
name: "https://[::1]:8443",
resource: "[::1]:8443",
}, {
url: "gopher://gopher.invalid:70",
addr: "gopher.invalid",
port: 70,
name: "gopher://gopher.invalid:70",
resource: "gopher.invalid:70",
}, {
url: "gopher://gopher.invalid",
addr: "gopher.invalid",
port: 0,
name: "gopher://gopher.invalid",
resource: "gopher.invalid",
}}

Expand All @@ -116,7 +110,6 @@ func TestSpanContextSetHTTPRequest(t *testing.T) {
Port: tc.port,
Service: &model.DestinationServiceSpanContext{
Type: spans[0].Type,
Name: tc.name,
Resource: tc.resource,
},
}, spans[0].Context.Destination)
Expand All @@ -132,29 +125,23 @@ func TestSetDestinationService(t *testing.T) {
}

testcases := []testcase{{
name: "",
resource: "",
expectEmpty: true,
}, {
name: "",
resource: "nonempty",
expectEmpty: true,
expectEmpty: false,
}, {
name: "nonempty",
resource: "",
expectEmpty: true,
}, {
name: "nonempty",
resource: "nonempty",
}}

for _, tc := range testcases {
t.Run(fmt.Sprintf("%s_%s", tc.name, tc.resource), func(t *testing.T) {
_, spans, _ := apmtest.WithTransaction(func(ctx context.Context) {
span, _ := apm.StartSpan(ctx, "name", "span_type")
span.Context.SetDestinationAddress("testing.invalid", 123)
span.Context.SetDestinationService(apm.DestinationServiceSpanContext{
Name: tc.name,
Resource: tc.resource,
})
span.End()
Expand All @@ -164,7 +151,6 @@ func TestSetDestinationService(t *testing.T) {
assert.Nil(t, spans[0].Context.Destination.Service)
} else {
assert.Equal(t, &model.DestinationServiceSpanContext{
Name: tc.name,
Resource: tc.resource,
Type: "span_type",
}, spans[0].Context.Destination.Service)
Expand Down