Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update gateway to avoid an extra gRPC call #1174

Closed
wants to merge 1 commit into from
Closed

Update gateway to avoid an extra gRPC call #1174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 7 additions & 15 deletions receiver/otlpreceiver/otlp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,24 +224,16 @@ func (r *Receiver) startServer(host component.Host) error {
err := componenterror.ErrAlreadyStarted
r.startServerOnce.Do(func() {
err = nil
// Register the grpc-gateway on the HTTP server mux
c := context.Background()
opts := []grpc.DialOption{grpc.WithInsecure()}
endpoint := r.ln.Addr().String()

_, ok := r.ln.(*net.UnixListener)
if ok {
endpoint = "unix:" + endpoint
}

err = collectortrace.RegisterTraceServiceHandlerFromEndpoint(c, r.gatewayMux, endpoint, opts)
if err != nil {
return
// Register the grpc-gateway on the HTTP server mux
if r.traceReceiver != nil {
// Cannot return error, impossible to test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@nilebox nilebox Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_ = collectortrace.RegisterTraceServiceHandlerServer(context.Background(), r.gatewayMux, r.traceReceiver)
}

err = collectormetrics.RegisterMetricsServiceHandlerFromEndpoint(c, r.gatewayMux, endpoint, opts)
if err != nil {
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1177

Expand Down