Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Add baggage to B3 codec #319

Merged
merged 9 commits into from
Mar 23, 2019
Merged

Add baggage to B3 codec #319

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
722c096
Add baggage to B3 codec
pavolloffay Jun 5, 2018
2560411
Fix fmt
pavolloffay Jun 5, 2018
8fe5a65
Use baggage manager
pavolloffay Jun 5, 2018
cc6ce0b
Use functional options
pavolloffay Jun 7, 2018
474fd99
Make baggage prefix public
pavolloffay Jun 7, 2018
9528efb
Use option type
pavolloffay Jun 7, 2018
5b0beda
Use baggage setter in test
pavolloffay Jun 7, 2018
45a743a
Merge branch 'master' into b3-baggage
yurishkuro Mar 23, 2019
d447887
Fix test
Mar 23, 2019
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
27 changes: 23 additions & 4 deletions zipkin/propagation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,21 @@ import (
)

// Propagator is an Injector and Extractor
type Propagator struct{}
type Propagator struct {
baggagePrefix string
}

// NewZipkinB3HTTPHeaderPropagator creates a Propagator for extracting and injecting
// Zipkin HTTP B3 headers into SpanContexts.
// Zipkin HTTP B3 headers into SpanContexts. Baggage is by default enabled and uses prefix
// 'baggage-'.
func NewZipkinB3HTTPHeaderPropagator() Propagator {
Copy link
Contributor

@black-adder black-adder Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can we use functional options here instead? We reduce the API surface area and it's backward and forward compatible.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 it's nice design

Copy link
Member Author

Choose a reason for hiding this comment

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

There are various styles how this can be done. I have changed it slightly in the last commit.

Copy link
Member

Choose a reason for hiding this comment

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

Go experts advise against using functional options and use option structs, because structs compose better (as evidenced by our duplication of tracer and config options).

The reason we traditionally used options is in cases where the existing API did not have configurability, in which case adding a vararg option list is backwards compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a resource on this? I remember someone talking about this but never read anything about it

Copy link
Member Author

Choose a reason for hiding this comment

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

@yurishkuro do you it's better to pass option struct to the "constructor" function (or whatever it's called).

Copy link
Member

Choose a reason for hiding this comment

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

I think for new code it might make sense to pass a struct with options.

return Propagator{}
return Propagator{baggagePrefix: "baggage-"}
}

// NewZipkinB3HTTPHeaderPropagatorWithBaggage creates a Propagator for extracting and injecting
// Zipkin HTTP B3 headers into SpanContexts with baggage configuration
func NewZipkinB3HTTPHeaderPropagatorWithBaggage(baggagePrefix string) Propagator {
return Propagator{baggagePrefix: baggagePrefix}
}

// Inject conforms to the Injector interface for decoding Zipkin HTTP B3 headers
Expand All @@ -53,6 +62,10 @@ func (p Propagator) Inject(
} else {
textMapWriter.Set("x-b3-sampled", "0")
}
sc.ForeachBaggageItem(func(k, v string) bool {
textMapWriter.Set(p.baggagePrefix+k, v)
return true
})
return nil
}

Expand All @@ -66,6 +79,7 @@ func (p Propagator) Extract(abstractCarrier interface{}) (jaeger.SpanContext, er
var spanID uint64
var parentID uint64
sampled := false
var baggage map[string]string
err := textMapReader.ForeachKey(func(rawKey, value string) error {
key := strings.ToLower(rawKey) // TODO not necessary for plain TextMap
var err error
Expand All @@ -77,6 +91,11 @@ func (p Propagator) Extract(abstractCarrier interface{}) (jaeger.SpanContext, er
spanID, err = strconv.ParseUint(value, 16, 64)
} else if key == "x-b3-sampled" && value == "1" {
sampled = true
} else if strings.HasPrefix(key, p.baggagePrefix) {
if baggage == nil {
baggage = make(map[string]string)
}
baggage[key[len(p.baggagePrefix):]] = value
}
return err
})
Expand All @@ -91,5 +110,5 @@ func (p Propagator) Extract(abstractCarrier interface{}) (jaeger.SpanContext, er
jaeger.TraceID{Low: traceID},
jaeger.SpanID(spanID),
jaeger.SpanID(parentID),
sampled, nil), nil
sampled, baggage), nil
}
25 changes: 20 additions & 5 deletions zipkin/propagation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,17 @@ import (
)

var (
rootSampled = newSpanContext(1, 2, 0, true)
nonRootSampled = newSpanContext(1, 2, 1, true)
nonRootNonSampled = newSpanContext(1, 2, 1, false)
rootSampled = newSpanContext(1, 2, 0, true, map[string]string{"foo": "bar"})
nonRootSampled = newSpanContext(1, 2, 1, true, nil)
nonRootNonSampled = newSpanContext(1, 2, 1, false, nil)
)

var (
rootSampledHeader = opentracing.TextMapCarrier{
"x-b3-traceid": "1",
"x-b3-spanid": "2",
"x-b3-sampled": "1",
"baggage-foo": "bar",
}
nonRootSampledHeader = opentracing.TextMapCarrier{
"x-b3-traceid": "1",
Expand All @@ -58,13 +59,13 @@ var (
propagator = NewZipkinB3HTTPHeaderPropagator()
)

func newSpanContext(traceID, spanID, parentID uint64, sampled bool) jaeger.SpanContext {
func newSpanContext(traceID, spanID, parentID uint64, sampled bool, baggage map[string]string) jaeger.SpanContext {
return jaeger.NewSpanContext(
jaeger.TraceID{Low: traceID},
jaeger.SpanID(spanID),
jaeger.SpanID(parentID),
sampled,
nil,
baggage,
)
}

Expand Down Expand Up @@ -111,3 +112,17 @@ func TestInjectorNonRootNonSampled(t *testing.T) {
assert.Nil(t, err)
assert.EqualValues(t, nonRootNonSampledHeader, hdr)
}

func TestCustomBaggagePrefix(t *testing.T) {
propag := NewZipkinB3HTTPHeaderPropagatorWithBaggage("emoji:)")
hdr := opentracing.TextMapCarrier{}
sc := newSpanContext(0, 0, 0, false, map[string]string{"foo": "bar"})
err := propag.Inject(sc, hdr)
assert.Nil(t, err)
assert.EqualValues(t, map[string]string{
"x-b3-traceid": "0",
"x-b3-spanid": "0",
"x-b3-sampled": "0",
"emoji:)foo": "bar",
}, hdr)
}