E2E Test for Broker Tracing #1995

Harwayne · 2019-10-03T23:57:19Z

Helps with #1766

Proposed Changes

Add an E2E Test for tracing through the Broker.

The initial work for this PR was done by @daisy-ycguo in #1878.

Release Note

Tracing works through Broker, Triggers, and their replies.

knative-prow-robot · 2019-10-03T23:57:35Z

[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

mattmoor-sockpuppet · 2019-10-04T00:03:42Z

test/conformance/helpers/broker_tracing_test_helper.go

+	channelTypeMeta := common.GetChannelTypeMeta(channel)
+	client.CreateBrokerOrFail(brokerName, channelTypeMeta)
+
+	// DO NOT SUBMIT!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!111!!!!!!!!!!!!!!!!!!


Found "DO NOT SUBMIT".

Harwayne · 2019-10-04T01:44:28Z

test/conformance/helpers/tracing/traces.go

+				return it[key] < jt[key]
+			}
+		}
+		panic(fmt.Errorf("can't tell the difference between %v, %v", ic, jc))


Don't panic.

Harwayne · 2019-10-04T16:27:34Z

/retest

Harwayne · 2019-10-04T16:32:59Z

/test pull-knative-eventing-integration-tests

Harwayne · 2019-10-04T16:49:08Z

/test pull-knative-eventing-integration-tests

Harwayne · 2019-10-04T18:23:28Z

/test pull-knative-eventing-integration-tests

test/common/operation.go

Harwayne · 2019-10-07T20:09:48Z

@chizhg This is ready for review.

chizhg · 2019-10-07T23:51:00Z

pkg/broker/filter/filter_handler.go

@@ -252,10 +253,9 @@ func (r *Handler) sendEvent(ctx context.Context, tctx cloudevents.HTTPTransportC
 	}

 	sendingCTX := utils.ContextFrom(tctx, subscriberURI)


Ctx instead of CTX?

From what I understand, Go prefers non-word segments to share capitalization. https://talks.golang.org/2014/names.slide#5 refers to acronyms, "Acronyms should be all capitals, as in ServeHTTP and IDProcessor." CTX isn't an acronym, but it does feel like a similar, non-word segment. So I chose to keep it at the same level of capitalization.

chizhg · 2019-10-07T23:51:47Z

pkg/broker/ingress/ingress_handler.go

@@ -91,6 +89,10 @@ func (h *Handler) serveHTTP(ctx context.Context, event cloudevents.Event, resp *

 	start := time.Now()
 	sendingCTX := utils.ContextFrom(tctx, h.ChannelURI)


test/common/creation.go

chizhg · 2019-10-07T23:56:12Z

test/common/tracker.go

@@ -144,7 +145,7 @@ func (t *Tracker) WaitForKResourcesReady() error {
 	t.logf("Waiting for all KResources to become ready")
 	for _, metaResource := range t.resourcesToCheckStatus {
 		if err := base.WaitForResourceReady(t.dynamicClient, &metaResource); err != nil {
-			return err
+			return fmt.Errorf("waiting for %+v to become ready: %v", metaResource, err)


Suggested change

return fmt.Errorf("waiting for %+v to become ready: %v", metaResource, err)

return fmt.Errorf("failed waiting for %+v to become ready: %v", metaResource, err)

chizhg · 2019-10-08T00:11:35Z

test/conformance/helpers/broker_tracing_test_helper.go

+
+	// Create a Trigger that receives events (type=bar) and sends them to the logger Pod.
+	loggerTrigger := client.CreateTriggerOrFail(
+		"logger",


Define it as a constant?

I could, but why? This is the only place the value is used. Everywhere else it is needed, we use loggerTrigger.Name.

test/conformance/helpers/broker_tracing_test_helper.go

chizhg · 2019-10-08T00:20:46Z

test/conformance/helpers/broker_tracing_test_helper.go

+						Tags: map[string]string{
+							"http.method":      "POST",
+							"http.status_code": "202",
+							"http.url":         fmt.Sprintf("http://%s/", loggerSVCHost),


Can we remove the trailing /?

No. This is doing an exact match against the values recorded in the trace. The trace records the trailing /, so the test needs it too.

chizhg · 2019-10-08T00:26:19Z

test/conformance/helpers/tracing/traces.go

-	Kind                     model.Kind
-	LocalEndpointServiceName string
-	Tags                     map[string]string
+	Note                     string            `json:"aaNote,omitempty"`


Are these a and aa prefixes added for sorting? If yes can we have some comments for them?

Added a comment, does it make sense?

chizhg · 2019-10-08T00:30:57Z

test/conformance/helpers/tracing/traces.go

 }

 func (t TestSpanTree) String() string {
 	b, _ := json.Marshal(t)
 	return string(b)
 }

+func (t *TestSpanTree) SortChildren() {


Can we have some comments for what this sort function does?

chizhg · 2019-10-08T00:36:26Z

test/conformance/helpers/tracing/traces.go

+	if g, w := actual.ToTestSpanTree().SpanCount(), t.SpanCount(); g != w {
+		return fmt.Errorf("unexpected number of spans. got %d want %d", g, w)
+	}
+	t.SortChildren()
 	err := traceTreeMatches(".", t, actual)


Inline the error check?

chizhg · 2019-10-08T00:43:28Z

test/conformance/helpers/broker_tracing_test_helper.go

+
+	// Due to https://github.com/knative/eventing/issues/1998, wait for the Broker to become ready
+	// before creating the Triggers.
+	err := client.WaitForResourceReady(brokerName, &metav1.TypeMeta{


We can use common.BrokerTypeMeta here.
Also sounds like this is a bug we'll fix anyway, can we change the comment to a TODO? Since we also have the similar logic in e2e tests.

Done.

I think the other e2e tests use the default Broker, which is likely created via the injection annotation and happens to be ready in time.

There are also two test cases that create brokers:

eventing/test/e2e/helpers/broker_channel_flow_test_helper.go

Line 62 in b2e44b5

client.CreateBrokerOrFail(brokerName, channelTypeMeta)

eventing/test/e2e/helpers/broker_event_transformation_test_helper.go

Line 57 in b2e44b5

client.CreateBrokerOrFail(brokerName, channelTypeMeta)

chizhg · 2019-10-08T23:04:37Z

test/conformance/broker_tracing_test.go

+// +build e2e
+
+/*
+Copyright 2019 The Knative Authors


Suggested change

Copyright 2019 The Knative Authors

Copyright 2019 The Knative Authors

chizhg · 2019-10-08T23:06:23Z

test/conformance/broker_tracing_test.go

+Copyright 2019 The Knative Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at


Also missed linefeed here, and in the header of other new files.

chizhg · 2019-10-08T23:09:50Z

test/conformance/helpers/constants.go

@@ -0,0 +1,48 @@
+/*


Since you define them as functions here, do you think uri.go will be a better file name?

chizhg · 2019-10-08T23:10:14Z

test/conformance/helpers/tracing/traces.go

+//   1. Shorter children first.
+//   2. Span kind.
+//   3. "http.url", "http.host", "http.path" tag presence and values.
+// If all of those are equal, then arbitrarily chose the earlier index.


Suggested change

// If all of those are equal, then arbitrarily chose the earlier index.

// If all of those are equal, then arbitrarily choose the earlier index.

chizhg · 2019-10-08T23:11:22Z

test/conformance/helpers/tracing/traces.go

+// manually.
+//
+// The order it uses:
+//   1. Shorter children first.


Lower children first?

Shorter. Where height is defined as the maximal number of descendants before hitting a leaf node. For example:

A / \ B C | D

'A' has a height of 2. 'B' has a height of 1. 'C' and 'D' have heights of 0.

knative-metrics-robot · 2019-10-09T00:20: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/broker/filter/filter_handler.go	81.8%	81.5%	-0.3

chizhg · 2019-10-09T01:20:56Z

It looks really hard to get this test working, I can imagine how much effort you put on it 👍
/lgtm

Harwayne added 14 commits September 30, 2019 10:58

Copied verbatim from @daisy-ycguo's knative#1878.

097daaf

Trace outline.

f61cc37

Add a hacky sleep so the Broker comes online.

b2a5e32

Merge branch 'master' into broker-trace

f6809cd

Single trace!

56bd430

Single place adds all the outgoing tracing.

d447731

Random

c35f140

TestSpanTrace.Notes.

89d1595

Small Changes.

0e60281

Merge branch 'master' into broker-trace

8e88ecb

In the right direction.

1b1e7c6

Close to working.

0cff4ed

It works!

2710b02

Remove debug logging.

7c934a6

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Oct 3, 2019

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

2 minutes.

bd97735

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

knative-prow-robot requested review from lionelvillard and yt3liu October 3, 2019 23:57

mattmoor-sockpuppet reviewed Oct 3, 2019

View reviewed changes

Remove unneeded addition of the traceparent attribute.

fc7509d

mattmoor-sockpuppet reviewed Oct 4, 2019

View reviewed changes

Harwayne added 2 commits October 3, 2019 17:01

Fix trace name unit test.

e253e96

Merge branch 'master' into broker-trace

a0b6373

mattmoor-sockpuppet reviewed Oct 4, 2019

View reviewed changes

Harwayne commented Oct 4, 2019

View reviewed changes

Improve comments.

16e8104

chizhg reviewed Oct 4, 2019

View reviewed changes

test/common/operation.go Show resolved Hide resolved

Swtich the sleep to waiting for the Broker to be ready.

85cd1b7

Harwayne marked this pull request as ready for review October 4, 2019 19:39

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 Oct 4, 2019

chizhg reviewed Oct 8, 2019

View reviewed changes

daisy-ycguo mentioned this pull request Oct 8, 2019

E2E test for broker tracing #1878

Closed

Harwayne mentioned this pull request Oct 8, 2019

Test Helpers should return the created resources #2011

Closed

PR comments.

99e4368

chizhg reviewed Oct 8, 2019

View reviewed changes

Harwayne added 2 commits October 8, 2019 17:13

Rename to uri.go.

c6b8a34

chose -> choose

fdffd78

knative-prow-robot assigned chizhg Oct 9, 2019

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

knative-prow-robot merged commit aca7b61 into knative:master Oct 9, 2019

Harwayne deleted the broker-trace branch October 9, 2019 15:39

		@@ -252,10 +253,9 @@ func (r *Handler) sendEvent(ctx context.Context, tctx cloudevents.HTTPTransportC
		}

		sendingCTX := utils.ContextFrom(tctx, subscriberURI)

		@@ -91,6 +89,10 @@ func (h Handler) serveHTTP(ctx context.Context, event cloudevents.Event, resp

		start := time.Now()
		sendingCTX := utils.ContextFrom(tctx, h.ChannelURI)

	return fmt.Errorf("waiting for %+v to become ready: %v", metaResource, err)
	return fmt.Errorf("failed waiting for %+v to become ready: %v", metaResource, err)

	Copyright 2019 The Knative Authors
	Copyright 2019 The Knative Authors

	// If all of those are equal, then arbitrarily chose the earlier index.
	// If all of those are equal, then arbitrarily choose the earlier index.

E2E Test for Broker Tracing #1995

E2E Test for Broker Tracing #1995

Conversation

Harwayne commented Oct 3, 2019 • edited Loading

Proposed Changes

knative-prow-robot commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harwayne commented Oct 4, 2019

Harwayne commented Oct 4, 2019

Harwayne commented Oct 4, 2019

Harwayne commented Oct 4, 2019

Harwayne commented Oct 7, 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

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

chizhg Oct 8, 2019 • edited Loading

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 Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chizhg Oct 8, 2019 • edited Loading

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

knative-metrics-robot commented Oct 9, 2019

chizhg commented Oct 9, 2019

Harwayne commented Oct 3, 2019 •

edited

Loading

chizhg Oct 8, 2019 •

edited

Loading

chizhg Oct 8, 2019 •

edited

Loading

chizhg Oct 8, 2019 •

edited

Loading