Move e2e tests for metrics export to integration status to reduce crosstalk flakiness by evankanderson · Pull Request #1782 · knative/pkg

evankanderson · 2020-10-07T04:49:02Z

Maybe fixes #1672

Creating a PR to see what I need to change in Prow

/hold

…sstalk flakiness

knative-prow-robot · 2020-10-07T04:49:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~metrics/OWNERS~~ [evankanderson]
~~test/OWNERS~~ [evankanderson]

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

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

codecov · 2020-10-07T05:06:27Z

Codecov Report

Merging #1782 into master will decrease coverage by 0.11%.
The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           master    #1782      +/-   ##
==========================================
- Coverage   68.22%   68.11%   -0.12%     
==========================================
  Files         222      222              
  Lines        9437     9442       +5     
==========================================
- Hits         6438     6431       -7     
- Misses       2678     2692      +14     
+ Partials      321      319       -2

Impacted Files	Coverage Δ
metrics/opencensus_exporter.go	`51.16% <ø> (-14.75%)`	⬇️
metrics/config.go	`80.48% <77.77%> (-0.97%)`	⬇️
metrics/exporter.go	`79.38% <85.71%> (+1.12%)`	⬆️
metrics/resource_view.go	`87.73% <85.71%> (+0.93%)`	⬆️
metrics/stackdriver_exporter.go	`78.84% <0.00%> (-2.57%)`	⬇️
test/gcs/mock/mock.go	`91.39% <0.00%> (-1.08%)`	⬇️
reconciler/events.go	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f492d08...c7d694f. Read the comment docs.

evankanderson · 2020-10-12T20:46:13Z

/test pull-knative-pkg-integration-tests

Fix one bug where prometheus wouldn't work if only the port changed. Fix timeout to work correctly in the record_e2e_test. Fix one bug in the record_e2e_test when multiple measures came in for the same metric.

evankanderson · 2020-10-21T22:14:14Z

Looking into the new segv...

evankanderson · 2020-10-22T17:19:59Z

$ (base) evana-a01:pkg evana$ go test ./metrics -run '^TestS[d]'
ok  	knative.dev/pkg/metrics	0.055s
$ go test ./metrics/... -run '^TestS[e]'
ok  	knative.dev/pkg/metrics	0.049s
$ go test ./metrics -run '^TestS[d-e]'
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x236ba92]

goroutine 6 [running]:
knative.dev/pkg/metrics.(*testExporter).ExportView(0xc00000efa0, 0xc0000be0f0)
	<autogenerated>:1 +0x32
go.opencensus.io/stats/view.(*worker).reportView(0xc00017e000, 0xc00000f400)
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker.go:357 +0x252
go.opencensus.io/stats/view.(*unregisterFromViewReq).handleCommand(0xc0001d87a0, 0xc00017e000)
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker_commands.go:98 +0xe7
go.opencensus.io/stats/view.(*worker).start(0xc00017e000)
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker.go:278 +0x1bb
created by go.opencensus.io/stats/view.init.0
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker.go:34 +0x68
FAIL	knative.dev/pkg/metrics	0.049s
FAIL

So there's something that changes test execution order here that breaks existing tests.

evankanderson · 2020-10-22T17:25:55Z

go test ./metrics -run '^(TestSdRecord|TestSetFactory)'
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x236ba92]

goroutine 19 [running]:
knative.dev/pkg/metrics.(*testExporter).ExportView(0xc000801100, 0xc0000b6690)
	<autogenerated>:1 +0x32
go.opencensus.io/stats/view.(*worker).reportView(0xc00017a000, 0xc000801460)
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker.go:357 +0x252
go.opencensus.io/stats/view.(*unregisterFromViewReq).handleCommand(0xc0008019e0, 0xc00017a000)
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker_commands.go:98 +0xe7
go.opencensus.io/stats/view.(*worker).start(0xc00017a000)
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker.go:278 +0x1bb
created by go.opencensus.io/stats/view.init.0
	/Users/evana/go/src/knative.dev/pkg/vendor/go.opencensus.io/stats/view/worker.go:34 +0x68
FAIL	knative.dev/pkg/metrics	0.048s
FAIL

vagababov · 2020-10-22T17:29:57Z

Something about how we setup globals?

evankanderson · 2020-10-23T01:23:10Z

Yeah, I think I've figured it out finally and may have reduced flakiness in TestMetricsExport, too.

vagababov · 2020-10-23T03:06:56Z

+	if newConfig.backendDestination == prometheus {
+		return newConfig.prometheusPort != cc.prometheusPort
+	}
+
 	return newConfig.backendDestination == stackdriver && newConfig.stackdriverClientConfig != cc.stackdriverClientConfig
 }


return dest=promo && nc.pp != cc.pp || des == stack && nc.sCC != cc.sCC?

I might make this a switch, I worry about having a conditional with 4 boolean expressions being hard to maintain.

…register(nil), but not to Register(nil).

lgtm-com · 2020-10-23T16:58:53Z

This pull request introduces 1 alert when merging 67c9600 into 22e4e0a - view on LGTM.com

new alerts:

1 for Expression has no effect

knative-prow-robot · 2020-10-31T23:50:08Z

@evankanderson: PR needs rebase.

Details

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.

evankanderson · 2021-01-12T00:57:42Z

/close

Fixed this with #1957

knative-prow-robot · 2021-01-12T00:57:49Z

@evankanderson: Closed this PR.

Details

In response to this:

/close

Fixed this with #1957

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.

Move e2e tests for metrics export to integration status to reduce cro…

fe41fe3

…sstalk flakiness

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

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 7, 2020

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 7, 2020

knative-prow-robot requested review from chaodaiG and coryrc October 7, 2020 04:49

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release labels Oct 7, 2020

mattmoor reviewed Oct 7, 2020

View reviewed changes

Fix gofmt

21c185f

vagababov reviewed Oct 7, 2020

View reviewed changes

Evan Anderson added 5 commits October 12, 2020 14:18

Add e2e script for pkg.

4470e5c

Mark test executable

73fede0

Add common library for e2e tests

25cf33e

Don't need serving's common library at all

60c3c49

Address Victor's feedback.

e7d5ebe

Fix one bug where prometheus wouldn't work if only the port changed. Fix timeout to work correctly in the record_e2e_test. Fix one bug in the record_e2e_test when multiple measures came in for the same metric.

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

Merge remote-tracking branch 'upstream/master' into metrics-e2e

a03b609

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

Merge remote-tracking branch 'upstream/master' into metrics-e2e

b2f9091

vagababov reviewed Oct 23, 2020

View reviewed changes

Clean up the factory/exporter registration lifecycle. It's fine to Un…

211b2be

…register(nil), but not to Register(nil).

Clean up isNewExporterRequired

c7d694f

evankanderson force-pushed the metrics-e2e branch from 67c9600 to c7d694f Compare October 23, 2020 16:59

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

knative-prow-robot closed this Jan 12, 2021

Conversation

evankanderson commented Oct 7, 2020

Uh oh!

knative-prow-robot commented Oct 7, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evankanderson commented Oct 12, 2020

Uh oh!

evankanderson commented Oct 21, 2020

Uh oh!

evankanderson commented Oct 22, 2020

Uh oh!

evankanderson commented Oct 22, 2020

Uh oh!

vagababov commented Oct 22, 2020

Uh oh!

evankanderson commented Oct 23, 2020

Uh oh!

vagababov Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

evankanderson Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

lgtm-com Bot commented Oct 23, 2020

Uh oh!

knative-prow-robot commented Oct 31, 2020

Uh oh!

evankanderson commented Jan 12, 2021

Uh oh!

knative-prow-robot commented Jan 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Oct 7, 2020 •

edited

Loading