Update generated proto code to latest protoc

[anuraaga]

From what I can tell, this is required for binaries that use google.golang.org/protobuf. I think it means users of this package will have to use it 1.20.x+ or 1.4.x of github.com/golang/protobuf. Not sure if this constitutes a breaking change, let me know :)

Adds go_package to the proto file which is required in latest protoc versions. This means the package name changed to be the same as the folder name.

Fixes #164

[coveralls]

Coverage remained the same at 60.568% when pulling 60a2d9e on anuraaga:proto-1.4.2 into ef1b344 on openzipkin:master.

[coveralls]

Coverage remained the same at 60.568% when pulling 4478b86 on anuraaga:proto-1.4.2 into ef1b344 on openzipkin:master.

[jcchavezs]

While package name is not too important outside the scope of this codebase I am curious about the reason for changing the name (I guess this is autogenerated and the name comes from the folder name). In any case we might need to change the description in line 16: https://github.com/openzipkin/zipkin-go/pull/165/files#diff-aed4b0e2183470649d6be446904a6842R16

[anuraaga]

Yeah now protoc requires a go_package in the proto file, and the code is automatically generated into the folder with the same package name so this changed. I updated the description, but let me know if there could be any other issues.

[jcchavezs]

LGTM unless @basvanbeek has any objection.

[basvanbeek]

@anuraaga I'm good with the change. @odeke-em do you see issues with the package rename as you are the one who added this logic?

[odeke-em]

@anuraaga I'm good with the change. @odeke-em do you see issues with the package rename as you are the one who added this logic?

I'd instead just rename that directory from v2 to zipkin_proto3 instead of to v2, because v2 is going to cause a whole lot of confusion with Zipkin's protocol v2, yet this is Zipkin proto3. The package's real name is zipkin_proto3 anyways https://pkg.go.dev/github.com/openzipkin/zipkin-go@v0.2.2/proto/v2?tab=doc and even https://github.com/census-instrumentation/opencensus-service/blob/c54ee82d39581d32338d3bc1400ec371fd26ae96/receiver/zipkinreceiver/proto_parse_test.go#L24

[anuraaga]

If I rename the folder, it will be a breaking change though won't it? For example, I don't think the linked census file will compile anymore. Is that worth it?

[odeke-em]

The benefits of the folder rename are higher than the benefits of the package rename IMHO: * Package rename to v2 Pros: + no need to change imports Cons: - Confusion between Zipkin v2 which is unrelated, yet it is Zipkin proto3 we are aiming to make a package for Zipkin proto3 - The package name becomes v2: not a valid default for Go packages— even new users have to now go type zipkinproto3 explicitly for a package named v3 in a folder proto/v2 living inside zipkin/v2 - It doesn’t match the protobuf defintion: it was built for proto3 not proto2 so proto/v2 is also misleading * Folder rename pros: + explicitness matching the import path + no confusion between Zipkin v2 and Zipkin proto3 + matches the purpose of proto3 Cons: + Users have to single change the import path— but we publish Go modules so this is a trivial change to handle

…

On Wed, Jul 29, 2020 at 6:39 PM Anuraag Agrawal ***@***.***> wrote: If I rename the folder, it will be a breaking change though won't it? For example, I don't think the linked census file will compile anymore. Is that worth it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#165 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V7QXNBPIRZZGAPU7MTR6DFLPANCNFSM4PKC4R2A> .

[anuraaga]

I personally didn't find the current folder confusing - it's the proto for Zipkin API V2. If there were a future Zipkin API V3, adding a v3 folder would be very natural.

Proto3 is more an implementation detail of the compiler and not as interesting - can't say I've seen it used much in other package names. Zipkin probably also should have been named just zipkin.proto but it's too late to change that - I don't think the go package is required to match it though if there is something more idiomatic - in go I guess it's not common to repeat the repo name in a package name.

But I just noticed zipkin-go isn't 1.x yet so a folder rename would be totally fine too. Let's get a bit more input on what seems easiest to manage.

[basvanbeek]

@anuraaga I'm on board with @odeke-em's reasoning. Please change package path to zipkin_proto3 to avoid confusion.

This package is mostly consumed by packages under his control so we can expect minimal disruption.

[anuraaga]

Ok renamed the package but please let me have my social commentary here (don't mean to be snarky but I'm confident in a deep understanding of protobuf, much more than I may wish actually :P)

[codefromthecrypt]

freedom of expression allowed :)

[basvanbeek]

@anuraaga merge at your convenience