Skip to content

Commit

Permalink
Remove resampling on span.SetName (#1545)
Browse files Browse the repository at this point in the history
The spec makes it optional to attempt resampling when changing the name
of a span and we're not sure whether it can be done in an appropriate
manner, so it's best to not do it at all for now.  We can try again
later if we find a good way to do it.
  • Loading branch information
Aneurysm9 authored Feb 18, 2021
1 parent 8da5299 commit 1b5b662
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 65 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Added `resource.Default()` for use with meter and tracer providers. (#1507)
- Added `Keys()` method to `propagation.TextMapCarrier` and `propagation.HeaderCarrier` to adapt `http.Header` to this interface. (#1544)

### Removed

- Removed attempt to resample spans upon changing the span name with `span.SetName()`. (#1545)

## [0.17.0] - 2020-02-12

### Changed
Expand Down
28 changes: 0 additions & 28 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,35 +302,7 @@ func (s *span) SetName(name string) {

s.mu.Lock()
defer s.mu.Unlock()

s.name = name
// SAMPLING
noParent := !s.parent.SpanID.IsValid()
var ctx trace.SpanContext
if noParent {
ctx = trace.SpanContext{}
} else {
// FIXME: Where do we get the parent context from?
ctx = s.spanContext
}
data := samplingData{
noParent: noParent,
remoteParent: s.hasRemoteParent,
parent: ctx,
name: name,
cfg: s.tracer.provider.config.Load().(*Config),
span: s,
attributes: s.attributes.toKeyValue(),
links: s.interfaceArrayToLinksArray(),
kind: s.spanKind,
}
sampled := makeSamplingDecision(data)

// Adding attributes directly rather than using s.SetAttributes()
// as s.mu is already locked and attempting to do so would deadlock.
for _, a := range sampled.Attributes {
s.attributes.add(a)
}
}

// Name returns the name of this span.
Expand Down
63 changes: 26 additions & 37 deletions sdk/trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,58 +158,47 @@ func (ts testSampler) Description() string {
}

func TestSetName(t *testing.T) {
fooSampler := &testSampler{prefix: "foo", t: t}
tp := NewTracerProvider(WithConfig(Config{DefaultSampler: fooSampler}))
tp := NewTracerProvider()

type testCase struct {
name string
newName string
sampledBefore bool
sampledAfter bool
name string
newName string
}
for idx, tt := range []testCase{
{ // 0
name: "foobar",
newName: "foobaz",
sampledBefore: true,
sampledAfter: true,
name: "foobar",
newName: "foobaz",
},
{ // 1
name: "foobar",
newName: "barbaz",
sampledBefore: true,
sampledAfter: false,
name: "foobar",
newName: "barbaz",
},
{ // 2
name: "barbar",
newName: "barbaz",
sampledBefore: false,
sampledAfter: false,
name: "barbar",
newName: "barbaz",
},
{ // 3
name: "barbar",
newName: "foobar",
sampledBefore: false,
sampledAfter: true,
name: "barbar",
newName: "foobar",
},
} {
span := startNamedSpan(tp, "SetName", tt.name)
if fooSampler.callCount == 0 {
t.Errorf("%d: the sampler was not even called during span creation", idx)
}
fooSampler.callCount = 0
if gotSampledBefore := span.SpanContext().IsSampled(); tt.sampledBefore != gotSampledBefore {
t.Errorf("%d: invalid sampling decision before rename, expected %v, got %v", idx, tt.sampledBefore, gotSampledBefore)
}
span.SetName(tt.newName)
if fooSampler.callCount == 0 {
t.Errorf("%d: the sampler was not even called during span rename", idx)
sp := startNamedSpan(tp, "SetName", tt.name)
if sdkspan, ok := sp.(*span); ok {
if sdkspan.Name() != tt.name {
t.Errorf("%d: invalid name at span creation, expected %v, got %v", idx, tt.name, sdkspan.Name())
}
} else {
t.Errorf("%d: unable to coerce span to SDK span, is type %T", idx, sp)
}
fooSampler.callCount = 0
if gotSampledAfter := span.SpanContext().IsSampled(); tt.sampledAfter != gotSampledAfter {
t.Errorf("%d: invalid sampling decision after rename, expected %v, got %v", idx, tt.sampledAfter, gotSampledAfter)
sp.SetName(tt.newName)
if sdkspan, ok := sp.(*span); ok {
if sdkspan.Name() != tt.newName {
t.Errorf("%d: span name not changed, expected %v, got %v", idx, tt.newName, sdkspan.Name())
}
} else {
t.Errorf("%d: unable to coerce span to SDK span, is type %T", idx, sp)
}
span.End()
sp.End()
}
}

Expand Down

0 comments on commit 1b5b662

Please sign in to comment.