Update protobuf from v1.3.5 to v1.4.2 and opencensus-proto from v0.2.1 to v0.3.0 #1308

nilebox · 2020-07-09T06:25:52Z

Description:
Updating Prometheus to v2.19.2 requires updating Protobuf, so I extracted this change to merge it separately.

Link to tracking Issue: #1124

nilebox · 2020-07-09T06:27:11Z

.golangci.yml

+    # Exclude some staticcheck messages
+    - linters:
+      - staticcheck
+      # See https://github.com/golang/protobuf/issues/1077


Protobuf v1.4.2 has // Deprecated comment in
https://github.com/golang/protobuf/blob/v1.4.2/proto/proto.go#L12
which was introduced in https://go-review.googlesource.com/c/protobuf/+/213901

The change below completely disables the following check:

SA1019 | Using a deprecated function, variable, constant or field

Is there any better workaround?

AFAIU migrating to the package google.golang.org/protobuf/proto will require re-generating protobuf types, some of which live outside of this repo (e.g. Zipkin, OpenCensus)

I think you can change OpenCensus. We need to also see if Zipkin has a new release which uses the new library

Is it possible to do a line based ignore on the import statement? https://staticcheck.io/docs/#line-based-linter-directives

Do we still need this?

I've reverted the blanket ignore rule in the STATICCHECK Makefile rule, but left this one.
It seems that for some reason we run staticcheck twice and they conflict with each other when I try to add ignore rules:

We have a separate target for staticcheck:

opentelemetry-collector/Makefile

Line 111 in c4a97db

.PHONY: lint-static-check

. To ignore errors which can't be trivially fixed, I added //lint:ignore SA1019

We also run staticcheck as part of golangci-lint, see

opentelemetry-collector/.golangci.yml

Line 100 in c4a97db

- staticcheck

. It requires //nolint:staticcheck comment which conflicts with the one above (only left-most comment disables linter)

@bogdandrutu Why do we run staticcheck twice?
For now I kept the SA1019 check in the lint-static-check, but excluded it from golangci-lint.
We can deduplicate running staticcheck in a separate PR if needed.

codecov · 2020-07-09T06:35:54Z

Codecov Report

Merging #1308 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1308      +/-   ##
==========================================
+ Coverage   90.23%   90.24%   +0.01%     
==========================================
  Files         233      233              
  Lines       16396    16396              
==========================================
+ Hits        14795    14797       +2     
+ Misses       1141     1140       -1     
+ Partials      460      459       -1

Impacted Files	Coverage Δ
exporter/fileexporter/file_exporter.go	`68.64% <ø> (ø)`
internal/data/metric.go	`100.00% <ø> (ø)`
processor/cloningfanoutconnector.go	`69.44% <ø> (ø)`
...eceiver/opencensusreceiver/ocmetrics/opencensus.go	`90.47% <ø> (ø)`
receiver/opencensusreceiver/octrace/opencensus.go	`83.33% <ø> (ø)`
consumer/pdatautil/pdatautil.go	`84.61% <100.00%> (ø)`
receiver/otlpreceiver/otlp.go	`93.65% <0.00%> (+3.17%)`	⬆️

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 670245c...df1b3c9. Read the comment docs.

tigrannajaryan · 2020-07-09T15:57:59Z

I remember last time we tried to update protobuf we saw a performance degradation. Please confirm that this does not cause changes in the performance (best to post before/after numbers from testbed execution performed on the same machine).

nilebox · 2020-07-16T06:09:28Z

@tigrannajaryan there seems to be a significant performance degradation in OpenCensus test cases, while OTLP seems fine, and Zipkin seems to actually perform even better.

nilebox · 2020-07-16T07:12:01Z

The newMarshaler interface was removed in golang/protobuf@cc376d7 (https://go-review.googlesource.com/c/protobuf/+/213901), which supposedly led to the performance degradation we see.

nilebox · 2020-07-16T07:17:30Z

I will try to migrate the auto-generated OpenCensus Protobuf code to v1.4.2, which will allow us to migrate to the google.golang.org/protobuf/proto package and test if that fixes the performance issue.

nilebox · 2020-07-16T07:19:00Z

@tigrannajaryan @bogdandrutu Supposedly, the OTLP's performance was not affected because it's using https://github.com/gogo/protobuf ?

nilebox · 2020-07-16T23:17:44Z

Just tested with an experimental branch of opencensus-proto with types generated using the new protoc, protoc-gen-go and protoc-gen-go-grpc, and it seems to fix the performance issue.

bogdandrutu · 2020-07-17T01:30:39Z

@nilebox you definitely have a time travel machine :)) Started: Fri, 17 Jul 2020 09:06:22 +1000.

What is the next step? Wait for opencensus-proto to release?

nilebox · 2020-07-17T02:27:02Z

@bogdandrutu Yes, I will need to release opencensus-proto with the new generated types, which should unblock this PR.

go.mod

lizthegrey

Looks good. Please also update the description to indicate you changed opencensus-proto versions.

Also please add to CHANGELOG.md

nilebox · 2020-07-22T04:22:44Z

Added changelog.

nilebox · 2020-07-22T04:47:10Z

@tigrannajaryan please see updated testbed results above.
There doesn't seem to be a noticeable difference anymore, so should be good to merge?

tigrannajaryan · 2020-07-22T15:22:15Z

go.sum

 github.com/golang/protobuf v1.3.5/go.mod h1:6O5/vntMXwX2lRkT1hjjk0nAC1IDOTvTlVgjlRvqsdk=
+github.com/golang/protobuf v1.4.0-rc.1/go.mod h1:ceaxUfeHdC40wWswd/P6IGgMaK3YpKi5j83Wpe3EHw8=


Please run go mod tidy (if not already).

tigrannajaryan · 2020-07-22T15:27:45Z

@tigrannajaryan please see updated testbed results above.
There doesn't seem to be a noticeable difference anymore, so should be good to merge?

Yes, I cannot see any significant difference (I also checked the CircleCI run - the differences are below our measurement error).

tigrannajaryan · 2020-07-22T15:43:31Z

I am OK with merging this once the open comments are addressed/resolved.

bogdandrutu · 2020-07-22T23:05:13Z

Would prefer to not disable the check for the entire repo if possible

nilebox

@bogdandrutu I have cleaned up the ignore rules, and migrated to new protobuf package where it was trivial.
Also see comment about running staticcheck twice for some reason.

nilebox · 2020-07-23T00:34:10Z

consumer/pdata/metric_test.go

@@ -18,7 +18,7 @@ import (
 	"testing"

 	gogoproto "github.com/gogo/protobuf/proto"
-	goproto "github.com/golang/protobuf/proto"
+	goproto "github.com/golang/protobuf/proto" //lint:ignore SA1019 golang/protobuf/proto is deprecated


Can't replace it with "google.golang.org/protobuf/proto" because the OTLP types generated by gogo/protobuf don't implement ProtoReflect() function of ProtoMessage interface:

https://pkg.go.dev/google.golang.org/protobuf@v1.25.0/proto?tab=doc#Marshal

https://pkg.go.dev/google.golang.org/protobuf/reflect/protoreflect?tab=doc#ProtoMessage

nilebox · 2020-07-23T00:34:26Z

consumer/pdata/trace_test.go

@@ -18,7 +18,7 @@ import (
 	"testing"

 	gogoproto "github.com/gogo/protobuf/proto"
-	goproto "github.com/golang/protobuf/proto"
+	goproto "github.com/golang/protobuf/proto" //lint:ignore SA1019 golang/protobuf/proto is deprecated


Can't replace it with "google.golang.org/protobuf/proto" because the OTLP types generated by gogo/protobuf don't implement ProtoReflect() function of ProtoMessage interface:

https://pkg.go.dev/google.golang.org/protobuf@v1.25.0/proto?tab=doc#Marshal

https://pkg.go.dev/google.golang.org/protobuf/reflect/protoreflect?tab=doc#ProtoMessage

nilebox · 2020-07-23T00:34:53Z

exporter/fileexporter/file_exporter.go

-	"github.com/golang/protobuf/jsonpb"
-	"github.com/golang/protobuf/proto"
+	"github.com/golang/protobuf/jsonpb" //lint:ignore SA1019 golang/protobuf/jsonpb is deprecated
+	"github.com/golang/protobuf/proto"  //lint:ignore SA1019 golang/protobuf/proto is deprecated


Can't replace it with "google.golang.org/protobuf/proto" because the OTLP types generated by gogo/protobuf don't implement ProtoReflect() function of ProtoMessage interface:

https://pkg.go.dev/google.golang.org/protobuf/reflect/protoreflect?tab=doc#ProtoMessage

nilebox · 2020-07-23T00:36:28Z

receiver/zipkinreceiver/proto_parse_test.go

@@ -21,10 +21,11 @@ import (

 	commonpb "github.com/census-instrumentation/opencensus-proto/gen-go/agent/common/v1"
 	tracepb "github.com/census-instrumentation/opencensus-proto/gen-go/trace/v1"
-	"github.com/golang/protobuf/proto"
+	"github.com/golang/protobuf/proto" //lint:ignore SA1019 golang/protobuf/proto is deprecated


Zipkin still has types generated by older version of protoc-gen-go, and doesn't implement new ProtoMessage interface. The release we are using is the latest.

…RPC)

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* bump ci version * oops * oop * Change min version * use an input

nilebox requested review from bogdandrutu, dmitryax, flands, james-bebbington, owais, pjanotti, rghetia and tigrannajaryan as code owners July 9, 2020 06:25

nilebox mentioned this pull request Jul 9, 2020

Update Prometheus to v2.19.2 #1301

Merged

nilebox commented Jul 9, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

nilebox mentioned this pull request Jul 17, 2020

Generate Go types using protocolbuffers/protobuf-go census-instrumentation/opencensus-proto#218

Merged

This was referenced Jul 17, 2020

update Honeycomb Exporter and its dependencies open-telemetry/opentelemetry-collector-contrib#454

Merged

Exercise caution before upgrading protobuf honeycombio/libhoney-go#88

Closed

Bump github.com/golang/protobuf from 1.3.5 to 1.4.2 #1393

Closed

nilebox commented Jul 21, 2020

View reviewed changes

go.mod Show resolved Hide resolved

lizthegrey approved these changes Jul 21, 2020

View reviewed changes

lizthegrey mentioned this pull request Jul 21, 2020

Bump go.opencensus.io from 0.22.3 to 0.22.4 #1395

Merged

nilebox changed the title ~~Update protobuf from v1.3.5 to v1.4.2~~ Update protobuf from v1.3.5 to v1.4.2 and opencensus-proto from v0.2.1 to v0.3.0 Jul 21, 2020

This comment has been minimized.

Sign in to view

james-bebbington approved these changes Jul 22, 2020

View reviewed changes

lizthegrey approved these changes Jul 22, 2020

View reviewed changes

jrcamp assigned tigrannajaryan Jul 22, 2020

tigrannajaryan reviewed Jul 22, 2020

View reviewed changes

bogdandrutu added the waiting-for-author label Jul 22, 2020

nilebox commented Jul 23, 2020

View reviewed changes

Nail Islamov added 8 commits July 23, 2020 11:18

Update protobuf to v1.4.2

c230dac

Disable SA1019 linter check (requires using different protobuf packages)

4757e9e

Update to opencensus-proto v0.3.0

ce3602c

Embed unimplemented servers in OpenCensus receiver (required by new g…

cf24d61

…RPC)

Switch to go-cmp for comparing protobuf messages

8ebdb50

Document changes in the changelog

7d69019

Apply ignore lint rules to individual lines, fix trivial issues

debc6fc

go mod tidy

df1b3c9

tigrannajaryan mentioned this pull request Jul 23, 2020

Move stackdriver trace exporter to new interface and pdata open-telemetry/opentelemetry-collector-contrib#486

Merged

bogdandrutu approved these changes Jul 23, 2020

View reviewed changes

bogdandrutu merged commit be4d0cf into open-telemetry:master Jul 23, 2020

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

Fix zipkin example module name (open-telemetry#1308)

c9ae670

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024

Bump version for CI helm (open-telemetry#1308)

33cd258

* bump ci version * oops * oop * Change min version * use an input

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update protobuf from v1.3.5 to v1.4.2 and opencensus-proto from v0.2.1 to v0.3.0 #1308

Update protobuf from v1.3.5 to v1.4.2 and opencensus-proto from v0.2.1 to v0.3.0 #1308

nilebox commented Jul 9, 2020

nilebox Jul 9, 2020 •

edited

Loading

nilebox Jul 9, 2020 •

edited

Loading

bogdandrutu Jul 9, 2020 •

edited

Loading

james-bebbington Jul 22, 2020

bogdandrutu Jul 22, 2020

nilebox Jul 23, 2020

codecov bot commented Jul 9, 2020 •

edited

Loading

tigrannajaryan commented Jul 9, 2020

This comment has been minimized.

This comment has been minimized.

nilebox commented Jul 16, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

nilebox commented Jul 16, 2020

nilebox commented Jul 16, 2020 •

edited

Loading

nilebox commented Jul 16, 2020

nilebox commented Jul 16, 2020

This comment has been minimized.

bogdandrutu commented Jul 17, 2020

nilebox commented Jul 17, 2020

lizthegrey left a comment

nilebox commented Jul 22, 2020

This comment has been minimized.

nilebox commented Jul 22, 2020

tigrannajaryan Jul 22, 2020

tigrannajaryan commented Jul 22, 2020

tigrannajaryan commented Jul 22, 2020

bogdandrutu commented Jul 22, 2020

nilebox left a comment

nilebox Jul 23, 2020

nilebox Jul 23, 2020

nilebox Jul 23, 2020

nilebox Jul 23, 2020

		github.com/golang/protobuf v1.3.5/go.mod h1:6O5/vntMXwX2lRkT1hjjk0nAC1IDOTvTlVgjlRvqsdk=
		github.com/golang/protobuf v1.4.0-rc.1/go.mod h1:ceaxUfeHdC40wWswd/P6IGgMaK3YpKi5j83Wpe3EHw8=

Update protobuf from v1.3.5 to v1.4.2 and opencensus-proto from v0.2.1 to v0.3.0 #1308

Update protobuf from v1.3.5 to v1.4.2 and opencensus-proto from v0.2.1 to v0.3.0 #1308

Conversation

nilebox commented Jul 9, 2020

nilebox Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

nilebox Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

bogdandrutu Jul 9, 2020 • 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

codecov bot commented Jul 9, 2020 • edited Loading

Codecov Report

tigrannajaryan commented Jul 9, 2020

This comment has been minimized.

This comment has been minimized.

nilebox commented Jul 16, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

nilebox commented Jul 16, 2020

nilebox commented Jul 16, 2020 • edited Loading

nilebox commented Jul 16, 2020

nilebox commented Jul 16, 2020

This comment has been minimized.

bogdandrutu commented Jul 17, 2020

nilebox commented Jul 17, 2020

lizthegrey left a comment

Choose a reason for hiding this comment

nilebox commented Jul 22, 2020

This comment has been minimized.

nilebox commented Jul 22, 2020

Choose a reason for hiding this comment

tigrannajaryan commented Jul 22, 2020

tigrannajaryan commented Jul 22, 2020

bogdandrutu commented Jul 22, 2020

nilebox left a 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

nilebox Jul 9, 2020 •

edited

Loading

nilebox Jul 9, 2020 •

edited

Loading

bogdandrutu Jul 9, 2020 •

edited

Loading

codecov bot commented Jul 9, 2020 •

edited

Loading

nilebox commented Jul 16, 2020 •

edited

Loading

nilebox commented Jul 16, 2020 •

edited

Loading