Add baggage to B3 codec #319
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
ghost
assigned 
| return Propagator{enableBaggage: true, baggagePrefix: "baggage-"} | ||
| } | ||
|
|
||
| // NewZipkinB3HTTPHeaderPropagator creates a Propagator for extracting and injecting |
There was a problem hiding this comment.
comment on exported function NewZipkinB3HTTPHeaderPropagatorWithBaggage should be of the form "NewZipkinB3HTTPHeaderPropagatorWithBaggage ..."
zipkin/propagation.go
Outdated
| @@ -66,6 +81,7 @@ func (p Propagator) Extract(abstractCarrier interface{}) (jaeger.SpanContext, er | |||
| var spanID uint64 | |||
| var parentID uint64 | |||
| sampled := false | |||
| var baggage map[string]string = nil | |||
There was a problem hiding this comment.
should drop = nil from declaration of var baggage; it is the zero value
zipkin/propagation.go
Outdated
| @@ -24,12 +24,21 @@ import ( | |||
| ) | |||
|
|
|||
| // Propagator is an Injector and Extractor | |||
| type Propagator struct{} | |||
| type Propagator struct{ | |||
zipkin/propagation_test.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| rootSampledHeader = opentracing.TextMapCarrier{ | ||
| "x-b3-traceid": "1", | ||
| "x-b3-spanid": "2", | ||
| "x-b3-sampled": "1", | ||
| "baggage-foo": "bar", |
Signed-off-by: Pavol Loffay <[email protected]>
|
The same question like in java imp jaegertracing/jaeger-client-java#438 (comment). Should it be enabled by default? should it contain enable flag? |
Codecov Report
@@ Coverage Diff @@
## master #319 +/- ##
==========================================
+ Coverage 87.31% 87.39% +0.07%
==========================================
Files 54 54
Lines 3011 3030 +19
==========================================
+ Hits 2629 2648 +19
Misses 271 271
Partials 111 111
Continue to review full report at Codecov.
|
Signed-off-by: Pavol Loffay <[email protected]>
zipkin/propagation.go
Outdated
|
|
||
| // 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 { |
There was a problem hiding this comment.
Can we use functional options here instead? We reduce the API surface area and it's backward and forward compatible.
There was a problem hiding this comment.
There are various styles how this can be done. I have changed it slightly in the last commit.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is there a resource on this? I remember someone talking about this but never read anything about it
There was a problem hiding this comment.
@yurishkuro do you it's better to pass option struct to the "constructor" function (or whatever it's called).
There was a problem hiding this comment.
I think for new code it might make sense to pass a struct with options.
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Related to jaegertracing/jaeger#755 (comment)