E2E test for Channel Tracing #1858

Harwayne · 2019-09-10T20:19:03Z

Helps with #1765.

Proposed Changes

Add an E2E test for tracing through Channels.

Release Note

NONE

knative-prow-robot · 2019-09-10T20:19:27Z

[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

vendor/knative.dev/pkg/tracing/zipkin.go

vendor/knative.dev/pkg/tracing/opencensus.go

daisy-ycguo · 2019-09-12T14:44:54Z

test/conformance/channel_tracing_test.go

+				expectedNumSpans := 3
+				if tc.incomingTraceId {
+					expectedNumSpans = 4
+				}


Could you explain why the number of spans is different when the incoming message includes trace Id ?

Added a comment to the code, does it make sense?

daisy-ycguo · 2019-09-12T15:58:50Z

test/test_images/logevents/main.go

@@ -32,7 +35,10 @@ func handler(event cloudevents.Event) {
 }

 func main() {
-	c, err := cloudevents.NewDefaultClient()
+	if err := tracing.SetupStaticPublishing(zap.NewNop().Sugar(), "", tracing.AlwaysSample); err != nil {


Why both logevents and sendevents need to call tracing.SetupStaticPublishing? Will it bring a duplicate tracing setup ?

Each Pod must setup tracing independently. The only way I am aware of doing it without our container doing the work is with a sidecar like Istio. As this test does not require Istio, we need to setup the tracing ourselves.

In general, we want to test that a trace that started before the request to the Channel is continued through the Channel to the Channel's subscribers. To do that, we need to ensure there is a trace going into the Channel, hence we instrument the sending pod, as well as ensure the same trace is going to the Channel's subscribers, hence we instrument the logging pod.

Harwayne · 2019-09-16T23:13:26Z

/test pull-knative-eventing-integration-tests

chizhg · 2019-09-17T00:45:00Z

/retest

test/conformance/channel_tracing_test.go

test/e2e-common.sh

chizhg · 2019-09-17T01:28:27Z

test/conformance/channel_tracing_test.go

+		t.Fatalf("Error getting logs: %v", err)
+	}
+	// This is the format that the eventdetails image prints headers.
+	re := regexp.MustCompile("\nGot Header X-B3-Traceid: ([a-zA-Z0-9]{32})\n")


This is a bit hacky...
We should find a better way to get the header, but can be in a different PR..

Totally agree, what do you recommend?

We can define a library to add and parse headers to/from the logs, each header will be quoted with something like <header>...</header>. We can also use the similar way to log and parse the body of cloud event, if we need more accurate check on it.

This requires bigger changes, so it can be in a different PR.

#1945 to track.

test/e2e-common.sh

test/conformance/channel_tracing_test.go

vendor/knative.dev/pkg/test/zipkin/util.go

…acing.

Harwayne · 2019-09-19T20:21:12Z

This is ready for review.

chizhg · 2019-09-19T20:42:29Z

test/conformance/channel_tracing_test.go

+		incomingTraceId bool
+		istio           bool
+	}{
+		//"no incoming trace id": {},


Ah, so this one is still not working? :)

Works consistently on my Linux desktop, is flaky on my Mac laptop. For now, I'll leave it commented out (or I could remove the line if you prefer), so that the rest of the test can get in.

Removed it for now, rather than just commenting it out.

chizhg · 2019-09-19T20:47:29Z

test/conformance/channel_tracing_test.go

+	for n, tc := range testCases {
+		loggerPodName := "logger"
+		t.Run(n, func(t *testing.T) {
+			channelTestRunner.RunTests(t, common.FeatureBasic, func(st *testing.T, channel string) {


This function needs to be defined as a helper function, if you want it to be ported to eventing-contrib to test other channel implementations, example https://github.com/knative/eventing/blob/master/test/conformance/helpers/channel_header_single_event_helper.go

Done.

I didn't know, thanks for pointing it out.

…e reused by eventing-contrib.

chizhg · 2019-09-20T00:33:43Z

/lgtm

Harwayne added 9 commits September 9, 2019 14:13

WIP

ff72461

Simple one works.

ca5f88a

Revert deep vendored stuff.

9ca4206

Revert upload test images.

5d4cd00

It works!

b442008

Run in parallel.

a9a55d1

pkg determines the hostname.

5f536c5

Cleanup

7c9b13c

Use GetLog.

68693df

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 10, 2019

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Sep 10, 2019

knative-prow-robot requested review from chizhg and matzew September 10, 2019 20:19

Harwayne changed the title ~~E2E tracing~~ [WIP] E2E test for Channel Tracing Sep 10, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2019

daisy-ycguo reviewed Sep 11, 2019

View reviewed changes

vendor/knative.dev/pkg/tracing/zipkin.go Outdated Show resolved Hide resolved

vendor/knative.dev/pkg/tracing/opencensus.go Outdated Show resolved Hide resolved

Harwayne added 2 commits September 11, 2019 15:56

Merge branch 'master' into e2e-tracing

e165ca5

Try installing monitoring and setting tracing to 100%.

3b33fb9

daisy-ycguo reviewed Sep 12, 2019

View reviewed changes

daisy-ycguo mentioned this pull request Sep 12, 2019

E2E test for broker tracing #1878

Closed

Harwayne added 4 commits September 12, 2019 10:23

Add a comment about where the expectedNumSpans comes from.

ca04836

Create the istio-system ns.

3a3fc29

Update knative.dev/pkg.

e833388

Switch back to knative.dev/pkg's zipkin.

3a84ed5

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 12, 2019

Harwayne added 2 commits September 16, 2019 15:54

Small changes.

1a8a05c

Merge branch 'master' into e2e-tracing

0fbd809

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

chizhg reviewed Sep 17, 2019

View reviewed changes

test/e2e-common.sh Outdated Show resolved Hide resolved

test/e2e-common.sh Show resolved Hide resolved

test/conformance/channel_tracing_test.go Outdated Show resolved Hide resolved

Works on the Mac.

a13ed44

Harwayne commented Sep 18, 2019

View reviewed changes

vendor/knative.dev/pkg/test/zipkin/util.go Outdated Show resolved Hide resolved

Update knative/pkg.

30c5b0d

Harwayne mentioned this pull request Sep 18, 2019

Update knative/pkg. #1919

Merged

Harwayne added 7 commits September 18, 2019 10:42

Merge branch 'master' into e2e-tracing

b7acbfb

Merge branch 'update-pkg' into e2e-tracing

1a36cd2

Merge branch 'master' into e2e-tracing

5ef3845

Switch to KNATIVE_MONITORING_RELEASE.

fd4e160

Add logging for the tracing setup in all the test images that have tr…

04f6246

…acing.

Only set the ConfigMap in the code, not the shell script.

86fe35a

Remove setting the ConfigMap again.

22d45d3

Harwayne changed the title ~~[WIP] E2E test for Channel Tracing~~ E2E test for Channel Tracing Sep 18, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2019

chizhg reviewed Sep 19, 2019

View reviewed changes

Move the bulk of the test into the helpers directory so that it can b…

037fe7b

…e reused by eventing-contrib.

knative-prow-robot assigned chizhg Sep 20, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2019

knative-prow-robot merged commit 989058c into knative:master Sep 20, 2019

Harwayne mentioned this pull request Sep 20, 2019

Reliable header logging in tests #1945

Closed

boramalper mentioned this pull request Jun 23, 2021

How to set OpenTelemetry tracing parameters? vhive-serverless/vHive#299

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E test for Channel Tracing #1858

E2E test for Channel Tracing #1858

Harwayne commented Sep 10, 2019 •

edited

Loading

knative-prow-robot commented Sep 10, 2019

daisy-ycguo Sep 12, 2019 •

edited

Loading

Harwayne Sep 12, 2019

daisy-ycguo Sep 12, 2019 •

edited

Loading

Harwayne Sep 12, 2019

Harwayne commented Sep 16, 2019

chizhg commented Sep 17, 2019

chizhg Sep 17, 2019

Harwayne Sep 18, 2019

chizhg Sep 19, 2019

Harwayne Sep 20, 2019

Harwayne commented Sep 19, 2019

chizhg Sep 19, 2019

Harwayne Sep 19, 2019

Harwayne Sep 19, 2019

chizhg Sep 19, 2019

Harwayne Sep 19, 2019

chizhg commented Sep 20, 2019

E2E test for Channel Tracing #1858

E2E test for Channel Tracing #1858

Conversation

Harwayne commented Sep 10, 2019 • edited Loading

Proposed Changes

knative-prow-robot commented Sep 10, 2019

daisy-ycguo Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daisy-ycguo Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harwayne commented Sep 16, 2019

chizhg commented Sep 17, 2019

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 Sep 19, 2019

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

chizhg commented Sep 20, 2019

Harwayne commented Sep 10, 2019 •

edited

Loading

daisy-ycguo Sep 12, 2019 •

edited

Loading

daisy-ycguo Sep 12, 2019 •

edited

Loading