[PROTOTYPE] Combine gRPC and HTTP servers #3765

a-feld · 2021-08-03T18:33:45Z

gRPC / HTTP server consolidation 🚧 [PROTOTYPE] 🚧

This PR consolidates the gRPC server and HTTP servers into 1 centralized server. The architecture of this solution enables:

A consolidated 4317 port for both gRPC and HTTP traffic
More configurable transports given that the official h2 / h2c golang servers are used rather than the gRPC implementation
A smaller footprint for the binary given that the gRPC library is no longer compiled into the receiver

Notes

We should probably discuss direction on how to transport headers into the processors / exporters. There are valid reasons for wanting to access the headers - one example being the implementation of a multi-tenant solution where API keys may change.

tigrannajaryan · 2021-08-04T09:02:10Z

We have just merged an amendment to the spec that says gRPC and HTTP servers use different ports: open-telemetry/opentelemetry-specification#1839 because the previous research concluded it is not possible.

Is there now a new evidence that it is possible? Not sure what we do with spec change if that's the case. If we absolutely certain this is doable we may need to revert the spec change.

We should probably discuss direction on how to transport headers into the processors / exporters. There are valid reasons for wanting to access the headers - one example being the implementation of a multi-tenant solution where API keys may change.

There is a proposal that enables this via a new concept called PDataContext: #3745

tigrannajaryan · 2021-08-06T08:32:36Z

@a-feld would you be able to make progress on this in the next couple days? I have reverted the recent spec change that has split the ports. The spec is now waiting for the final decision that we make here. If we are able to serve OTLP/gRPC and OTLP/HTTP on one port we will keep the spec as is, if not we will do the splitting as it was planned.

a-feld · 2021-08-06T13:30:23Z

@tigrannajaryan will do! I'm splitting my time between this PR and open-telemetry/opentelemetry-collector-contrib#4444

I'll try to make some progress on this PR today!

a-feld · 2021-08-06T22:16:40Z

I didn't get to this today. I'm actually out on holiday next week but I'll try to make progress on Monday so that we can push this forward.

The majority of the logic changes simply extract the http2.Server into the ToServer method so that it is accessible for adding the h2c handler.

By default, the experimental server is disabled. If enabled, the default endpoint is set to 0.0.0.0:4317.

a-feld · 2021-08-10T02:29:56Z

@tigrannajaryan I made some progress on this PR today. I focused on simplifying the diff and breaking out the commits so that it's more easily reviewable.

Using @bogdandrutu 's suggestion from the SIG last week, I created a new experimental_server_enabled configuration option. By default, the new gRPC handlers are not active.

The only truly different change here is that there is an h2c handler installed on http servers when TLS is not enabled (in accordance with rfc7540 section-3.4). This shouldn't be a breaking change though.

Please let me know what your thoughts are! If acceptable, I'll continue to build out robust testing of this new change.

tigrannajaryan · 2021-08-10T04:43:55Z

@open-telemetry/collector-approvers @Aneurysm9 @dashpole @jmacd or anyone else who have experience with Golang HTTP server and gRPC, can you help review this? This is a relatively small but important and high impact change and it would be great to have several eyes on this.

tigrannajaryan

Please add a unit test that verifies that experimental mode with actual OTLP receiver and OTLP/gRPC and OTLP/HTTP exporers.

tigrannajaryan · 2021-08-10T04:46:56Z

config/confighttp/confighttp.go

+
+	// Declare an HTTP/2 server which will handle any H2 protocol traffic
+	h2Server := &http2.Server{
+		NewWriteScheduler: func() http2.WriteScheduler { return http2.NewPriorityWriteScheduler(nil) },


Add a comment to explain why we need priority scheduler and not the default one.

tigrannajaryan · 2021-08-10T04:52:52Z

receiver/otlpreceiver/otlp.go

@@ -202,6 +203,11 @@ func (r *otlpReceiver) registerTraceConsumer(tc consumer.Traces) error {
 		return componenterror.ErrNilNextConsumer
 	}
 	r.traceReceiver = trace.New(r.cfg.ID(), tc)
+	if r.cfg.ExperimentalServerEnabled {
+		r.httpMux.HandleFunc("/opentelemetry.proto.collector.trace.v1.TraceService/Export", func(resp http.ResponseWriter, req *http.Request) {


This is a bit fragile. If we rename a message or service in the proto this will no longer work. Can we have a test that ensures the path for this handler matches the proto definition correctly?
Or maybe event better, there is a way to get this string from generated protobuf *.pb.go files? (I don't see it).

tigrannajaryan · 2021-08-10T04:55:36Z

receiver/otlpreceiver/otlp.go

+	if r.cfg.ExperimentalServerEnabled {
+		r.httpMux.HandleFunc("/opentelemetry.proto.collector.metrics.v1.MetricsService/Export", func(resp http.ResponseWriter, req *http.Request) {
+			handleMetrics(resp, req, grpcContentType, r.metricsReceiver, metricsPbUnmarshaler)
+		}).Methods(http.MethodPost).Headers("Content-Type", grpcContentType)
+	}


Should this be inside if r.httpMux != nil { check? Not sure when can r.httpMux be nil though. Perhaps it can't anymore since we always create http server when ExperimentalServerEnabled?

tigrannajaryan · 2021-08-10T05:00:50Z

receiver/otlpreceiver/otlphttp.go

+		var err error
+		resp, err = newGrpcResponseWriter(resp, req)
+		if err != nil {
+			return


Should we read and close the body even if this errors out? Just wondering if otherwise it may leak memory since the request is pending somewhere.

Should we return an InternalServerError to the client especially since we don't record the error in any other way here?

tigrannajaryan · 2021-08-10T05:04:26Z

receiver/otlpreceiver/otlphttp.go

@@ -57,7 +96,7 @@ func handleTraces(
 	}

 	// TODO: Pass response from grpc handler when otlpgrpc returns concrete type.
-	writeResponse(resp, contentType, http.StatusOK, &types.Empty{})
+	writeResponse(resp, contentType, http.StatusOK, emptyMessage)


I was a bit confused by this. For clarity we should probably write proper Export*ServiceResponse protobuf message, which indeed is empty right now, but may not be empty in the future if the proto evolves.

tigrannajaryan · 2021-08-10T05:05:53Z

receiver/otlpreceiver/otlphttp.go

+	grpcRw.WriteHeader(int(codes.Internal))
+	compressed, err := unwrapProtoFromGrpc(req.Body)
+	if err != nil {
+		writeError(grpcRw, grpcContentType, err, http.StatusBadRequest)
+		return nil, err
+	}


Add some comments to explain what happens here and why.

tigrannajaryan · 2021-08-10T05:09:15Z

receiver/otlpreceiver/otlphttp.go

+		if s, ok := rsp.(*spb.Status); ok {
+			statusCode = int(s.Code)
+			w.Header().Set(grpcMessage, s.Message)
+			rsp = emptyMessage
+		} else {
+			statusCode = int(codes.OK)
+		}


Any links to gRPC specs that make it clear that this is the right thing to do? Similarly for other code that deals with gRPC peculiarities.

jpkrohling · 2021-08-10T11:48:48Z

This is a relatively small but important and high impact change and it would be great to have several eyes on this.

During last week's SIG Collector call, we discussed this and agreed to make this opt-in for at least a couple of releases, to get people to test it in the real world. My biggest concern right now is that we do not know how it would impact load balancers and proxies that are aware of gRPC, like NGINX and some service meshes.

If we can get this change as a forked OTLP receiver in the contrib, we can then compare the two implementations. With that data, we'll be able to make a better decision.

a-feld · 2021-08-10T17:46:48Z

Thanks for the feedback @tigrannajaryan and @jpkrohling !

During last week's SIG Collector call, we discussed this and agreed to make this opt-in for at least a couple of releases, to get people to test it in the real world.

@jpkrohling The change as currently defined is definitely opt-in. The usage of this new feature requires experimental_server_enabled: true to the configuration. By default, that flag is false and results in no changes to the gRPC handlers, and what should be no changes to the http handlers. With that in mind, do we still want to create a fork of the receiver?

tigrannajaryan · 2021-08-11T08:22:28Z

With that in mind, do we still want to create a fork of the receiver?

@jpkrohling are you specifically looking for a fork? It seems to be an option would be sufficient and will allow us to test the new behavior while keeping the old behavior intact. Please clarify what you had in your mind with forking.

We do definitely need tests that verify the new behavior anyway, including end-to-end using our own OTLP/gRPC and OTLP/HTTP exporters.

jpkrohling · 2021-08-11T09:08:18Z

are you specifically looking for a fork?

I mean to have another OTLP receiver in the contrib repository using this new approach.

tigrannajaryan · 2021-08-11T10:28:54Z

are you specifically looking for a fork?

I mean to have another OTLP receiver in the contrib repository using this new approach.

I understand. I wonder if we get about the same level of testability with the opt-in experimental_server_enabled config flag and without forking? Or you are worried that the changes in this PR may break something even when experimental_server_enabled is not set?

If we are going to fork then to be completely safe forking will require to also fork HTTPServerSettings since this PR changes how the http.Server instance is created.

jpkrohling · 2021-08-11T11:02:56Z

I guess it depends on how intrusive the changes are. I remember asking @a-feld about it, and he mentioned that they were pretty intrusive.

tigrannajaryan · 2021-08-11T11:05:57Z

I guess it depends on how intrusive the changes are. I remember asking @a-feld about it, and he mentioned that they were pretty intrusive.

I would like to know as well, but since I am not familiar with Go's http server implementation and gRPC in general it is hard for me to assess the impact. It would be great if someone who is deeply familiar with it to chime in.

github-actions · 2021-09-04T03:15:48Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

tigrannajaryan · 2021-09-14T14:00:11Z

@a-feld do you plan to continue working on this?

a-feld · 2021-09-14T19:04:54Z

Hi @tigrannajaryan I've been waiting for #3890 to get merged.

Also, as a bit of news - I have resigned from New Relic effective 9/24/2021 so I will be closing any open PRs after that date. I'm happy to continue to work to get these PRs merged in until then! After that time, I will be unable to submit further contributions until further notice.

github-actions · 2021-09-22T03:16:07Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

sodabrew · 2021-09-22T16:06:32Z

Driveby reminder that @a-feld will close this PR in two days. Is merging ports still the canonical plan?

Is there someone else who can take on the work using the same code, on the basis that it's been contributed by New Relic to CNCF, or does the next person need to start blank?

mtwo · 2021-09-22T16:24:06Z

@bogdandrutu to review and merger prior to 9/24

a-feld · 2021-09-24T14:09:26Z

This will not likely be merged by 9/24. I'll go ahead and close this out for others to pick up.

Related to open-telemetry#1816 Fixes open-telemetry#1835 Fixes open-telemetry#1920 Some historical context: we wanted to make grpc and http use the same port and we had an open issue in the Collector to do so: open-telemetry/opentelemetry-collector#1256 The conclusion is that there are technical hurdles that make it unfeasible: open-telemetry/opentelemetry-collector#1256 (comment) Because of that we need to keep grpc and http ports separate. This means we need to change the spec to say that otlp/http uses port 4318. Once this PR is merged we will also need to submit for port 4318 registration with IANA like we did previously with port 4317: open-telemetry#1148 (comment) There was also a draft PR to merge the ports in the Collector but it was not completed: open-telemetry/opentelemetry-collector#3765 Note that this change was initially submitted in PR open-telemetry#1839 and then reverted in PR open-telemetry#1847 because we hoped that the merging could be successfully done. We believe that we should no longer pursue this and should consider the ports separate from now on.

Related to #1816 Fixes #1835 Fixes #1920 Some historical context: we wanted to make grpc and http use the same port and we had an open issue in the Collector to do so: open-telemetry/opentelemetry-collector#1256 The conclusion is that there are technical hurdles that make it unfeasible: open-telemetry/opentelemetry-collector#1256 (comment) Because of that we need to keep grpc and http ports separate. This means we need to change the spec to say that otlp/http uses port 4318. Once this PR is merged we will also need to submit for port 4318 registration with IANA like we did previously with port 4317: #1148 (comment) There was also a draft PR to merge the ports in the Collector but it was not completed: open-telemetry/opentelemetry-collector#3765 Note that this change was initially submitted in PR #1839 and then reverted in PR #1847 because we hoped that the merging could be successfully done. We believe that we should no longer pursue this and should consider the ports separate from now on. Note 2: we consider this a spec bug fix (spec was impossible to implement), rather than a functionality change, that's why we believe this is an allowed change.

Related to open-telemetry/opentelemetry-specification#1816 Fixes open-telemetry/opentelemetry-specification#1835 Fixes open-telemetry/opentelemetry-specification#1920 Some historical context: we wanted to make grpc and http use the same port and we had an open issue in the Collector to do so: open-telemetry/opentelemetry-collector#1256 The conclusion is that there are technical hurdles that make it unfeasible: open-telemetry/opentelemetry-collector#1256 (comment) Because of that we need to keep grpc and http ports separate. This means we need to change the spec to say that otlp/http uses port 4318. Once this PR is merged we will also need to submit for port 4318 registration with IANA like we did previously with port 4317: open-telemetry/opentelemetry-specification#1148 (comment) There was also a draft PR to merge the ports in the Collector but it was not completed: open-telemetry/opentelemetry-collector#3765 Note that this change was initially submitted in PR open-telemetry/opentelemetry-specification#1839 and then reverted in PR open-telemetry/opentelemetry-specification#1847 because we hoped that the merging could be successfully done. We believe that we should no longer pursue this and should consider the ports separate from now on. Note 2: we consider this a spec bug fix (spec was impossible to implement), rather than a functionality change, that's why we believe this is an allowed change.

tigrannajaryan mentioned this pull request Aug 6, 2021

Revert "Change OTLP/HTTP port from 4317 to 4318 (#1839)" open-telemetry/opentelemetry-specification#1847

Merged

a-feld added 8 commits August 9, 2021 21:31

Add h2c handler when tls configuration is absent.

dd3aa4d

The majority of the logic changes simply extract the http2.Server into the ToServer method so that it is accessible for adding the h2c handler.

Add an experimental server configuration field.

bb8482a

Add experimental server configuration.

c4936c1

By default, the experimental server is disabled. If enabled, the default endpoint is set to 0.0.0.0:4317.

Use experimental server config to start http server.

3cb6ba5

Export the body decompression logic for use in gRPC.

ae11f8a

Add a small gRPC wrapper to translate HTTP<->gRPC.

6137abe

Add modifications to handle gRPC content type on the http server.

60c44c8

Add handlers for the experimental gRPC server.

1b0a9e0

a-feld force-pushed the grpc-to-http2-server branch from 61fb0bb to 1b0a9e0 Compare August 10, 2021 02:26

Use bool for experimental server.

b6d342c

tigrannajaryan requested a review from bogdandrutu August 10, 2021 04:39

tigrannajaryan requested review from a team, Aneurysm9, dashpole and jmacd August 10, 2021 04:44

tigrannajaryan reviewed Aug 10, 2021

View reviewed changes

longility mentioned this pull request Aug 28, 2021

Follow spec defined port 4318 for HTTP exporter open-telemetry/opentelemetry-js#2395

Closed

github-actions bot added the Stale label Sep 4, 2021

bogdandrutu removed the Stale label Sep 7, 2021

tigrannajaryan mentioned this pull request Sep 14, 2021

otlp default endpoint port for http is different from collector receiver default open-telemetry/opentelemetry-specification#1920

Closed

codeboten mentioned this pull request Sep 14, 2021

update OTLP/HTTP example & test port to 4318 open-telemetry/opentelemetry-python#2016

Merged

6 tasks

github-actions bot added the Stale label Sep 22, 2021

bogdandrutu removed the Stale label Sep 22, 2021

a-feld closed this Sep 24, 2021

rauno56 mentioned this pull request Sep 27, 2021

Specify export protocol signalfx/gdi-specification#131

Closed

tigrannajaryan mentioned this pull request Sep 27, 2021

Change OTLP/HTTP port from 4317 to 4318 (#1839) open-telemetry/opentelemetry-specification#1970

Merged

jpkrohling mentioned this pull request Mar 23, 2022

Implement gRPC deframing directly on HTTP receiver port. #5067

Closed

spencergilbert mentioned this pull request Jul 18, 2022

Support ingesting OpenTelemetry logs vectordotdev/vector#13551

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROTOTYPE] Combine gRPC and HTTP servers #3765

[PROTOTYPE] Combine gRPC and HTTP servers #3765

a-feld commented Aug 3, 2021

tigrannajaryan commented Aug 4, 2021

tigrannajaryan commented Aug 6, 2021

a-feld commented Aug 6, 2021

a-feld commented Aug 6, 2021

a-feld commented Aug 10, 2021 •

edited

Loading

tigrannajaryan commented Aug 10, 2021

tigrannajaryan left a comment

tigrannajaryan Aug 10, 2021

tigrannajaryan Aug 10, 2021

tigrannajaryan Aug 10, 2021

tigrannajaryan Aug 10, 2021

tigrannajaryan Aug 10, 2021

tigrannajaryan Aug 10, 2021

tigrannajaryan Aug 10, 2021

jpkrohling commented Aug 10, 2021

a-feld commented Aug 10, 2021

tigrannajaryan commented Aug 11, 2021

jpkrohling commented Aug 11, 2021

tigrannajaryan commented Aug 11, 2021

jpkrohling commented Aug 11, 2021

tigrannajaryan commented Aug 11, 2021

github-actions bot commented Sep 4, 2021

tigrannajaryan commented Sep 14, 2021

a-feld commented Sep 14, 2021

github-actions bot commented Sep 22, 2021

sodabrew commented Sep 22, 2021

mtwo commented Sep 22, 2021

a-feld commented Sep 24, 2021

[PROTOTYPE] Combine gRPC and HTTP servers #3765

[PROTOTYPE] Combine gRPC and HTTP servers #3765

Conversation

a-feld commented Aug 3, 2021

gRPC / HTTP server consolidation 🚧 [PROTOTYPE] 🚧

Notes

tigrannajaryan commented Aug 4, 2021

tigrannajaryan commented Aug 6, 2021

a-feld commented Aug 6, 2021

a-feld commented Aug 6, 2021

a-feld commented Aug 10, 2021 • edited Loading

tigrannajaryan commented Aug 10, 2021

tigrannajaryan left a comment

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

tigrannajaryan Aug 10, 2021

Choose a reason for hiding this comment

jpkrohling commented Aug 10, 2021

a-feld commented Aug 10, 2021

tigrannajaryan commented Aug 11, 2021

jpkrohling commented Aug 11, 2021

tigrannajaryan commented Aug 11, 2021

jpkrohling commented Aug 11, 2021

tigrannajaryan commented Aug 11, 2021

github-actions bot commented Sep 4, 2021

tigrannajaryan commented Sep 14, 2021

a-feld commented Sep 14, 2021

github-actions bot commented Sep 22, 2021

sodabrew commented Sep 22, 2021

mtwo commented Sep 22, 2021

a-feld commented Sep 24, 2021

a-feld commented Aug 10, 2021 •

edited

Loading