Accept both B3 and TraceContext Tracing #1335

Harwayne · 2020-06-24T00:39:03Z

Helps with #1147.

Proposed Changes

Use the tracecontextb3.TraceContextEgress in all of our client code for consistency with our server code (which should have been updated when upstream Accept both B3 and TraceContext style tracing knative/eventing#3388 was vendored in).
Only TraceContext style tracing is egressed.
- This was done for backwards compatibility with the existing system, which sends via TraceContext style. We could output both formats, but it was determined that the additional overhead of ~100 bytes per message was too high for the potential benefit. See here for more discussion.

Release Note

NONE

…ontext style. Egressing only TraceContext style is done for backwards compatibility with the existing system, which sends TraceContext style. We could output both styles, but it was determined that the additional overhead of ~100 bytes on every outgoing message was not worth it.

knative-prow-robot · 2020-06-24T00:39:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne

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~~ [Harwayne]

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

Harwayne · 2020-06-24T00:40:20Z

@nachocano should we hold this until after #1296 is in?

ericlem · 2020-06-24T00:49:53Z

/lgtm
/hold
Holding for nacho's reply

nachocano · 2020-06-24T00:55:21Z

Yes. It'd be great if we can hold it. We need the other one in asap. And this will have a bunch of conflicts. Hopefully we will get it in later today or tomorrow. There is a new problem with UTs with the newer eventing reconcilers. Cong is helping investigate

…

On Tue, Jun 23, 2020 at 5:50 PM Eric Lemar ***@***.***> wrote: /lgtm /hold Holding for nacho's reply — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1335 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABD65DE3INBEPRFT7MQTRTDRYFET3ANCNFSM4OGFMD2A> .

nachocano · 2020-06-24T05:38:58Z

And this will have a bunch of conflicts.

I take this back. I was changing all e2e to start using cev2 and removing the good_client, but I rollbacked those changes as it can be done in a follow up, after mine and yours get in...
I'm currently experiencing the flakiness on the Brokercell UT. Hope we can get mine in early tomorrow

nachocano · 2020-06-24T20:27:20Z

/hold cancel
the other PR got merged. Thanks for waiting @Harwayne !

ericlem · 2020-06-24T22:04:05Z

/lgtm

Harwayne · 2020-06-24T22:38:50Z

/hold

I clearly did something wrong:

TestGCPBrokerTracing: broker_tracing.go:45: multiple root spans: span_id:7881518364301324575 name:"Recv./test-g-c-p-broker-tracing-qwp7n/gcp-bfcyyklc" start_time:{seconds:1593035140 nanos:460429848} end_time:{seconds:1593035140 nanos:783118377} parent_span_id:22965739263654966 labels:{key:"/http/host" value:"default-brokercell-ingress.cloud-run-events.svc.cluster.local"} labels:{key:"/http/method" value:"POST"} labels:{key:"/http/path" value:"/test-g-c-p-broker-tracing-qwp7n/gcp-bfcyyklc"} labels:{key:"/http/user_agent" value:"Go-http-client/1.1"} labels:{key:"g.co/agent" value:"opencensus-go 0.23.0; stackdriver-exporter 0.10.0"} labels:{key:"http.url" value:"/test-g-c-p-broker-tracing-qwp7n/gcp-bfcyyklc"}, span_id:6804529643887419959 name:"Recv./test-g-c-p-broker-tracing-qwp7n/gcp-bfcyyklc" start_time:{seconds:1593035140 nanos:460488105} end_time:{seconds:1593035140 nanos:783081955} parent_span_id:22965739263654966 labels:{key:"/http/host" value:"default-brokercell-ingress.cloud-run-events.svc.cluster.local"} labels:{key:"/http/method" value:"POST"} labels:{key:"/http/path" value:"/test-g-c-p-broker-tracing-qwp7n/gcp-bfcyyklc"} labels:{key:"/http/user_agent" value:"Go-http-client/1.1"} labels:{key:"g.co/agent" value:"opencensus-go 0.23.0; stackdriver-exporter 0.10.0"} labels:{key:"http.url" value:"/test-g-c-p-broker-tracing-qwp7n/gcp-bfcyyklc"}

ericlem · 2020-06-24T22:56:52Z

Is there a possibility the handlers were already wrapped (or internally registered traces) in the new wrapping calls you added?

pkg/broker/ingress/handler.go

ian-mi · 2020-07-01T22:39:52Z

pkg/utils/clients/factories.go

@@ -68,7 +67,7 @@ func NewHTTPClient(ctx context.Context, maxConnsPerHost MaxConnsPerHost) *nethtt
 				MaxConnsPerHost:     int(maxConnsPerHost),
 				IdleConnTimeout:     30 * time.Second,
 			},
-			Propagation: &tracecontext.HTTPFormat{},
+			Propagation: tracecontextb3.TraceContextEgress,


This shouldn't be necessary as the client is already configured to use tracecontext.

I think you're right, this isn't really needed as this function returns an http.Client, which is egress only. However, I would like to change it to be consistent with all other places in the repo. The actual difference in runtime will be miniscule.

Can we just use the regular tracecontext format everywhere we don't need special behavior? That would maintain consistency and I feel that it is somewhat confusing to configure the client with a format that has special unneeded ingress behavior. I reviewed the tracecontextb3 PR before the formats were split but it should actually be unnecessary to have separate egress and ingress formats since the format is only every used with a handler (ingress) or a client transport (egress).

Can we just use the regular tracecontext format everywhere we don't need special behavior? That would maintain consistency

Consistency with what? For the rest of this comment I will assume it is consistency within this repo or knative more generally. As opposed to consistency with the wider world of software using OpenCensus.

I disagree that it is simpler or more consistent to use &tracecontext.HTTPFormat{} for ingress and tracecontextb3.TraceContextEgress for egress, rather than always using tracecontextb3.TraceContextEgress for both. There is some runtime overhead, but it is going to be effectively imperceptible (one stack frame and an iteration of a size one slice per HTTP request, AFAICT).

If we always use the same format, then developers don't have to understand when to use each. They just always use tracecontextb3.TraceContextEgress, which gives the desired behavior in both circumstances.

ian-mi · 2020-07-01T22:41:09Z

test/test_images/internal/knockdown/knockdown.go

@@ -42,7 +43,7 @@ type Config struct {
 // Main should be called by the process' main function. It will run the knockdown test. The return
 // value MUST be used in os.Exit().
 func Main(config Config, kdr Receiver) int {
-	client, err := cloudevents.NewDefaultClient()
+	client, err := kncloudevents.NewDefaultClient()


I'm not sure that we should change the test image as we expect to always be given tracecontext headers.

My reasoning is consistency. There is little downside to accepting both, so why not do it? The only real reason we wouldn't is to verify that the incoming headers truly are TraceContext, rather than B3.

I also expect sooner or later someone will copy-paste this code and want the 'best practice' to be what they copy.

Right my concern is detecting a regression where B3 is used instead of tracecontext. I also think that for normal event consumers accepting only tracecontext is good practice as that is what they should expect the broker to give them. On the other hand broker components should always use the clients package providers.

Switched back with a comment explaining this is for tests only.

pkg/pubsub/publisher/publisher.go

ian-mi · 2020-07-01T22:59:19Z

pkg/broker/handler/providers.go

@@ -45,7 +45,7 @@ var (
 				MaxConnsPerHost:     500,
 				IdleConnTimeout:     30 * time.Second,
 			},
-			Propagation: &tracecontext.HTTPFormat{},
+			Propagation: tracecontextb3.TraceContextEgress,


This also should be unnecessary as the client is already configured to use trace context.

Is this the same argument as above? Namely that because this is an http.Client, it is inherently an egress only thing, and therefore doesn't need this new format?

pkg/pubsub/publisher/publisher.go

Harwayne · 2020-07-11T01:06:33Z

/unhold

Harwayne · 2020-07-11T04:37:00Z

/retest

Harwayne · 2020-07-11T05:10:17Z

/retest

Harwayne · 2020-07-11T18:37:10Z

/retest

ian-mi · 2020-07-13T18:13:00Z

/lgtm

Harwayne · 2020-07-13T20:17:07Z

/retest

Harwayne · 2020-07-14T01:03:39Z

/retest

Harwayne · 2020-07-14T22:01:29Z

/retest

knative-test-reporter-robot · 2020-07-14T22:37:05Z

The following jobs failed:

Test name	Triggers	Retries
pull-google-knative-gcp-integration-tests	pull-google-knative-gcp-integration-tests pull-google-knative-gcp-integration-tests pull-google-knative-gcp-integration-tests	3/3
pull-google-knative-gcp-wi-tests	pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests pull-google-knative-gcp-wi-tests 2020-07-14 22:37:00.384 +0000 UTC	3/3

Job pull-google-knative-gcp-wi-tests expended all 3 retries without success.

Harwayne · 2020-07-15T15:42:36Z

/retest

Harwayne added 2 commits June 23, 2020 17:21

hack/update-deps.sh

41fa4dd

knative-prow-robot requested review from ericlem and grantr June 24, 2020 00:39

knative-prow-robot added the approved label Jun 24, 2020

googlebot added the cla: yes (override cla status due to multiple authors bug) label Jun 24, 2020

knative-prow-robot added the size/XXL label Jun 24, 2020

knative-prow-robot added the do-not-merge/hold label Jun 24, 2020

knative-prow-robot assigned ericlem Jun 24, 2020

knative-prow-robot added the lgtm label Jun 24, 2020

knative-prow-robot added needs-rebase and removed do-not-merge/hold labels Jun 24, 2020

Harwayne added 3 commits June 24, 2020 13:57

Merge branch 'master' into tracecontext-egress

51776c0

hack/update-deps.sh

35c6509

Update trace formats for the newly merged code.

9bafaf3

knative-prow-robot added size/XL and removed lgtm needs-rebase size/XXL labels Jun 24, 2020

knative-prow-robot added the lgtm label Jun 24, 2020

knative-prow-robot added the do-not-merge/hold label Jun 24, 2020

ericlem reviewed Jun 24, 2020

View reviewed changes

pkg/broker/ingress/handler.go Outdated Show resolved Hide resolved

knative-prow-robot added the needs-rebase label Jun 28, 2020

Merge branch 'master' into tracecontext-egress

0b8c39d

knative-prow-robot added size/M and removed lgtm needs-rebase size/XL labels Jun 30, 2020

Do not double wrap the Broker Ingress.

8b314a2

ian-mi suggested changes Jul 1, 2020

View reviewed changes

ericlem reviewed Jul 1, 2020

View reviewed changes

pkg/pubsub/publisher/publisher.go Outdated Show resolved Hide resolved

ian-mi reviewed Jul 1, 2020

View reviewed changes

Harwayne added 3 commits July 1, 2020 16:43

Do not double wrap the Broker Publisher.

2ec0222

Merge branch 'master' into tracecontext-egress

5d667ef

Switch knockdown tests back to cloudevents.NewDefaultClient().

83633a6

knative-prow-robot removed the do-not-merge/hold label Jul 11, 2020

knative-prow-robot assigned ian-mi Jul 13, 2020

knative-prow-robot added the lgtm label Jul 13, 2020

knative-prow-robot merged commit d8db92a into google:master Jul 15, 2020

Harwayne deleted the tracecontext-egress branch July 15, 2020 18:45

grac3gao-zz mentioned this pull request Jul 15, 2020

Update sender Pod image #1444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept both B3 and TraceContext Tracing #1335

Accept both B3 and TraceContext Tracing #1335

Harwayne commented Jun 24, 2020 •

edited

Loading

knative-prow-robot commented Jun 24, 2020

Harwayne commented Jun 24, 2020

ericlem commented Jun 24, 2020

nachocano commented Jun 24, 2020 via email

nachocano commented Jun 24, 2020

nachocano commented Jun 24, 2020

ericlem commented Jun 24, 2020

Harwayne commented Jun 24, 2020

ericlem commented Jun 24, 2020

ian-mi Jul 1, 2020

Harwayne Jul 1, 2020

ian-mi Jul 1, 2020

Harwayne Jul 11, 2020

ian-mi Jul 1, 2020

Harwayne Jul 1, 2020

ian-mi Jul 1, 2020

Harwayne Jul 11, 2020

ian-mi Jul 1, 2020

Harwayne Jul 11, 2020

Harwayne commented Jul 11, 2020

Harwayne commented Jul 11, 2020

Harwayne commented Jul 11, 2020

Harwayne commented Jul 11, 2020

ian-mi commented Jul 13, 2020

Harwayne commented Jul 13, 2020

Harwayne commented Jul 14, 2020

Harwayne commented Jul 14, 2020

knative-test-reporter-robot commented Jul 14, 2020

Harwayne commented Jul 15, 2020

Accept both B3 and TraceContext Tracing #1335

Accept both B3 and TraceContext Tracing #1335

Conversation

Harwayne commented Jun 24, 2020 • edited Loading

Proposed Changes

knative-prow-robot commented Jun 24, 2020

Harwayne commented Jun 24, 2020

ericlem commented Jun 24, 2020

nachocano commented Jun 24, 2020 via email

nachocano commented Jun 24, 2020

nachocano commented Jun 24, 2020

ericlem commented Jun 24, 2020

Harwayne commented Jun 24, 2020

ericlem commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harwayne commented Jul 11, 2020

Harwayne commented Jul 11, 2020

Harwayne commented Jul 11, 2020

Harwayne commented Jul 11, 2020

ian-mi commented Jul 13, 2020

Harwayne commented Jul 13, 2020

Harwayne commented Jul 14, 2020

Harwayne commented Jul 14, 2020

knative-test-reporter-robot commented Jul 14, 2020

Harwayne commented Jul 15, 2020

Harwayne commented Jun 24, 2020 •

edited

Loading