open-telemetry / opentelemetry-java

OpenTelemetry Java SDK
https://opentelemetry.io
Apache License 2.0
2.01k stars 835 forks source link

Make the exporter sender a public SPI #6718

Open brunobat opened 2 months ago

brunobat commented 2 months ago

Right now we have the exporter sender SPI under private folders: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/ And https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/

We should allow frameworks to create their own senders and use an official SPI for it.

Describe the solution you'd like Publicly support the SPI, classes and interfaces under the above packages.

Additional context There are many sender implementations already. One example is Quarkus, which has implemented a Vert.x client based sender. Wildfly will use it as well and other application servers will also have the same need.

jasondlee commented 2 months ago

This would be super helpful.

jack-berg commented 2 months ago

I'm not comfortable making these SPIs stable for a couple reasons:

  1. The API for HttpSender, GrpcSender continue to churn. I'm not confident that we've reached the correct API contract and want to preserve the ability to making breaking changes. This is only possible while the SPIs are kept as an internal implementation detail.
  2. I don't want to support OtlpHttp{Signal}Exporter, OtlpGrpc{Signal}Exporter based on HttpSender, GrpcSenders which are not published by this project. We have limited resource capacity in this project and already maintain 2 HttpSender and 2 GrpcSender implementations. An ecosystem where is possible / popular to bring-your-own sender adds more support burden and complexity since its likely that users won't understand this subtlety and will open issues in this repo.
  3. I don't understand when the built-in senders are insufficient given that there's a zero-dependency java HttpClient based implementation of HttpSender for Java 11+. Users would have to be stuck on java 8 AND be unable to use OkHttp or any of the grpc upstream transports.
brunobat commented 2 months ago

Thanks fort the reply, Jack.

  1. Are there any concrete plans to change this API in any way or is this just an hypothesis?
  2. If HttpSender, GrpcSenders are not published by this project, you don't have to support them. Have you received any questions about the Vert.x based senders?
  3. They are insufficient because OkHttp depends on Kotlin, which is a hard stop for most integrators and the JDK HTTP Client is too simplistic. All frameworks have their own optimised clients of choice.
geoand commented 2 months ago

I don't understand when the built-in senders are insufficient given that there's a zero-dependency java HttpClient based implementation of HttpSender for Java 11+. Users would have to be stuck on java 8 AND be unable to use OkHttp or any of the grpc upstream transports.

As the person who wrote the Vert.x senders for Quarkus, I can chime in on this: As Bruno said we did not want OkHttp anywhere on our classpath because it has a dependency on Kotlin. The reason we chose Vert.x instead of some other stack, is because we want our observability stack to have as low overhead as possible on the application. In this specific case, overhead can creep in by loading tons of extra classes (which would otherwise never be loaded) and management of client specific thread pools.

jack-berg commented 2 months ago

Are there any concrete plans to change this API in any way or is this just an hypothesis?

Check out the history in the sender / providers:

The churn in the APIs is very real, and could continue to change with additional enhancements for things like authentication providers, retry configurability, and whatever other features users request.

If HttpSender, GrpcSenders are not published by this project, you don't have to support them. Have you received any questions about the Vert.x based senders?

Users won't know the difference. Can't say whether or not the things we've supported are based on Vert.x senders.

They are insufficient because OkHttp depends on Kotlin, which is a hard stop for most integrators and the JDK HTTP Client is too simplistic. All frameworks have their own optimised clients of choice.

"too simplistic" - not a convincing argument for an implementation detail. You can always implement your own SpanExporter, MetricExporter, LogRecordExporter from the ground up. After all, OTLP is a generic protocol and there's no reason that all implementations in java have to be based on the same stack of tooling.

The reason we chose Vert.x instead of some other stack, is because we want our observability stack to have as low overhead as possible on the application. In this specific case, overhead can creep in by loading tons of extra classes (which would otherwise never be loaded) and management of client specific thread pools.

Why not use the built-in JDK HTTP sender? No extra classes required and it appears quarkus already requries java 11 (or 17).

brunobat commented 2 months ago

The JDK HTTP client has many drawbacks for our case, here are a few:

geoand commented 2 months ago

No extra classes required and it appears quarkus already requries java 11 (or 17).

Sure, but those are never loaded. And the thread pool issue still remain

trask commented 2 months ago

the thread pool issue still remain

OpenTelemetry exporters are batched and single-threaded, can you elaborate on what kind of thread pool issue you are seeing?

geoand commented 2 months ago

The JDK client manages its own Thread Pool and that inevitably leads to some overhead

jack-berg commented 1 month ago

The JDK client manages its own Thread Pool and that inevitably leads to some overhead

JdkHttpSender only sends synchronous requests (i.e. it never calls sendAsync(..)). I assume this means that the calling thread is used to perform the work and no thread pool is created / used. The executor used to execute async requests is configurable, and it looks like if its not configured Executors.newCachedThreadPool(ThreadFactory) is used. I believe this will only create threads if needed, which should be never since we don't use any async methods.