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

Should we provide jdk11 version of OTLP http exporters? #5351

Closed
jack-berg opened this issue Apr 5, 2023 · 13 comments · Fixed by #5557
Closed

Should we provide jdk11 version of OTLP http exporters? #5351

jack-berg opened this issue Apr 5, 2023 · 13 comments · Fixed by #5557
Labels
Feature Request Suggest an idea for this project

Comments

@jack-berg
Copy link
Member

Related issues: #4777

@brunobat brings up in CNCF slack that OkHttp is problematic because older versions may have unpatched CVEs, while newer versions have a dependency on kotlin stdlib.

In #5341 I've prototyped an approach where we publish a multi-release jar where in which the java11+ version uses the jdk 11+ Http Client. After some initial concerns about performance, I've been able to iron out much of the problem by replacing some of the default implementation of BodyPublisher.

But additional work is needed to get the prototype in a place to merge.

Before I do that work, I want to answer a few key questions:

  1. Should we provide an OTLP exporter option which doesn't rely on dependencies?
  2. If yes, how should we deliver that solution?
    a. One option is to do the multi-release jar prototyped in Promote log API / SDK to stable #5341. Users would need to explicitly exclude the okhttp dependency using gradle exclude syntax.
    b. Another option is to provide dedicated "sender" or "tranasport" artifacts representing the different implementations. I.e. publish new opentelemetry-exporter-jdk-http-sender and opentelemetry-exporter-okhttp-sender. We could then include the okhttp sender as a default dependency of opentelemetry-exporter-otlp. Or we could force users to choose one, and document that when you add opentelemetry-exporter-otlp, you must also add a opentelemetry-exporter-*-sender.

Want to gather feedback before I go any further working on this. I think the #5341 prototype makes it clear that its possible and reasonable to have a zero dependency otlp http exporter. Now we need to decide whether we to deliver and maintain that.

@jack-berg jack-berg added the Feature Request Suggest an idea for this project label Apr 5, 2023
@io7m
Copy link

io7m commented May 3, 2023

A common way to handle this is to define an interface, have various implementations implement it, and then load an implementation using ServiceLoader. This is explicitly supported by the module system (uses and provides).

At a very high level, you'd have something like:

A module io.opentelemetry.http.spi defining the interface of "things that can provide HTTP clients" but no actual executable code:

interface HTTPClient {
  // Whatever methods are needed for OTLP
}

interface HTTPClientProvider {
  HTTPClient create();
}

Then a module io.opentelemetry.http.jdk that provides an implementation of HTTPClient using the JDK 11+ client. Additionally, a module io.opentelemetry.http.okhttp3 that provides an alternative implementation of the HTTP client using okhttp3. Both modules would include an entry in their module descriptors such as:

module io.opentelemetry.http.jdk {
  provides io.opentelemetry.http.spi.HttpClient
    with io.opentelemetry.http.jdk.JDKHttpClients; // ... Or whatever class name is used.
}

The OLTP implementation then declares a dependency solely on io.opentelemetry.http.spi, and uses ServiceLoader to ask for an implementation of HTTPClientProvider. An implementation will be automatically chosen (somewhat arbitrarily) from whatever modules are present on the module path that claim they provide an implementation. The person assembling the application is responsible for making sure that at least one implementation is available, by ensuring that either io.opentelemetry.http.jdk or io.opentelemetry.http.okhttp3 are on the module path at runtime.
Obviously OLTP would be responsible for providing a useful error message if the users have failed to do this.

@io7m
Copy link

io7m commented May 3, 2023

The nice thing about using services is that it makes the dependency explicit whilst also leaving users free to pick which dependency they want. The service declarations are also recognized by GraalVM and jlink and so on.

@io7m
Copy link

io7m commented May 3, 2023

This is also assuming that you actually want to continue to have multiple http client implementations. I'm not sure if that was determined or not.

@brunobat
Copy link
Contributor

brunobat commented May 3, 2023

@io7m there are retry requirements and the need to use a provided serializer

@io7m
Copy link

io7m commented May 3, 2023

@io7m there are retry requirements and the need to use a provided serializer

I can't speak to the actual implementation of the client as I don't know the OLTP codebase well enough. I just wanted to explain that there's a fairly standard way to deliver something like this (where you have users picking between optional dependencies) that's supported by the module system. That was question 2 on the list above. 🙂

@jack-berg
Copy link
Member Author

After further consideration I think the best approach will be to create multiple http sender artifacts, one for okhttp and one for jdk http client, and require that one be present in order to use the OTLP http exporters. The implementation of this is more clear than the multi version jar approach, and means that users are including / excluding opentelemetry artifacts in order to achieve the desired behavior rather than okhttp dependencies.

I propose we:

  • Add a new HttpSender interface to opentelemetry-exporter-common. This will contain the generic contract to send an http request and process the response, including retry logic.
  • Add a new HttpSenderProvider SPI interface to opentelemetry-exporter-common. This SPI will be used to find / instantiate the HttpSender on the classpath.
  • Start publishing a new opentelemetry-exporter-http-sender-okhttp artifact, containing the OkHttp based implementation of HttpSender.
  • Start publishing a new opentelemetry-exporter-http-sender-jdk artifact, containing the jdk 11 http client based implementation of HttpSender.

We can decide whether opentelemetry-exporter-otlp should include one of the senders as a dependency by default, or force the user to choose one. I'm in favor of forcing the user to choose one so its very explicit. This would result in dependencies like:

implementation("io.opentelemetry:opentelemetry-exporter-otlp")
implementation("io.opentelemetry:opentelemetry-exporter-http-sender-jdk")
// or alternatively...
// implementation("io.opentelemetry:opentelemetry-exporter-http-sender-okhttp")

The otel java agent would add a dependency on the opentelemetry-exporter-http-sender-okhttp to maintain java 8 compatibility, and since the okhttp dependency isn't a problem in the agent.

As mentioned in the java sig, we suspect the majority of SDK users would add a dependency on opentelemetry-exporter-http-sender-jdk because:

  • Using the SDK directly means that the application is actively maintained, since it requires modification of source code / dependencies
  • Actively maintained projects are likely to be upgraded to java 11+ and have access to jdk http client. If jdk http client is available and has acceptable performance, it will likely be preferred since it avoids extra dependencies.
  • The projects that are likely stuck on java 8 are those which not being actively developed, which will prefer the java agent approach.

I'm working on implementing this plan locally and will deliver it incrementally to make review more manageable. Please let me know if you have strong feelings against this approach.

@bogdandrutu
Copy link
Member

What is the lifetime of java8? Since OkHTTP and other major libraries dropped support for java8 have you considered to do the same?

@jack-berg
Copy link
Member Author

Removing java 8 support would mean removing java 8 support for the otel java agent, which would be extremely unpopular given that the java agent is popular among legacy apps.

@mateuszrzeszutek
Copy link
Member

Since OkHTTP and other major libraries dropped support for java8 have you considered to do the same?

OkHttp still supports Java 8 though.

Android still uses Java 8 level APIs, and I'm not sure whether that's likely to change -- probably not, Kotlin's supposed to bridge that gap. If we drop Java 8 we might run into issues with Android support.

@brunobat
Copy link
Contributor

I added a comment on the PR because I think we should consider only allowing one HttpSender, to avoid mistakes.

@geoand
Copy link

geoand commented Jul 7, 2023

I think this is a great way to embrace the newer versions of the JDK.

It makes me wonder, is there a plan to do JDK version of GrpcExporter as well?

@jack-berg
Copy link
Member Author

It makes me wonder, is there a plan to do JDK version of GrpcExporter as well?

We would love to but the JDK HttpClient doesn't support trailing headers, which are required for gRPC 😞

@geoand
Copy link

geoand commented Jul 7, 2023

I did not know that, very interesting, thanks!

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

Successfully merging a pull request may close this issue.

6 participants