zipkinv2: add encoders, decoders, marshalers #3426
Conversation
Also update zipkin receiver to use them.
jrcamp
force-pushed
the
zipkinv2-encoders
branch
from
a286caf to
992993d
Compare
| return pdata.Traces{}, err | ||
| if hdr.Get("Content-Type") == "application/x-protobuf" { | ||
| // TODO: (@odeke-em) record the unique types of Content-Type uploads | ||
| if debugWasSet { |
There was a problem hiding this comment.
Can you please investigate the reason why this was added and maybe consider to remove it if no longer useful.
There was a problem hiding this comment.
Zipkin uses debug=true to indicate that sampling decisions is overriden but I'm not sure why it is recorded as a part of the span. I assume, it allows developers to query traces initiated for debugging purposes. It's a part of their span model.
There was a problem hiding this comment.
@bogdandrutu does this resolve it?
There was a problem hiding this comment.
That is for in process/on the wire (on the main request path) propagation as far as I know, not for collection. This is where it is unclear to me
|
Please resolve conflicts. |
|
Please resolve the conflicts. |
| type protobufDecoder struct { | ||
| // DebugWasSet toggles the Debug field of each Span. It is usually set to true if | ||
| // the "X-B3-Flags" header is set to 1 on the request. | ||
| DebugWasSet bool |
There was a problem hiding this comment.
Hah, very cryptic name but I agree that we should use what zipkin is using.
There was a problem hiding this comment.
Also why public in a private structure?
| return pdata.Traces{}, err | ||
| if hdr.Get("Content-Type") == "application/x-protobuf" { | ||
| // TODO: (@odeke-em) record the unique types of Content-Type uploads | ||
| if debugWasSet { |
There was a problem hiding this comment.
Zipkin uses debug=true to indicate that sampling decisions is overriden but I'm not sure why it is recorded as a part of the span. I assume, it allows developers to query traces initiated for debugging purposes. It's a part of their span model.
|
@bogdandrutu updated, lint failure looks unrelated |
Also update zipkin receiver to use them.