Update to opencensus v0.22 #893
Conversation
Sync from googleforgames/agones
Sync from repo
|
Build Failed 😱 Build Id: 7f248d53-bcaa-4d3b-bc86-deea5f3fa61f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😱 Build Id: 220b0c06-c9e1-4512-aaae-2d01aa064025 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
/cc @Kuqd I figure you may want to review this? 😄 |
|
Build Succeeded 👏 Build Id: 33fd90f4-7de1-4486-9042-dbfa0557df4e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 4d508ada-8bf9-40b9-8377-79bf3601944c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 615a36e5-d990-4c04-8133-951fcd206102 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Failed 😱 Build Id: afdddd7f-532a-4f89-97f2-b9eb98968f08 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 👏 Build Id: b502571c-c4be-4700-ab1d-5daedab561d9 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: 2d3e42f7-9085-4618-95d6-8fcf22372fea The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
I'll take a look this week. |
|
Build Failed 😱 Build Id: c373a647-a86f-436b-8c84-fd5bed4f722f To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
| google.golang.org/genproto v0.0.0-20190111180523-db91494dd46c | ||
| google.golang.org/grpc v1.17.0 | ||
| google.golang.org/genproto v0.0.0-20190425155659-357c62f0e4bb | ||
| google.golang.org/grpc v1.20.1 |
There was a problem hiding this comment.
grpc bump and protobuf bump. @markmandel is that ok ? does it requires tooling update ?
There was a problem hiding this comment.
Good catch. That's a big jump. If we do this, yes - we should regenerate things, and confirm
Do we need this update to update opencensus as well? Are they linked dependencies?
I'm almost wondering if this should be split into some separate PRs to make this easier to review?
- The update to OpenCensus
- The updates to the test code
- Anything else?
Would that be possible?
Chrome actually crashes on this page 😞 I should fire up Firefox, it usually works.
I'll also dig through this this week too.
There was a problem hiding this comment.
I suspect they are linked dependencies. The only change I had made was to upgrade to 0.22. The rest came from running go mod vendor and go mod tidy.
I cannot split the test changes. One consequence of moving to 0.22 is that @Kuqd 's trick of unregistering/registering to publish metrics in tests no longer works reliably.
There was a problem hiding this comment.
I would like to have first a grpc/protobuf bump to the version you need.
There was a problem hiding this comment.
That seems like a good strategy to me 👍 grpc is annoyingly backward compatible, so i expect nothing will break even if we update only one side.
There was a problem hiding this comment.
FWIW, if grpc or genproto are not backwards compatible than a lot of things would break. Both libraries need to maintain backwards compatibility to be useful since clients and servers cannot be updated at the same time.
There was a problem hiding this comment.
So I would agree with @Kuqd - I think the right step would be:
- Update gRPC to the version required here (one PR)
- Then (another PR) one of us can regenerate the SDK toolchain, and make sure that all work appropriately.
- Then we can apply the opencensus update and changes (another PR)
That should make it a nicer, and safer process. WDYT?
|
This include a fix for Prometheus tags, which was happening in my branch, @pjayara-g it would be great to also remove any tag value |
|
Build Succeeded 👏 Build Id: 4fb137a4-290a-4671-88e7-5c6dc7235781 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
| val interface{} | ||
| } | ||
|
|
||
| func verifyMetricData(exporter *metricExporter, metricName string, expected []expectedMetricData) error { |
There was a problem hiding this comment.
this could be:
func assertMetricData(t *test.T, exporter *metricExporter, metricName string, expected []expectedMetricData) bool
And then you could use the assert library all the way through, and wouldn't need to use reflect, and if there is a difference between deep structs, you will get nicer reporting out of the test. WDYT?
| {labels: []string{"test-fleet", "Shutdown"}, val: int64(1)}, | ||
| {labels: []string{"none", "PortAllocation"}, val: int64(2)}, | ||
| }) | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
If you keep returning an error, assert.NoError is better, since if there is an error, it returns the error's message.
The changes updates the version to v0.22. It also uses the newish ReadAndExport method in opencensus instead of using the prometheus exporter for validation in unit tests. Addresses #892.