Remove Traceparent Extension Handling #2873

ian-mi · 2020-03-31T03:23:51Z

Fixes #2052

Proposed Changes

Remove handling of the traceparent extension. Handling of this extension has moved to the cloudevents SDK.
Remove remaining references to B3 and OpenTracing headers. W3C tracecontext headers are used instead.
Remove the OpenCensus SetSpanID patch. This is a breaking change and require the patch to be removed downstream when eventing versions are updated.

Also fix the config-tracing link.

knative-metrics-robot · 2020-03-31T03:28:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/channel/message_receiver.go	76.8%	75.5%	-1.3

ian-mi · 2020-03-31T03:54:03Z

/assign @n3wscott

slinkydeveloper

@ian-mi Why are you removing the traceparent transformer & the traceparent handling in message_receiver? protocol/http.Message, protocol.http/WriteRequest & protocol.http/WriteResponse doesn't handle the tracing, these still needs to be handled by eventing directly.

ian-mi · 2020-03-31T06:52:42Z

Message_receiver seems to be the wrong place to inject tracing attributes, we should inject them as close to actually sending the event as possible in order to include any additional spans created during fanout or in the SDK. The intention behind adding trace handling to the cloudevents client was to delegate handling of tracing extensions to the SDK and avoid duplicating code across components and protocols. Is there a reasonable place to put this in the SDK? Ideally we should avoid re-implementing this per-protocol since this code is mostly protocol agnostic (except for the ochttp plugins which must be added for HTTP).

slinkydeveloper · 2020-03-31T07:17:37Z

@ian-mi honestly I think the code we have now in message_receiver still doesn't work 😄 I've opened another PR here: #2862, but i'm going a little blindly on tracing b/c i'm not an expert, i definitely need some help to fix it 😄.

I honestly don't want to implement in sdk-go the tracing at binding level (in the message read/write process), i think that should be done by "who sends/receives the messages". In case of channels, this entity is the code that you see in eventing/pkg/channel. That's why this code should live in eventing.

btw in message_receiver the trace is not injected, it's just "prepared" i think...

n3wscott · 2020-03-31T23:01:51Z

THANK YOU SO MUCH <3

/lgtm
/approve

knative-prow-robot · 2020-03-31T23:02:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ian-mi, n3wscott

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

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

n3wscott · 2020-03-31T23:02:23Z

/hold
I did not see the open question

grantr · 2020-03-31T23:33:48Z

I honestly don't want to implement in sdk-go the tracing at binding level (in the message read/write process), i think that should be done by "who sends/receives the messages". In case of channels, this entity is the code that you see in eventing/pkg/channel. That's why this code should live in eventing.

Traceparent is an official CloudEvents extension. It doesn't seem specific to Knative. Why shouldn't the SDK implement it? Is there a technical reason?

This function breaks tracing by setting the transport HTTP client. Users of this function should already be migrated to NewDefaultHTTPClient.

…handling

ian-mi · 2020-03-31T23:57:44Z

I think that we should look for a way to encapsulate this functionality in the SDK as requiring every implementation to deal with setting tracing extensions will be error prone. For now I've left the traceparent transformer in-place but modified the transformer to use the SDK DistributedTracingExtension type rather than the now-deleted traceparent code so that this PR can be unblocked. Lets sync on how we want to handle tracing in the new bindings code-paths and see where I can help with the implementation.

ian-mi · 2020-04-01T02:49:08Z

/unhold

slinkydeveloper · 2020-04-01T07:11:30Z

Traceparent is an official CloudEvents extension. It doesn't seem specific to Knative. Why shouldn't the SDK implement it? Is there a technical reason?

We have the implementation of extensions for the Event data structure, but nothing with transformers, which are not designed to be "materialized" views of some data, but are direct transformations, while the message is written on the wire. In the SDK we provide some generic transformers (add attribute, add extensions, update extension, ...), but I'm not sure if we want to have these specific transformers... It's something we still didn't discussed

slinkydeveloper · 2020-04-01T07:13:47Z

DistributedTracingExtension type rather than the now-deleted traceparent code so that this PR can be unblocked. Lets sync on how we want to handle tracing in the new bindings code-paths and see where I can help with the implementation.

Fine, i'll get in touch with you

slinkydeveloper · 2020-04-01T07:14:20Z

/lgtm

mattmoor · 2020-04-01T13:56:45Z

hack/update-deps.sh

-# artificially set the SpanId. See pkg/tracing/traceparent.go for more details.
-# Produced with:
-# git diff origin/master HEAD -- vendor/go.opencensus.io/trace/trace.go > ./hack/set-span-id.patch
-git apply ${REPO_ROOT_DIR}/hack/set-span-id.patch


Awesome! thanks @ian-mi 🎉

ian-mi added 2 commits March 30, 2020 17:05

Remove reference to B3 and OpenTracing in channel doc

e4ba956

Also fix the config-tracing link.

Replace B3 and OpenTracing headers with Traceparent header in tests

6705902

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 31, 2020

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

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Mar 31, 2020

knative-prow-robot requested review from aslom and chizhg March 31, 2020 03:24

knative-prow-robot assigned n3wscott Mar 31, 2020

slinkydeveloper suggested changes Mar 31, 2020

View reviewed changes

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

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

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2020

ian-mi added 4 commits March 31, 2020 16:41

Use the DistributedTracingExtension type in traceparent_transformer

2fb3bee

Remove traceparent handling from pkg/tracing

0f7e221

Remove OpenCensus SetSpanID patch

cfc6f2c

Remove deprecated NewDefaultClientGivenHttpTransport function.

c8b9c10

This function breaks tracing by setting the transport HTTP client. Users of this function should already be migrated to NewDefaultHTTPClient.

ian-mi force-pushed the remove-traceparent-handling branch from c4536e7 to c8b9c10 Compare March 31, 2020 23:43

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 31, 2020

Merge remote-tracking branch 'origin/master' into remove-traceparent-…

7c21919

…handling

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

knative-prow-robot assigned slinkydeveloper Apr 1, 2020

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

knative-prow-robot merged commit 71a9588 into knative:master Apr 1, 2020

mattmoor reviewed Apr 1, 2020

View reviewed changes

slinkydeveloper mentioned this pull request May 4, 2020

KafkaChannel Tracing knative/eventing-contrib#1155

Merged

travis-minke-sap mentioned this pull request May 5, 2020

CloudEvent Tracing Compliance kyma-incubator/knative-kafka#125

Closed

travis-minke-sap mentioned this pull request Jun 10, 2020

CloudEvent Tracing Compliance knative-extensions/eventing-kafka#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Traceparent Extension Handling #2873

Remove Traceparent Extension Handling #2873

ian-mi commented Mar 31, 2020 •

edited

Loading

knative-metrics-robot commented Mar 31, 2020

ian-mi commented Mar 31, 2020

slinkydeveloper left a comment •

edited

Loading

ian-mi commented Mar 31, 2020 •

edited

Loading

slinkydeveloper commented Mar 31, 2020 •

edited

Loading

n3wscott commented Mar 31, 2020

knative-prow-robot commented Mar 31, 2020

n3wscott commented Mar 31, 2020

grantr commented Mar 31, 2020

ian-mi commented Mar 31, 2020

ian-mi commented Apr 1, 2020

slinkydeveloper commented Apr 1, 2020

slinkydeveloper commented Apr 1, 2020

slinkydeveloper commented Apr 1, 2020

mattmoor Apr 1, 2020

Remove Traceparent Extension Handling #2873

Remove Traceparent Extension Handling #2873

Conversation

ian-mi commented Mar 31, 2020 • edited Loading

Proposed Changes

knative-metrics-robot commented Mar 31, 2020

ian-mi commented Mar 31, 2020

slinkydeveloper left a comment • edited Loading

Choose a reason for hiding this comment

ian-mi commented Mar 31, 2020 • edited Loading

slinkydeveloper commented Mar 31, 2020 • edited Loading

n3wscott commented Mar 31, 2020

knative-prow-robot commented Mar 31, 2020

n3wscott commented Mar 31, 2020

grantr commented Mar 31, 2020

ian-mi commented Mar 31, 2020

ian-mi commented Apr 1, 2020

slinkydeveloper commented Apr 1, 2020

slinkydeveloper commented Apr 1, 2020

slinkydeveloper commented Apr 1, 2020

mattmoor Apr 1, 2020

Choose a reason for hiding this comment

ian-mi commented Mar 31, 2020 •

edited

Loading

slinkydeveloper left a comment •

edited

Loading

ian-mi commented Mar 31, 2020 •

edited

Loading

slinkydeveloper commented Mar 31, 2020 •

edited

Loading