build: move to new protobuf library #10019

serenibyss · 2021-10-28T20:22:16Z

This PR upgrades Telegraf to use the new google.golang.org/protobuf library over the deprecated github.com/golang/protobuf and abandoned github.com/gogo/protobuf. This matches similar work performed in IDPE, InfluxDB, Chronograf, Kapacitor, and InfluxCLI.

Summaries of major changes:

`jti_openconfig_telemetry` input plugin

The JTI Openconfig plugin was using the golang protobuf, and I simply regenerated the existing .proto files with the new protobuf library.

`stackdriver` input/output plugin

Stackdriver was using the golang protobuf, and was primarily using it for the known types, which exist in the new Protobuf in an API-compatible way, so these were replaced with the new references.

`riemann_listener` input plugin

The Riemann Listener plugin was using the gogo protobuf, and I resolved this by copying the riemann repo's .proto file and generating a new .pb.go file using the new protobuf library.

`cisco_telemetry_mdt` input plugin

The Cisco Telemetry plugin was using a library of pre-generated .pb.go files, which lived under github.com/cisco-ie/nx-telemetry-proto. Instead of depending on this library, which was using the golang protobuf, I instead copied in the .proto files from the original repo and generated new files using the new protobuf library.

`prometheus` serializer plugin

The Prometheus plugin was using the gogo protobuf, however Prometheus generates their .pb.go files with Marshal() and Unmarshal() functions on the proto structs, so Proto was removed from this in favor of directly calling these functions.

The end result of this PR is depending on 3 less libraries (gogo/protobuf, golang/protobuf, and cisco-ie/nx-telemetry-proto), keeping Protobuf consistent across the repo, and avoiding future issues that arise from the deprecated/abandoned repos.

srebhan

Thank you very much @DStrand1 for this PR! It's very much appreciated. My only concern, beside some smaller comments, is moving the protocol buffer files into Telegraf instead of importing them from their upstream location. Can you please either revert this to use the upstream version or elaborate on why this is needed?

plugins/inputs/cisco_telemetry_mdt/cisco_telemetry_mdt.go

srebhan · 2021-11-01T08:37:01Z

plugins/inputs/cisco_telemetry_mdt/cisco_telemetry_mdt.go

+	// Required embed to maintain compatibility
+	dialout.UnimplementedGRPCMdtDialoutServer


Can you elaborate a bit on why it is needed? It was not there previously and I don't see how it is used in the code...

protoc-gen-go-grpc requires that this be embedded in any struct that uses the proto-gRPC generated types, as it is meant to provide easy forward-compatibility if the generator changes, so that implemented server code doesn't break.

More realistically though, there is an inaccessible member function mustEmbedUnimplementedGRPCMdtDialoutServer() created as part of the generated code, that without this embed in the struct, calling dialout.RegisterGRPCMdtDialoutServer() on line 171 would give an error. Since it is inaccessible, embedding this is the best way to resolve the issue, and is recommended in a generated code comment (from dialout_grpc.pb.go#71-73):

// UnimplementedGRPCMdtDialoutServer must be embedded to have forward compatible implementations. type UnimplementedGRPCMdtDialoutServer struct { }

Understood. Can you please summarize this as a comment in the code as otherwise someone will remove this and stuff starts to break. :-)

Can you please add a comment here?

Suggested change

// Required embed to maintain compatibility

dialout.UnimplementedGRPCMdtDialoutServer

// Though unused in the code, required by protoc-gen-go-rpc to maintain (forward-)compatibility

dialout.UnimplementedGRPCMdtDialoutServer

plugins/inputs/cisco_telemetry_mdt/cisco_telemetry_util.go

plugins/inputs/jti_openconfig_telemetry/openconfig_telemetry_test.go

plugins/inputs/riemann_listener/riemann_listener.go

plugins/serializers/prometheusremotewrite/prometheusremotewrite.go

serenibyss · 2021-11-02T19:15:48Z

With regards to copying-in .proto files:

Cisco_telemetry_mdt: The two options for these .proto files are either to copy them in directly and generate our own .pb.go files (which we are doing in this PR), or to use the forked repo which provides generated .pb.go files (which we were doing before). However, those generated files are using one of the older Protobuf libraries, so they cannot be used any longer. The home repo of these proto files, https://github.com/CiscoDevNet/nx-telemetry-proto, has no packages and nothing real to vendor, so pulling these files in seems easiest to me. They are unlikely to change in impactful ways, as that would break compatibility with previous wire format. It may be possible to still grab this package and generate .pb.go files from it similar to below, but I haven't been able to get it working yet.

Riemann_listener: This one I was able to change the go:generate directive to use the vendored .proto file successfully. We may still have to update the generated file if the .proto file changes, but it is as easy as re-running go generate ./...

srebhan · 2021-11-03T15:59:00Z

AFAICS there is an open pull-request with this origin https://github.com/sbezverk/nx-telemetry-proto/tree/mod_update. You could use a replace statement in go.mod to use this version until things get merged. I don't think copying the .proto files or the generated code here will be accepted. @reimda correct me if I'm wrong here.

srebhan

Nice update @DStrand1! I have some more comments and a request to also get rid of the riemann stuff which also fails in CI. Can you please fix this upstream or in a fork of the upstream repo? I really want to avoid to open this box...

plugins/inputs/cisco_telemetry_mdt/cisco_telemetry_mdt.go

srebhan · 2021-11-05T08:09:45Z

plugins/inputs/cisco_telemetry_mdt/cisco_telemetry_mdt.go

+	// Required embed to maintain compatibility
+	dialout.UnimplementedGRPCMdtDialoutServer


Can you please add a comment here?

Suggested change

// Required embed to maintain compatibility

dialout.UnimplementedGRPCMdtDialoutServer

// Though unused in the code, required by protoc-gen-go-rpc to maintain (forward-)compatibility

dialout.UnimplementedGRPCMdtDialoutServer

plugins/inputs/riemann_listener/riemann_listener.go

srebhan · 2021-11-05T08:13:57Z

plugins/inputs/riemann_listener/riemann_listener.go

@@ -204,7 +204,7 @@ func (rsl *riemannListener) read(conn net.Conn) {
 			riemannReturnErrorResponse(conn, "Failed to unmarshal")
 			return
 		}
-		riemannEvents := riemanngo.ProtocolBuffersToEvents(messagePb.Events)
+		riemannEvents := protocolBuffersToEvents(messagePb.Events)


Is there any reason why you need a copy of the received events? Can't you directly iterate on the events?

We could directly iterate over the events, but since we are now using an updated fork of the riemann repo (i.e., not including our own copy of the ProtocolBuffersToEvents() function in Telegraf), I think it is cleaner to call this function in riemanngo instead of handling the logic from there ourselves

- move to forked riemann-client-go lib until PR is merged into main

telegraf-tiger · 2021-11-05T17:30:01Z

⚠️ This pull request increases the Telegraf binary size by 1.12 % for linux amd64 (new size: 132.2 MB, nightly size 130.7 MB)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_armv7.tar.gz
armhf.deb	i386.rpm	freebsd_i386.tar.gz
i386.deb	ppc64le.rpm	linux_amd64.tar.gz
mips.deb	s390x.rpm	linux_arm64.tar.gz
mipsel.deb	x86_64.rpm	linux_armel.tar.gz
ppc64el.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

srebhan

Amazing work @DStrand1! Code looks good to me and I keep my fingers crossed for you to get your PRs accepted upstream.

sspaink · 2021-11-08T22:35:43Z

Thanks for working on this, using the forked repository for now seems like a good idea to me. Just to make sure, @srebhan and @DStrand1 the intention isn't to wait for upstream to merge the changes before merging this pr?

bai · 2021-11-09T08:24:59Z

Not a reviewer but wanted to stop by and say thanks for fantastic work in this PR 🙏🏼

srebhan · 2021-11-09T09:46:01Z

@sspaink exactly. We can get rid of some pretty old libs with this PR and we shouldn't wait for upstream to get out of their holes... ;-)

(cherry picked from commit ddeb6ec)

DStrand1 added 6 commits October 28, 2021 14:58

build: move jti-openconfig-telemetry to new protobuf library

8c8940f

build: move stackdriver to new protobuf library

1093ee6

build: move riemann_listener to new protobuf library

c29139d

build: move cisco_telemetry_mdt to new protobuf library

8a3a896

build: move prometheus serializer to new protobuf library

01d771c

chore: update dependencies

1829310

srebhan reviewed Nov 1, 2021

View reviewed changes

srebhan self-assigned this Nov 1, 2021

srebhan added dependencies Pull requests that update a dependency file feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 1, 2021

chore: address requested changes

f60adbf

serenibyss requested a review from srebhan November 2, 2021 19:16

DStrand1 added 2 commits November 3, 2021 12:07

chore: update to using forked cisco-ie .pb.go files

4a55956

chore: revert removing cisco-ie dependency license

892d4da

srebhan requested changes Nov 5, 2021

View reviewed changes

chore: make requested changes

e12d79c

- move to forked riemann-client-go lib until PR is merged into main

serenibyss requested a review from srebhan November 5, 2021 18:00

srebhan approved these changes Nov 8, 2021

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 8, 2021

sspaink approved these changes Nov 8, 2021

View reviewed changes

sspaink merged commit ddeb6ec into master Nov 9, 2021

sspaink deleted the ds-upgrade-protobuf branch November 9, 2021 14:51

powersj pushed a commit that referenced this pull request Nov 17, 2021

build: move to new protobuf library (#10019)

88f0560

(cherry picked from commit ddeb6ec)

VladislavSenkevich pushed a commit to gwos/telegraf that referenced this pull request Nov 23, 2021

build: move to new protobuf library (influxdata#10019)

6a62ac8

powersj mentioned this pull request Feb 15, 2022

gogo/protobuf is likely to be discontinued #9193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: move to new protobuf library #10019

build: move to new protobuf library #10019

serenibyss commented Oct 28, 2021 •

edited

srebhan left a comment

srebhan Nov 1, 2021

serenibyss Nov 2, 2021

srebhan Nov 3, 2021

srebhan Nov 5, 2021

serenibyss commented Nov 2, 2021

srebhan commented Nov 3, 2021

srebhan left a comment

srebhan Nov 5, 2021

srebhan Nov 5, 2021

serenibyss Nov 5, 2021

telegraf-tiger bot commented Nov 5, 2021

Artifact URLs

srebhan left a comment

sspaink commented Nov 8, 2021

bai commented Nov 9, 2021

srebhan commented Nov 9, 2021

		// Required embed to maintain compatibility
		dialout.UnimplementedGRPCMdtDialoutServer

build: move to new protobuf library #10019

build: move to new protobuf library #10019

Conversation

serenibyss commented Oct 28, 2021 • edited

Summaries of major changes:

jti_openconfig_telemetry input plugin

stackdriver input/output plugin

riemann_listener input plugin

cisco_telemetry_mdt input plugin

prometheus serializer plugin

srebhan left a comment

Choose a reason for hiding this comment

srebhan Nov 1, 2021

Choose a reason for hiding this comment

serenibyss Nov 2, 2021

Choose a reason for hiding this comment

srebhan Nov 3, 2021

Choose a reason for hiding this comment

srebhan Nov 5, 2021

Choose a reason for hiding this comment

serenibyss commented Nov 2, 2021

srebhan commented Nov 3, 2021

srebhan left a comment

Choose a reason for hiding this comment

srebhan Nov 5, 2021

Choose a reason for hiding this comment

srebhan Nov 5, 2021

Choose a reason for hiding this comment

serenibyss Nov 5, 2021

Choose a reason for hiding this comment

telegraf-tiger bot commented Nov 5, 2021

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

sspaink commented Nov 8, 2021

bai commented Nov 9, 2021

srebhan commented Nov 9, 2021

serenibyss commented Oct 28, 2021 •

edited

`jti_openconfig_telemetry` input plugin

`stackdriver` input/output plugin

`riemann_listener` input plugin

`cisco_telemetry_mdt` input plugin

`prometheus` serializer plugin