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

Update SamplingParameters #1749

Merged
merged 3 commits into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
This means that `"go.opentelemetry.io/otel/trace".ContextWithRemoteSpanContext` will now overwrite any existing current Span, not just existing remote Spans, and make it the current Span in a `context.Context`. (#1731)
- Information about a parent span context in a `"go.opentelemetry.io/otel/export/trace".SpanSnapshot` is unified in a new `Parent` field.
The existing `ParentSpanID` and `HasRemoteParent` fields are removed in favor of this. (#1748)
- The `ParentContext` field of the `"go.opentelemetry.io/otel/sdk/trace".SamplingParameters` is updated to hold a `context.Context` containing the parent span.
This changes it to make `SamplingParameters` conform with the OpenTelemetry specification. (#1749)

### Removed

Expand All @@ -29,6 +31,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
Because of this `"go.opentelemetry.io/otel/trace".RemoteSpanContextFromContext` is removed as it is no longer needed.
Instead, `"go.opentelemetry.io/otel/trace".SpanContextFromContex` can be used to return the current Span.
If needed, that Span's `SpanContext.IsRemote()` can then be used to determine if it is remote or not. (#1731)
- The `HasRemoteParent` field of the `"go.opentelemetry.io/otel/sdk/trace".SamplingParameters` is removed.
This field is redundant to the information returned from the `Remote` method of the `SpanContext` held in the `ParentContext` field. (#1749)

## [0.19.0] - 2021-03-18

Expand Down
32 changes: 17 additions & 15 deletions sdk/trace/sampling.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package trace // import "go.opentelemetry.io/otel/sdk/trace"

import (
"context"
"encoding/binary"
"fmt"

Expand All @@ -30,13 +31,12 @@ type Sampler interface {

// SamplingParameters contains the values passed to a Sampler.
type SamplingParameters struct {
ParentContext trace.SpanContext
TraceID trace.TraceID
Name string
HasRemoteParent bool
Kind trace.SpanKind
Attributes []attribute.KeyValue
Links []trace.Link
ParentContext context.Context
paivagustavo marked this conversation as resolved.
Show resolved Hide resolved
TraceID trace.TraceID
Name string
Kind trace.SpanKind
Attributes []attribute.KeyValue
Links []trace.Link
}

// SamplingDecision indicates whether a span is dropped, recorded and/or sampled.
Expand Down Expand Up @@ -69,16 +69,17 @@ type traceIDRatioSampler struct {
}

func (ts traceIDRatioSampler) ShouldSample(p SamplingParameters) SamplingResult {
psc := trace.SpanContextFromContext(p.ParentContext)
x := binary.BigEndian.Uint64(p.TraceID[0:8]) >> 1
if x < ts.traceIDUpperBound {
return SamplingResult{
Decision: RecordAndSample,
Tracestate: p.ParentContext.TraceState(),
Tracestate: psc.TraceState(),
}
}
return SamplingResult{
Decision: Drop,
Tracestate: p.ParentContext.TraceState(),
Tracestate: psc.TraceState(),
}
}

Expand Down Expand Up @@ -111,7 +112,7 @@ type alwaysOnSampler struct{}
func (as alwaysOnSampler) ShouldSample(p SamplingParameters) SamplingResult {
return SamplingResult{
Decision: RecordAndSample,
Tracestate: p.ParentContext.TraceState(),
Tracestate: trace.SpanContextFromContext(p.ParentContext).TraceState(),
}
}

Expand All @@ -132,7 +133,7 @@ type alwaysOffSampler struct{}
func (as alwaysOffSampler) ShouldSample(p SamplingParameters) SamplingResult {
return SamplingResult{
Decision: Drop,
Tracestate: p.ParentContext.TraceState(),
Tracestate: trace.SpanContextFromContext(p.ParentContext).TraceState(),
}
}

Expand Down Expand Up @@ -260,15 +261,16 @@ func (o localParentNotSampledOption) Apply(config *config) {
func (localParentNotSampledOption) private() {}

func (pb parentBased) ShouldSample(p SamplingParameters) SamplingResult {
if p.ParentContext.IsValid() {
if p.HasRemoteParent {
if p.ParentContext.IsSampled() {
psc := trace.SpanContextFromContext(p.ParentContext)
if psc.IsValid() {
if psc.IsRemote() {
if psc.IsSampled() {
return pb.config.remoteParentSampled.ShouldSample(p)
}
return pb.config.remoteParentNotSampled.ShouldSample(p)
}

if p.ParentContext.IsSampled() {
if psc.IsSampled() {
return pb.config.localParentSampled.ShouldSample(p)
}
return pb.config.localParentNotSampled.ShouldSample(p)
Expand Down
76 changes: 50 additions & 26 deletions sdk/trace/sampling_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"math/rand"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"go.opentelemetry.io/otel/attribute"
Expand All @@ -30,11 +31,14 @@ func TestParentBasedDefaultLocalParentSampled(t *testing.T) {
sampler := ParentBased(AlwaysSample())
traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7")
parentCtx := trace.NewSpanContext(trace.SpanContextConfig{
TraceID: traceID,
SpanID: spanID,
TraceFlags: trace.FlagsSampled,
})
parentCtx := trace.ContextWithSpanContext(
context.Background(),
trace.NewSpanContext(trace.SpanContextConfig{
TraceID: traceID,
SpanID: spanID,
TraceFlags: trace.FlagsSampled,
}),
)
if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != RecordAndSample {
t.Error("Sampling decision should be RecordAndSample")
}
Expand All @@ -44,10 +48,13 @@ func TestParentBasedDefaultLocalParentNotSampled(t *testing.T) {
sampler := ParentBased(AlwaysSample())
traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7")
parentCtx := trace.NewSpanContext(trace.SpanContextConfig{
TraceID: traceID,
SpanID: spanID,
})
parentCtx := trace.ContextWithSpanContext(
context.Background(),
trace.NewSpanContext(trace.SpanContextConfig{
TraceID: traceID,
SpanID: spanID,
}),
)
if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != Drop {
t.Error("Sampling decision should be Drop")
}
Expand Down Expand Up @@ -108,35 +115,48 @@ func TestParentBasedWithSamplerOptions(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7")
parentCtx := trace.NewSpanContext(trace.SpanContextConfig{
pscc := trace.SpanContextConfig{
TraceID: traceID,
SpanID: spanID,
})

Remote: tc.isParentRemote,
}
if tc.isParentSampled {
parentCtx = parentCtx.WithTraceFlags(trace.FlagsSampled)
pscc.TraceFlags = trace.FlagsSampled
}

params := SamplingParameters{ParentContext: parentCtx}
if tc.isParentRemote {
params.HasRemoteParent = true
params := SamplingParameters{
ParentContext: trace.ContextWithSpanContext(
context.Background(),
trace.NewSpanContext(pscc),
),
}

sampler := ParentBased(
nil,
tc.samplerOption,
)

var wantStr, gotStr string
switch tc.expectedDecision {
case RecordAndSample:
if sampler.ShouldSample(params).Decision != tc.expectedDecision {
t.Error("Sampling decision should be RecordAndSample")
}
wantStr = "RecordAndSample"
case Drop:
wantStr = "Drop"
default:
wantStr = "unknown"
}

actualDecision := sampler.ShouldSample(params).Decision
switch actualDecision {
case RecordAndSample:
gotStr = "RecordAndSample"
case Drop:
if sampler.ShouldSample(params).Decision != tc.expectedDecision {
t.Error("Sampling decision should be Drop")
}
gotStr = "Drop"
default:
gotStr = "unknown"
}

assert.Equalf(t, tc.expectedDecision, actualDecision, "want %s, got %s", wantStr, gotStr)
})
}
}
Expand Down Expand Up @@ -225,10 +245,14 @@ func TestTracestateIsPassed(t *testing.T) {
t.Error(err)
}

parentCtx := trace.NewSpanContext(trace.SpanContextConfig{
TraceState: traceState,
})
params := SamplingParameters{ParentContext: parentCtx}
params := SamplingParameters{
ParentContext: trace.ContextWithSpanContext(
context.Background(),
trace.NewSpanContext(trace.SpanContextConfig{
TraceState: traceState,
}),
),
}

require.Equal(t, traceState, tc.sampler.ShouldSample(params).Tracestate, "TraceState is not equal")
})
Expand Down
13 changes: 6 additions & 7 deletions sdk/trace/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -543,13 +543,12 @@ func startSpanInternal(ctx context.Context, tr *tracer, name string, o *trace.Sp
span.spanLimits = spanLimits

samplingResult := provider.sampler.ShouldSample(SamplingParameters{
ParentContext: psc,
TraceID: tid,
Name: name,
HasRemoteParent: psc.IsRemote(),
Kind: o.SpanKind,
Attributes: o.Attributes,
Links: o.Links,
ParentContext: ctx,
TraceID: tid,
Name: name,
Kind: o.SpanKind,
Attributes: o.Attributes,
Links: o.Links,
})

scc := trace.SpanContextConfig{
Expand Down
3 changes: 2 additions & 1 deletion sdk/trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1532,7 +1532,8 @@ func (s *stateSampler) ShouldSample(p SamplingParameters) SamplingResult {
if strings.HasPrefix(p.Name, s.prefix) {
decision = RecordAndSample
}
return SamplingResult{Decision: decision, Tracestate: s.f(p.ParentContext.TraceState())}
ts := s.f(trace.SpanContextFromContext(p.ParentContext).TraceState())
return SamplingResult{Decision: decision, Tracestate: ts}
}

func (s stateSampler) Description() string {
Expand Down
13 changes: 12 additions & 1 deletion trace/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,30 @@ func ContextWithSpan(parent context.Context, span Span) context.Context {
return context.WithValue(parent, currentSpanKey, span)
}

// ContextWithSpanContext returns a copy of parent with sc as the current
// Span. The Span implementation that wraps sc is non-recording and performs
// no operations other than to return sc as the SpanContext from the
// SpanContext method.
func ContextWithSpanContext(parent context.Context, sc SpanContext) context.Context {
paivagustavo marked this conversation as resolved.
Show resolved Hide resolved
return ContextWithSpan(parent, nonRecordingSpan{sc: sc})
}

// ContextWithRemoteSpanContext returns a copy of parent with rsc set explicly
// as a remote SpanContext and as the current Span. The Span implementation
// that wraps rsc is non-recording and performs no operations other than to
// return rsc as the SpanContext from the SpanContext method.
func ContextWithRemoteSpanContext(parent context.Context, rsc SpanContext) context.Context {
return ContextWithSpan(parent, nonRecordingSpan{sc: rsc.WithRemote(true)})
return ContextWithSpanContext(parent, rsc.WithRemote(true))
}

// SpanFromContext returns the current Span from ctx.
//
// If no Span is currently set in ctx an implementation of a Span that
// performs no operations is returned.
func SpanFromContext(ctx context.Context) Span {
if ctx == nil {
return noopSpan{}
}
if span, ok := ctx.Value(currentSpanKey).(Span); ok {
return span
}
Expand Down
5 changes: 5 additions & 0 deletions trace/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ func TestSpanFromContext(t *testing.T) {
}{
{
name: "empty context",
context: nil,
expectedSpan: emptySpan,
},
{
name: "background context",
context: context.Background(),
expectedSpan: emptySpan,
},
Expand Down