Update gateway to avoid an extra gRPC call #1174
Conversation
bogdandrutu
requested review from
dmitryax,
flands,
owais,
pjanotti,
rghetia,
songy23 and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1174 +/- ##
==========================================
+ Coverage 86.87% 86.91% +0.03%
==========================================
Files 201 201
Lines 14524 14516 -8
==========================================
- Hits 12618 12616 -2
+ Misses 1456 1452 -4
+ Partials 450 448 -2
Continue to review full report at Codecov.
|
Signed-off-by: Bogdan Drutu <[email protected]>
| return | ||
| // Register the grpc-gateway on the HTTP server mux | ||
| if r.traceReceiver != nil { | ||
| // Cannot return error, impossible to test |
There was a problem hiding this comment.
If it cannot return error then it's safer to panic in case the impossible does happen than ignoring the error
if err != nil {
panic(err)
}There was a problem hiding this comment.
See the comment, I did this because of test coverage :(. And panic will not help. If you have any idea (I would avoid adding a func pointer just to test an impossible to fail function)
There was a problem hiding this comment.
I did this because of test coverage :(
Well, this really seems like a code smell just to make the code coverage happy :(
Go linters normally complain about the incorrect error handling like this.
Looks like the only way to exclude this block from code coverage is to extract it to a separate file and exclude from the file list for go tool cover. I agree that's not worth it.
There was a problem hiding this comment.
I know also that API smells a bit because returns an error that never happens. :(
Will give a shot tomorrow to have something more reasonable
| return | ||
| if r.metricsReceiver != nil { | ||
| // Cannot return error, impossible to test | ||
| _ = collectormetrics.RegisterMetricsServiceHandlerServer(context.Background(), r.gatewayMux, r.metricsReceiver) | ||
| } | ||
|
|
||
| // Start the gRPC and HTTP/JSON (grpc-gateway) servers on the same port. |
There was a problem hiding this comment.
The comment about JSON is probably misleading now, given the recent change #1021 introducing support for application/x-protobuf and your comment https://github.com/open-telemetry/opentelemetry-proto/issues/129#issuecomment-630413454.
It seems that grpc-gateway still supports JSON as a default serialization format, see https://github.com/grpc-ecosystem/grpc-gateway/blob/ee3ef70b7777cde4e61e4e224cb11e92beecee6a/runtime/marshaler_registry.go#L78
To disable JSON completely, we need to register protobuf handler for * (fallback wildcard) in addition to application/x-protobuf.
There was a problem hiding this comment.
This is the gRPC over HTTP (with the limitation of the trace/span id) from my comment. I am confused why to disable, also this PR does not change anything related to that, please make a different PR if you think there is a different problem.
There was a problem hiding this comment.
I am confused why to disable
In your comment I linked above, you say that we don't want to support JSON yet, but we already do, and there is a known issue with encoding of trace and span IDs.
So to prevent people from using the JSON format until we decide to officially support it, we need to override the default marshaller in gRPC Gateway.
You're right that this is out of scope of this PR, I just made a comment to clarify the desired behaviour since you've touched the gRPC Gateway.
There was a problem hiding this comment.
Please file an issue to not forget about this. As you said is not in the scope so don't want to focus on other things.
I'm afraid this won't work for OpenCensus. See comment on
|
|
@nilebox ack on the opencensus not being able to support this. we should still regenerate the files and update the proto deps etc. |
|
Done. in #1223 |
Same change cannot be done for opencensus because it uses an old version of the protoc-gen-frpc-gateway. @nilebox or @james-bebbington can you please regenerate protos in opencensus-proto repository?