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.

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

Make OTLP/HTTP the recommended default transport #1969

Merged

Conversation

tigrannajaryan
Copy link
Member

Resolves #1885

@tigrannajaryan
Copy link
Member Author

@open-telemetry/ruby-approvers @open-telemetry/php-approvers please review. This makes gRPC the default and required transport and the compliance matrix currently says that Ruby and PHP don't support it OTLP/gRPC so this impacts you the most.

@fbogsany
Copy link
Contributor

gRPC is less commonly used in the Ruby ecosystem than in other languages and Ruby appears to be a lower priority than other languages for gRPC maintainers. This means that we have much less experience with gRPC in the Ruby SIG and little demand for it so far. Battle scars from spectacular Ruby + gRPC production failures also make some of us quite wary of the support burden.

We have had considerable success with OTLP/HTTP in OpenTelemetry Ruby. Is there a strong reason to force gRPC as a default transport rather than leaving the default up to individual SIGs? Similarly, is there a strong reason to require gRPC as a transport? Both of these are challenging for the Ruby SIG.

@tigrannajaryan
Copy link
Member Author

@fbogsany can you please post your comment here #1885 and let's discuss there until we decide what's the way forward. I will keep this as draft for now.

@Oberon00

This comment has been minimized.

@tigrannajaryan tigrannajaryan marked this pull request as draft September 28, 2021 18:54
@tigrannajaryan
Copy link
Member Author

Discussed again in Maintainers meeting. Raw meeting notes:

Feedback from Bob: +1, gRPC has great implementations in C++, Go, Python, and Java, but the other language clients have usability and performance challenges. HTTP is ubiquitous.
Ted: we should still ensure that languages support both, even if they have a preference for HTTP or gRPC!
Proposal: maintainers / the TC no longer have a preference between gRPC or HTTP being the default, but OpenTelmetry components should support both.
No objections in the group

To summarize this is what we want to do:

  1. Strongly recommend SDKs to implement both OTLP/gRPC and OTLP/HTTP+Protobuf exporters (SHOULD clause).
  2. Strongly recommend OTLP/HTTP+Protobuf to be the default protocosl (SHOULD clause) but allow language SDKs to choose a different default if they have good reasons (MAY clause).

I am going to rewrite this PR according to the plan above. If there are any objections to the plan please speak up.

@tigrannajaryan tigrannajaryan changed the title Make OTLP/gRPC a required and default transport Make OTLP/HTTP the recommended default transport Oct 5, 2021
@tigrannajaryan tigrannajaryan marked this pull request as ready for review October 5, 2021 14:30
@tigrannajaryan tigrannajaryan force-pushed the default-otlp-transport branch 2 times, most recently from 2b70599 to 90c4a8a Compare October 5, 2021 14:35
specification/protocol/exporter.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers PTAL.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

gRPC has great implementations in C++, Go, Python, and Java, but the other language clients have usability and performance challenges. HTTP is ubiquitous.

I hope this is referring specifically to support for HTTP/2 in these languages. Unary gRPC is a trivial wire format and I think is basically the same to implement as HTTP given a library that supports HTTP/2. I don't know if some languages lack HTTP/2 client libraries, even in 2021, but maybe they do. I just hope people aren't getting confused into thinking gRPC is something very different, which it isn't, it's just modern HTTP like here where we don't use the gRPC library at all, it's more or less equivalent to the code implementing HTTP/protobuf.

https://github.com/open-telemetry/opentelemetry-java/blob/3640eb0129d5611ddbaea86212e27ba240ac8ef6/exporters/otlp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/grpc/GrpcRequestBody.java

The point is the HTTP/2 requirement - while it has become far more usable there are still plenty of situations such as internal load balancing strategies that can make it difficult. HTTP/1 is a better baseline. I just don't want people to be misunderstanding that this is about gRPC, which for unary is just a very small convention on top of HTTP/2, not something that itself would cause performance / usability problems. The Google API proto embedded in the OTLP HTTP/protobuf response probably provides more usability issues ;)

specification/protocol/exporter.md Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Oberon00 commented Oct 5, 2021

gRPC clients might be easy to implement (TIL!) but that is not obvious, and I don't think there is readily available documentation for that? I had the impression that gRPC is meant to be used with Google's toolchain for it (stub generator, ...) and everything else is rather unsupported.
But HTTP/2 vs 1.1 is probably the better argument, yes 👍 Especially considering that some SDKs may choose to support older versions of their technology too.

@Oberon00 Oberon00 added area:configuration Related to configuring the SDK area:sdk Related to the SDK spec:protocol Related to the specification/protocol directory labels Oct 5, 2021
@carlosalberto
Copy link
Contributor

Left a small comment regarding adding a "see below" note for the default value, but otherwise LGTM.

@tigrannajaryan
Copy link
Member Author

Enough approvals, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK area:sdk Related to the SDK spec:protocol Related to the specification/protocol directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default protocol for OTLP exporter selection SDK env
9 participants