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 the exporter sender a public SPI #6718

Open
brunobat opened this issue Sep 12, 2024 · 10 comments
Open

Make the exporter sender a public SPI #6718

brunobat opened this issue Sep 12, 2024 · 10 comments
Labels
Feature Request Suggest an idea for this project

Comments

@brunobat
Copy link
Contributor

brunobat commented Sep 12, 2024

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.

@brunobat brunobat added the Feature Request Suggest an idea for this project label Sep 12, 2024
@jasondlee
Copy link

This would be super helpful.

@jack-berg
Copy link
Member

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
Copy link
Contributor Author

brunobat commented Sep 13, 2024

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
Copy link

geoand commented Sep 13, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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

  • It's inefficient for high throughput loads unlike the reactive Vert.x HTTP client. Requires many additional threads on those cases.
  • We would need to include the JDK HTTP client related classes in the builds, just for this, increasing the footprint. Classes that are not used are excluded by the Quarkus build.
  • We would need to add additional integrations for that client like certificate, thread, fault tolerance management, and others. All this is already implemented and battle tested for the Vert.x HTTP client.
  • We do use GRPC (still default on Quarkus) and HTTP protocols. Doing the 1st with that client is not possible.

@geoand
Copy link

geoand commented Sep 13, 2024

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
Copy link
Member

trask commented Sep 13, 2024

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
Copy link

geoand commented Sep 14, 2024

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

@jack-berg
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

5 participants