Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Upgrade oc-proto version and fix errors.#102

Merged
songy23 merged 6 commits intocensus-ecosystem:masterfrom
songy23:fix-oc-proto
Mar 13, 2019
Merged

Upgrade oc-proto version and fix errors.#102
songy23 merged 6 commits intocensus-ecosystem:masterfrom
songy23:fix-oc-proto

Conversation

@songy23
Copy link
Copy Markdown
Contributor

@songy23 songy23 commented Mar 13, 2019

Fixes #101.

@songy23 songy23 added the bug Something isn't working label Mar 13, 2019
@songy23 songy23 requested review from odeke-em and rghetia March 13, 2019 03:21
odeke-em
odeke-em approved these changes Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and thank you @songy23!

@songy23 songy23 mentioned this pull request Mar 13, 2019
Merged
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 13, 2019

Seems this also depends on census-ecosystem/opencensus-go-exporter-ocagent#48.

bogdandrutu
bogdandrutu approved these changes Mar 13, 2019
View reviewed changes
metrics.go
@@ -363,7 +363,7 @@ func labelDescriptorsFromProto(defaults map[string]labelValue, protoLabelKeys []
}

func metricProseFromProto(metric *metricspb.Metric) (name, description, unit string, ok bool) {
Copy link
Copy Markdown
Contributor

@bogdandrutu bogdandrutu Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function name looks like a typo

Copy link
Copy Markdown
Contributor

@odeke-em odeke-em Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, prose https://www.merriam-webster.com/dictionary/prose

@odeke-em odeke-em mentioned this pull request Mar 13, 2019
Merged
@siriele
Copy link
Copy Markdown

siriele commented Mar 13, 2019

Is this change going to go live any time soon? It's currently breaking our builds

@bogdandrutu
Copy link
Copy Markdown
Contributor

bogdandrutu commented Mar 13, 2019

Hope so, @songy23 are you going to fix this?

@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 13, 2019

Yes, as soon as census-ecosystem/opencensus-go-exporter-ocagent#48 is submitted I should be able to submit this PR.

Meanwhile a quick fix is to pin a released version of opencensus-go-exporter-stackdriver and use v0.1.0 for opencensus-proto in go.mod.

@odeke-em
Copy link
Copy Markdown
Contributor

odeke-em commented Mar 13, 2019

@songy23 please update ocagent-exporter to https://github.com/census-ecosystem/opencensus-go-exporter-ocagent/releases/tag/v0.4.7 in here, do the GO111MODULE=on go mod tidy dance afterwards too.

odeke-em
odeke-em reviewed Mar 13, 2019
View reviewed changes
go.sum
@@ -55,6 +56,7 @@ github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1 h1:EGx4pi6eqNxGa
github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
github.com/grpc-ecosystem/grpc-gateway v1.5.0/go.mod h1:RSKVYQBd5MCa4OVpNdGskqpgL2+G+NZTnrVHpWWfpdw=
Copy link
Copy Markdown
Contributor

@odeke-em odeke-em Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you remember to run GO111MODULE=on go mod tidy? This change should have disappeared.

Copy link
Copy Markdown
Contributor Author

@songy23 songy23 Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I guess there're other dependencies that depend on it?

@odeke-em
Copy link
Copy Markdown
Contributor

odeke-em commented Mar 13, 2019 •
edited
Loading

@songy23 just a few more

[contrib.go.opencensus.io/exporter/stackdriver.test]
./metrics_test.go:101:5: cannot use promoted field Descriptor in struct literal of type "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1".Metric
./metrics_test.go:234:5: cannot use promoted field Descriptor in struct literal of type "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1".Metric
./metrics_test.go:247:5: cannot use promoted field Descriptor in struct literal of type "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1".Metric
The command "go vet ./..." exited with 2.
$ go test -v -race $PKGS
contrib.go.opencensus.io/exporter/stackdriver [contrib.go.opencensus.io/exporter/stackdriver.test]
./metrics_test.go:101:5: cannot use promoted field Descriptor in struct literal of type "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1".Metric
./metrics_test.go:234:5: cannot use promoted field Descriptor in struct literal of type "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1".Metric
./metrics_test.go:247:5: cannot use promoted field Descriptor in struct literal of type "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1".Metric

keep going champ!

@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 13, 2019

@songy23 just a few more

Thanks, that was fixed. I'll submit it once CI is green.

A price paid for making breaking changes...

@songy23 songy23 merged commit ab5a58a into census-ecosystem:master Mar 13, 2019
@songy23 songy23 deleted the fix-oc-proto branch March 13, 2019 06:40
@grantr grantr mentioned this pull request Apr 24, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@odeke-em odeke-em odeke-em approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@rghetia rghetia rghetia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@songy23 @siriele @bogdandrutu @odeke-em @rghetia @googlebot