Add tracecontextb3 HTTPFormats #1429

Harwayne · 2020-06-19T19:29:43Z

Changes

Add a tracecontextb3 package which contains three propagation.HTTPFormats.
- All formats read both TraceContext and B3 information during ingress, preferring TraceContext.
- Formats differ in what they write for egress, based on their name.
  - B3Egress sends only b3.
  - TraceContextEgress sends only TraceContext
  - TraceContextB3Egress sends both.

This is being created because we found that serving relied on B3 tracing headers. While eventing relied on TraceContext tracing headers. So an HTTP request sent from a Broker to a Knative Service would break into two distinct traces.

It will utilize either B3 or TraceContext propagation formats coming in (preferring TraceContext) and while sending both.

ian-mi · 2020-06-19T20:04:37Z

Maybe it would be better to only set the outgoing headers which are present in the incoming request? This could be a little tricky to implement but it should be possible to wrap the handler with one which inserts a format key into ctx. On the other hand there may be little downside to always writing both formats.

vagababov

/lint

vagababov · 2020-06-19T20:37:00Z

tracing/b3tracecontext/b3_traceparent.go

+
+var _ propagation.HTTPFormat = (*HTTPFormat)(nil)
+
+func (H *HTTPFormat) SpanContextFromRequest(req *http.Request) (trace.SpanContext, bool) {


Suggested change

func (H *HTTPFormat) SpanContextFromRequest(req *http.Request) (trace.SpanContext, bool) {

func (h *HTTPFormat) SpanContextFromRequest(req *http.Request) (trace.SpanContext, bool) {

Removed entirely.

vagababov · 2020-06-19T20:37:27Z

tracing/b3tracecontext/b3_traceparent.go

+	traceContext.SpanContextToRequest(sc, req)
+	b3Format.SpanContextToRequest(sc, req)
+}


so we set both headers?

knative-prow-robot

@vagababov: 2 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tracing/b3tracecontext/b3_traceparent.go

Harwayne · 2020-06-19T21:02:24Z

Maybe it would be better to only set the outgoing headers which are present in the incoming request? This could be a little tricky to implement but it should be possible to wrap the handler with one which inserts a format key into ctx. On the other hand there may be little downside to always writing both formats.

I think there is little downside in always writing both formats. Given that, I think writing a new Handler is overkill for this situation.

And even if we did write that Handler, I think we would still need this (write both formats) propagator for situations where something other than HTTP was used as the input. For example an eventing piece reading from Kafka or Pub/Sub. A wrapping handler would not help, as we don't have an incoming HTTP request. We would need to wrap an ochttp.Transport. It seems far easier, and with little downside, to just use the same propagator for both.

tracing/b3tracecontext/b3_traceparent.go

matzew · 2020-06-22T15:28:43Z

/assign @slinkydeveloper mind taking a look?

slinkydeveloper · 2020-06-22T15:34:03Z

Yeah i think that could be interesting for kafka tracing too!

/assign

ian-mi · 2020-06-22T15:49:06Z

tracing/propagation/http_format_sequence.go

+// HTTPFormatSequence.Formats.
+// For outgoing requests, it will apply all the formats to the outgoing request, in the order of
+// HTTPFormatSequence.Formats.
+type HTTPFormatSequence struct {


nit: The struct isn't necessary here.

Yeah, just inline with
type HHFS []proo.HTTPFormat.

ian-mi · 2020-06-22T15:49:23Z

/lgtm

slinkydeveloper

Similar to the issue @ian-mi raised, I don't get why we should write always in both formats, to me it sounds pretty overkill (and adds unnecessary overhead to the message). We could make it configurable for the egress maybe? Like b3/tracecontext/both and default to tracecontext? I would prefer to stick with one format on egress side, and support both on ingress.

Also, before merging this one, can you run the e2e tracing tests on eventing with this patch?

Harwayne · 2020-06-22T16:58:38Z

Similar to the issue @ian-mi raised, I don't get why we should write always in both formats, to me it sounds pretty overkill (and adds unnecessary overhead to the message). We could make it configurable for the egress maybe? Like b3/tracecontext/both and default to tracecontext? I would prefer to stick with one format on egress side, and support both on ingress.

My main hesitation of only outputting one format is that we are more likely to hit a component that speaks the other form of tracing, thus stopping the existing trace. That is what is happening in Knative today (before this PR). Every hop between eventing and serving cuts the trace because they use different tracing formats. Once this PR is in and all the downstream repos are updated and the user has updated to all those new versions, then it would no longer be a problem for Knative itself. But I'm worried about any software downstream of Knative that happens to rely on the existing format.

If we go the route of configurable egress, then I think we need to have serving use B3 output and eventing use TraceContext output, to match what is already present in both places. As both may already have downstream software that relies on it.

I think the additional overhead on the message is small enough (either ~70 or ~100 bytes), that its existence is justified by the increased chance of interoperating with other software.

Also, before merging this one, can you run the e2e tracing tests on eventing with this patch?

Will do.

slinkydeveloper · 2020-06-22T17:15:45Z

Every hop between eventing and serving cuts the trace because they use different tracing formats.

To me this sounds like a different problem, because we don't have an alignment between their tracing propagation and our tracing propagation. Also as far as I understood the b3 propagation is specific to zipkin (which, by the way, can understand also traces propagated with trace context), so that's another reason for me to prefer making it configurable (same on serving).

Also as far as I understood from the problem, we should make the config-tracing the unique source of truth both for serving and eventing, and configure on it the format to use. So all the knative installation will use the same format, and user won't have to worry about which one uses which format. This way, we won't need to use both formats but we can just stick with one of the two (and eventually, as opt-in feature, emit both)

I think the additional overhead on the message is small enough (either ~70 or ~100 bytes)

Well, it's always additional bytes that are potentially not used downstream.

Harwayne · 2020-06-22T21:05:45Z

Every hop between eventing and serving cuts the trace because they use different tracing formats.

To me this sounds like a different problem, because we don't have an alignment between their tracing propagation and our tracing propagation. Also as far as I understood the b3 propagation is specific to zipkin (which, by the way, can understand also traces propagated with trace context), so that's another reason for me to prefer making it configurable (same on serving).

Also as far as I understood from the problem, we should make the config-tracing the unique source of truth both for serving and eventing, and configure on it the format to use. So all the knative installation will use the same format, and user won't have to worry about which one uses which format. This way, we won't need to use both formats but we can just stick with one of the two (and eventually, as opt-in feature, emit both)

Makes sense. This PR will lay the ground work for allowing that option. This repo will need to extend the config-tracing format to include that option. Each downstream option will need to ensure all places that trace have the value plumbed through.

I think the additional overhead on the message is small enough (either ~70 or ~100 bytes)

Well, it's always additional bytes that are potentially not used downstream.

Done. Although I still believe it is better to send the extra bytes than to have the trace broken.

knative-metrics-robot · 2020-06-22T21:06:44Z

The following is the coverage report on the affected files.
Say /test pull-knative-pkg-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
tracing/propagation/http_format_sequence.go	Do not exist	100.0%

slinkydeveloper

/lgtm

vaikas · 2020-06-23T17:24:36Z

/lgtm
/approve

knative-prow-robot · 2020-06-23T17:24:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne, slinkydeveloper, vaikas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Harwayne added 2 commits June 19, 2020 12:26

Add a b3tracecontext.HTTPFormat.

b80ad66

It will utilize either B3 or TraceContext propagation formats coming in (preferring TraceContext) and while sending both.

hack/update-deps.sh

d99f4a6

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 19, 2020

knative-prow-robot added area/test-and-release size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2020

knative-prow-robot requested review from chaodaiG and yt3liu June 19, 2020 19:29

Harwayne assigned vaikas Jun 19, 2020

vagababov reviewed Jun 19, 2020

View reviewed changes

knative-prow-robot reviewed Jun 19, 2020

View reviewed changes

tracing/b3tracecontext/b3_traceparent.go Outdated Show resolved Hide resolved

tracing/b3tracecontext/b3_traceparent.go Outdated Show resolved Hide resolved

PR comments.

6982f09

ian-mi reviewed Jun 19, 2020

View reviewed changes

tracing/b3tracecontext/b3_traceparent.go Outdated Show resolved Hide resolved

Move to HTTPFormatSequence.

ba1f6d9

knative-prow-robot assigned slinkydeveloper Jun 22, 2020

ian-mi reviewed Jun 22, 2020

View reviewed changes

knative-prow-robot assigned ian-mi Jun 22, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020

slinkydeveloper reviewed Jun 22, 2020

View reviewed changes

Remove the struct.

cb68763

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020

Allow distinct ingress and egress formats.

8a25d03

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 22, 2020

Harwayne changed the title ~~Add a b3tracecontext.HTTPFormat~~ Add tracecontextb3 HTTPFormats Jun 22, 2020

slinkydeveloper approved these changes Jun 23, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020

knative-prow-robot merged commit 5658d93 into knative:master Jun 23, 2020

Harwayne deleted the b3traceparent branch June 23, 2020 17:42

This was referenced Jun 23, 2020

Accept both B3 and TraceContext style tracing knative/eventing#3388

Merged

Accept both B3 and TraceContext style tracing knative/serving#8440

Merged

Accept both B3 and TraceContext Tracing google/knative-gcp#1335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracecontextb3 HTTPFormats #1429

Add tracecontextb3 HTTPFormats #1429

Harwayne commented Jun 19, 2020 •

edited

Loading

ian-mi commented Jun 19, 2020

vagababov left a comment

vagababov Jun 19, 2020

Harwayne Jun 19, 2020

vagababov Jun 19, 2020

Harwayne Jun 19, 2020

knative-prow-robot left a comment

Harwayne commented Jun 19, 2020

matzew commented Jun 22, 2020

slinkydeveloper commented Jun 22, 2020

ian-mi Jun 22, 2020

vagababov Jun 22, 2020

Harwayne Jun 22, 2020

ian-mi commented Jun 22, 2020

slinkydeveloper left a comment •

edited

Loading

Harwayne commented Jun 22, 2020

slinkydeveloper commented Jun 22, 2020 •

edited

Loading

Harwayne commented Jun 22, 2020

knative-metrics-robot commented Jun 22, 2020

slinkydeveloper left a comment

vaikas commented Jun 23, 2020

knative-prow-robot commented Jun 23, 2020


		var _ propagation.HTTPFormat = (*HTTPFormat)(nil)

		func (H HTTPFormat) SpanContextFromRequest(req http.Request) (trace.SpanContext, bool) {

Add tracecontextb3 HTTPFormats #1429

Add tracecontextb3 HTTPFormats #1429

Conversation

Harwayne commented Jun 19, 2020 • edited Loading

Changes

ian-mi commented Jun 19, 2020

vagababov left a comment

Choose a reason for hiding this comment

vagababov Jun 19, 2020

Choose a reason for hiding this comment

Harwayne Jun 19, 2020

Choose a reason for hiding this comment

vagababov Jun 19, 2020

Choose a reason for hiding this comment

Harwayne Jun 19, 2020

Choose a reason for hiding this comment

knative-prow-robot left a comment

Choose a reason for hiding this comment

Harwayne commented Jun 19, 2020

matzew commented Jun 22, 2020

slinkydeveloper commented Jun 22, 2020

ian-mi Jun 22, 2020

Choose a reason for hiding this comment

vagababov Jun 22, 2020

Choose a reason for hiding this comment

Harwayne Jun 22, 2020

Choose a reason for hiding this comment

ian-mi commented Jun 22, 2020

slinkydeveloper left a comment • edited Loading

Choose a reason for hiding this comment

Harwayne commented Jun 22, 2020

slinkydeveloper commented Jun 22, 2020 • edited Loading

Harwayne commented Jun 22, 2020

knative-metrics-robot commented Jun 22, 2020

slinkydeveloper left a comment

Choose a reason for hiding this comment

vaikas commented Jun 23, 2020

knative-prow-robot commented Jun 23, 2020

Harwayne commented Jun 19, 2020 •

edited

Loading

slinkydeveloper left a comment •

edited

Loading

slinkydeveloper commented Jun 22, 2020 •

edited

Loading