-
Notifications
You must be signed in to change notification settings - Fork 897
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.
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
Default protocol for OTLP exporter selection SDK env #1885
Comments
OTLP/HTTP is slightly more CPU efficient than OTLP/gRCP so it may be preferable. However, it may be that most SDKs implement OTLP/gRCP already, but not OTLP/HTTP so making OTLP/HTTP may not work. This likely needs a discussion in Maintainers meeting. Adding to the next week's agenda. |
From the maintainers call: general agreement that gRPC should be the default on each artifact, unless the HTTP implementation is substantially better on that artifact. Most maintainers in the call were already following this pattern. |
Here is some informal performance data for Go implementation (benchmarks an OTLP client+server):
OTLP/HTTP is about 2x more CPU efficient for batches that contain a single span. For larger batches (>=100 spans) the difference between OTLP/HTTP and OTLP/gRPC is insignificant. I am fine with making/keeping gRPC the default given that it is already what we do unless someone feels very strong about the performance impact for small batches (and keep in mind that in other languages the benchmarks may show a different result, this is just for Go). |
It seems like supporting OTLP/gRPC in Ruby is not straightfoward. @fbogsany please comment here. I am personally slightly more favour of OTLP/HTTP but it seems like other languages preferred gRPC. If we believe it is more complicated (and undesirable) to make OTLP/gRPC the default for Ruby then we either make OTLP/HTTP the default and required for al languages (I would prefer this) or allow each language to choose a default (this is a bit less desirable since we loose uniformness and require receivers to support both transports to be able to receive from variety of Otel SDKs). |
Copied from #1969 (comment)
|
@mtwo (and anybody else who was in the Maintainers call) can you please list the arguments that the Maintainers had in favour of OTLP/gRPC as the default? There is evidence that it is difficult to implement (Ruby) and performs worse that OTLP/HTTP (see my comment). |
From a theoretical perspective, gRPC needs a HTTP/2 library underneath, so technically you should be able to implement OTLP/HTTP at least everywhere where you can implement OTLP/gRPC. The opposite is not true, since there might not be a (good) gRPC implementation for your target language/framework/version. Is there any reason why gRPC should be preferred over HTTP as a minimal requirement? |
From the compliance matrix, it seems that C++, Rust, Swift and .NET have implemented gRPC but not HTTP. For .NET there is a open PR already (open-telemetry/opentelemetry-dotnet#2316). Taking a quick look at Rust, it actually seems to be implemented (for traces only): https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-otlp/src/exporter/http.rs and https://github.com/open-telemetry/opentelemetry-rust/blob/e7ecf58e0b0d1eeff213e458e47e2238575826b4/opentelemetry-otlp/src/lib.rs#L253-L261 C++ also seems to support OTLP/HTTP actually: https://github.com/open-telemetry/opentelemetry-cpp/tree/main/exporters/otlp#configuration-options--otlp-http-exporter- (both JSON and binary according to that documentation) (CC @owent @lalitb @reyang) That leaves only swift as a language with gRPC support but no HTTP support. On the other hand we have Ruby where we know that supporting gRPC would not be easy while HTTP is supported. |
We should also be clear about OTLP/HTTP+protobuf (what I think we are talking about) vs OTLP/HTTP+JSON (what some seem to view as default for OTLP/HTTP) |
As far as I remember, we only supported OTLP/HTTP+protobuf but not OTLP/HTTP+JSON. Will update spec accordingly. |
For .NET, we'd be keen on defaulting to gRPC simply because it would be a breaking change to change it to OTLP/HTTP once this PR lands.
If the spec could be flexible and recommend gRPC as default but allow for HTTP as default when it makes sense, I think this would be best. I'm not a JS expert, but my understanding is that gRPC is not a thing in browsers, so this may be another use case where HTTP is preferred (or required). |
+1. Up until recently if I remember correctly AWS load balancers did not even let gRPC connections through (I think they do now), so there may be real production cases when you simply cannot use gRPC.
The only reason I can think of is that more SDKs today implement gRPC than HTTP, so it appears making HTTP is more work for Otel maintainers. It does not seem to be a huge deal though, if you have a working OTLP/gRPC exporter, adding OTLP/HTTP should not be very complicated. |
The spec currently strongly hints that OTLP/HTTP+JSON is optional:
|
@Oberon00 very useful research, thank you. I think this reinforced that overall adding support for OTLP/HTTP where it is missing is not a big effort, while OTLP/gRPC where it is missing looks problematic. Given this new evidence I now am now more strongly incline towards making OTLP/HTTP the required protocol. Let's hear some strong arguments against if there are any. |
We could make OTLP/HTTP support a |
Java's been released for several months now and has always used gRPC as the default for the env variable - it would be extremely disruptive to users to change the default at this point I think. This issue doesn't seem all that different from #1923 and I suspect allowing SIGs to have their default is going to be the most reasonable, it's one of those values that needed to be set early on or never set. Making OTLP/HTTP the required protocol seems ok though to me - I agree that every language should be able to support HTTP easily if they can support gRPC, while otherwise may not even be able to support gRPC at all. It just leaves the somewhat awkward situation where languages like Java would be defaulting to the non-required protocol. It's not terrible though IMO. |
@anuraaga so just to clarify, you suggest that:
I think this is OK, just a bit more work on the receivers side, but otherwise not a big problem. |
Everything aligns with me except this part - it depends on if JS in browser absolutely requires JSON. My personal experience with grpc-web is binary protobuf (not grpc proper) in browsers is OK - I suspect that browsers that did poorly with binary payloads are not supported by the vast majority of sites now, and only search engines, being core infrastructure, would still target such old ones. I don't expect such code to actually migrate to use OTel in practice, they generally delegate old browsers to old heirloomed code. If this is not true based on the JS SIG's experience, then that's that since they know best - for a receiver to be fully compatible even with browsers, it would need to support JSON. I think it is worth confirming this is really true in 2021. And if it is, special language for just the browser - so grpc and http are enough to "support OpenTelemetry" but JSON may be needed if that receiver specifically supports browsers - seems appropriate. Forcing support for web browsers on any backend implementing OTel seems like an overreach. One last note - if we do require receivers to support JSON, please please please require parsing to support reading trace IDs either as hex or base64. Protobuf-json doesn't support hex so it's irresponsible to require implementing the entire parsing logic by hand just because of this, especially given SDKs won't implement this since they are only concerned with export. For reference, this is how we "parse" for SDK unit tests Edit: Sorry after rereading my comment I realized it's not "require parsing to support reading trace IDs either as hex or base64" because that is already too hard, the hex path needs to be implemented. IMO it is either 1) Only support base64, just like proto or 2) require SDKs to provide parsing logic so backends can use it and not struggle so much on this deviation from proto3. |
On second thought I'm not so confident in this. It does seem a little excessive, but in the proposed vision doc |
I was able to override just the parsing to trace ids/span id and let Protobuf's default implementation parse the rest: https://github.com/open-telemetry/opentelemetry-collector/blob/afad36e3785b78906f6eb5f9e91354a9e05a274c/receiver/otlpreceiver/otlp_test.go#L76 |
I added this to next Maintainers' meeting agenda. I am not sure I will be able to join, but this needs to be discussed. So far I believe we are inclined to say that each language is free to choose their own default (we can also in addition recommend that OTLP/HTTP/Protobuf is the default). |
Just checked: AWS still does not fully support gRPC for all load balancer types: https://aws.amazon.com/elasticloadbalancing/features/ |
@tigrannajaryan NLB is just a TCP/IP load balancer so it is not tied to an application protocol like gRPC. Actually before ALB supported gRPC, NLB was the only way to implement a gRPC API with a load balancer on AWS. It had the limitation of not being able to integrate things like TLS, so the support in ALB made it much nicer :) But I would consider this to be full support for gRPC on the cloud side. |
All right, I guess I was misled by the |
@tigrannajaryan It seems the "Layer 7" section header type of thing is the scapegoat for that :) I think it basically means "Classic Load Balancer in L7 mode" doesn't support gRPC. FWIU, it's EoL so probably not needed for OTLP https://aws.amazon.com/blogs/aws/ec2-classic-is-retiring-heres-how-to-prepare/ |
Discussed again in Maintainers meeting. Raw meeting notes:
To summarize this is what we want to do:
I am going to rewrite this PR according to the plan above. If there are any objections to the plan please speak up. |
Resolves #1885 - Strongly recommend SDKs to implement both OTLP/gRPC and OTLP/HTTP+Protobuf exporters. - Strongly recommend OTLP/HTTP+Protobuf to be the default protocol but allow language SDKs to choose a different default if they have good reasons.
Resolves open-telemetry/opentelemetry-specification#1885 - Strongly recommend SDKs to implement both OTLP/gRPC and OTLP/HTTP+Protobuf exporters. - Strongly recommend OTLP/HTTP+Protobuf to be the default protocol but allow language SDKs to choose a different default if they have good reasons.
Resolves open-telemetry/opentelemetry-specification#1885 - Strongly recommend SDKs to implement both OTLP/gRPC and OTLP/HTTP+Protobuf exporters. - Strongly recommend OTLP/HTTP+Protobuf to be the default protocol but allow language SDKs to choose a different default if they have good reasons.
cc @jtescher ## Changes Update Rust's support for OTLP/HTTP binary protocol Related issues open-telemetry#1885
What are you trying to achieve?
Exporter selection https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#exporter-selection
part of spec has default for jaeger and zipkin but there is no default for otlp. It is unclear whether to use
otlp/grpc
orotlp/http
for the client libraries that have support for both.The text was updated successfully, but these errors were encountered: