protoc-gen-go-grpc support split single proto file gen to diffrent package

[qianlongzt]

Use case(s) - what problem will this feature solve?

opentelemetry-go1 supports protobuf over gRPC and over HTTP, but its proto file defines both the service and related messages in the same file.
When using protobuf over HTTP, it's necessary to remove the gRPC dependency to reduce the binary size.

Proposed Solution

Add an option to rename grpc file.GoImportPath 5

-	g := gen.NewGeneratedFile(filename, file.GoImportPath)
+	path := file.GoImportPath
+ 	if n, ok := rename[file.Desc.Path()]; ok {
+ 		path = n
+	}
+	g := gen.NewGeneratedFile(filename, path)

-grpc_rename log.proto=github.com/aaa/bbb/xxx -grpc_rename trace.proto=github.com/aaa/bbb/yyy

Alternatives Considered

1. opentelemetry-proto split proto file
2. opentelemetry-proto-go add same shim file Feat split grpc again open-telemetry/opentelemetry-proto-go#248

Additional Context

open-telemetry/opentelemetry-go#2579

[arjan-bal]

Hi @qianlongzt, I was going though the linked issue but I don't completely understand how the proposed change addresses the issue. I found this comment that summarises the change: open-telemetry/opentelemetry-go#2579 (comment)

Can you elaborate how your proposed solution will solve the problem of splitting gRPC client/server code and protobuf request/response messages into two separate packages?

[qianlongzt]

@arjan-bal #8245 I created a pr.

[arjan-bal]

@qianlongzt I tried running the changed from #8245 using the following command:

protoc \
    --go-grpc_out="${TEMPDIR}" \
    --go-grpc_opt=paths=source_relative,use_generic_streams_experimental=true,grpc_mapping='examples/route_guide/routeguide/route_guide.proto=google.golang.org/grpc/examples/route_guide/routeguidegrpc' \
    "examples/route_guide/routeguide/route_guide.proto"

The go package name was still routeguide, but the grpc generated code had an import for the default package name.

import (
       // Omitted
	routeguide "google.golang.org/grpc/examples/route_guide/routeguide"

Where does the new package name show up? It would be helpful if you could share an example using the examples/route_guide/routeguide/route_guide.proto file in this repo.

[qianlongzt]

The output is correct. The user does not need to use source_relative or manually move _grpc.pb.go to a new package, and the import statement is necessary.

examples/route_guide
  /routeguide
    route_guide.pb.go
    route_guide_grpc.pb.go

examples/route_guide
  /routeguide
    route_guide.pb.go
  /routeguidegrpc
    route_guide_grpc.pb.go (moved)

[arjan-bal]

I was trying to see if we can have two packages for the same proto file, one for grpc service code and one for messages without any code changes or splitting the proto file. There is a M option that protoc-gen-go accepts for mapping proto files to go packages. Using the following command I can get the generated code to be placed in separate packages, but the grpc code doesn't import the message code, so the code will not compile.

protoc \                  
  --go_out=Mroute_guide.proto=example.com/message:. \
  --go-grpc_out=Mroute_guide.proto=example.com/service:. \
  route_guide.proto

This seems to be the reason we need to change the grpc codegen code. I'm still trying to understand the grpc codegen code.

[arjan-bal]

I think I understand the proposed changes better now. Here we don't really need a mapping like the grpc_rename param mentioned above. We can instead use a boolean here.

	filename := file.GeneratedFilenamePrefix + "_grpc.pb.go"
	importPath := file.GoImportPath
	if *useSeparateServicePackage {
		importPath = file.GoImportPath + "_" + protogen.GoImportPath("grpc")
	}
	g := gen.NewGeneratedFile(filename, importPath)

The effect is that the symbols from the generated message code use qualified names (eg: routeguide.Point instead of Point) and an import for the message package is added to the top of the file, eg: routeguide "google.golang.org/grpc/examples/route_guide/routeguide".

However, since the package name at the top of the generated message and service code is the same, and both the files are in the same directory, the code will not compile complaining that the grpc code imports its own package causing a cycle. To get separate packages, users will need to specify the --go-grpc_out flag pointing to a different path for the service code.

[qianlongzt]

Use a mapping to let the user switch to a different package. For OpenTelemetry-Go, I'm not sure which package will be used.

the code will not compile complaining that the grpc code imports its own package causing a cycle.

This error is expected and acceptable.

[arjan-bal]

Use a mapping to let the user switch to a different package. For OpenTelemetry-Go, I'm not sure which package will be used.

Can you give me a real world example where the values in the mapping will matter? IIUC, service code will import message code, but

1. service code will never import other service code and
2. message code will never import service code

This means that the import path of the service code doesn't really get used anywhere.

[qianlongzt]

You are right—the importPath is not useful. I wanted to map the filename, but useSeparateServicePackage is sufficient. OpenTelemetry-Go generates the file and copies it, so modifying the copy command works well.

[arjan-bal]

I want to ensure that the usage of the flag is easy for users to understand. I was wondering why OpenTelemetry-Go needs to copy the generated file. Is there something preventing them from using --go-grpc_out with the --go-grpc_opt=paths= option to place the generated code in the correct location?

[qianlongzt]

I'm not very familiar with the generation of opentelemetry-proto-go. The protobuf definitions are in 1, the generation script is in 2, and the output is in 3.

They use temporary folders gen/go and gen/proto.

Maybe @pellared can answer your question.

[pellared]

We are using go-grpc_out with the --go-grpc_opt=paths= options. I do not remember the details why we use temporary folders but I think is simply helps us debugging.

I am not sure if it was called out anywhere but this issue is a workaround/mitigation for https://protobuf.dev/reference/go/faq/#namespace-conflict. Related issue that I see as the root cause: golang/protobuf#1122

[pellared]

Maybe reading from here: open-telemetry/opentelemetry-go#2579 (comment) will also be helpful.

[arjan-bal]

I'm discussing the addition of a new flag with other maintainers.

[arjan-bal]

There weren't any objections to this change. We need to implement the following:

1. Add a boolean flag that adds a "_grpc" suffix to the import path as mentioned in protoc-gen-go-grpc support split single proto file gen to diffrent package #8233 (comment).
2. Add a test to verify the expected output, see this file for an example.

[arjan-bal]

@qianlongzt will you be willing to work on a PR for this?

[qianlongzt]

@arjan-bal #8280 I created a pr