census-instrumentation / opencensus-java

A stats collection and distributed tracing framework
https://opencensus.io
Apache License 2.0
673 stars 202 forks source link

implement "b3 single" header format #1400

Open codefromthecrypt opened 6 years ago

codefromthecrypt commented 6 years ago

As discussed on https://github.com/openzipkin/b3-propagation/issues/21 and first implemented here: https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SingleFormat.java https://github.com/openzipkin/brave/blob/master/brave/src/test/java/brave/propagation/B3SingleFormatTest.java

Let's support at least reading "b3" header from a single string, most commonly traceid-spanid-1 It would also be nice to support optionally writing this, especially in message providers or others with constrained environments.

Brave currently has a property like this, but its name could change with feedback:

    /**
     * When true, only writes a single {@link B3SingleFormat b3 header} for outbound propagation.
     *
     * <p>Use this to reduce overhead. Note: normal {@link Tracing#propagation()} is used to parse
     * incoming headers. The implementation must be able to read "b3" headers.
     */
    public Builder b3SingleFormat(boolean b3SingleFormat) {
      this.b3SingleFormat = b3SingleFormat;
      return this;
}
l0s commented 5 years ago

Is the intention to introduce a new io.opencensus.trace.propagation.TextFormat implementation that extracts the b3 header (or tracestate header with "b3" prefix) and injects the b3 header?

codefromthecrypt commented 5 years ago

the b3 single header was indeed to be designed as both a top-level header and also a tracestate entry for when it is necessary to carry data present in b3, but no longer in the traceparent spec (such as before-the-fact sampling instructions b3=0, or the parent id)

l0s commented 5 years ago

@adriancole I provided two potential implementations. #1989 introduces a new TextFormat implementation and #1990 modifies the existing B3Format. The former extracts b3 from the single format as well as injects it in the same format. The latter extracts b3 from either the single or multi formats but only injects data in the multi format. Can you comment on which one most closely matches the intention of this issue?

codefromthecrypt commented 5 years ago

@IOs you are awesome btw. thank you for the help

codefromthecrypt commented 5 years ago

not sure if github is case-sensitive @l0s ^^

codefromthecrypt commented 5 years ago

I think in general there will be some tension about tracestate and b3. I'm not sure the responsibility for parsing tracestate should be in the b3 class. My original thought on how tracestate works is that handlers are registered per state name. This might be more flexible than having b3 responsible for parsing tracestate. That said priority will still be of issue. The more flexible system will just fallback to tracestate (which has b3 registered) upon a higher level parse fail.

l0s commented 5 years ago

@adriancole that makes sense. I can remove the tracestate handling from the b3 TextFormat. A traceparent / tracestate format can delegate to other formats as necessary. Should I add more robust traceparent / tracestate handling as part of this issue or a separate one?

Also, did you have thoughts on which approach is more appropriate, a new TextFormat for "b3 single" or a modification to the existing B3Format?

codefromthecrypt commented 5 years ago

what we do in brave is read b3 single, but only write when people explicitly choose it.

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3Propagation.java#L131 https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SinglePropagation.java

l0s commented 5 years ago

Ok great. I'll go with the modification to the existing B3Format. I'll also add a configuration parameter to support emitting b3 single.

stenskjaer commented 5 years ago

Just want to let you guys know that I'm following this issue and I really look forward to getting my hands on it (in due time). Thanks of a lot for implementing it 👍