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

Replacing the okhttp based sender using JDK autoconfigure #5958

Closed
fax4ever opened this issue Nov 2, 2023 · 6 comments · Fixed by #6110
Closed

Replacing the okhttp based sender using JDK autoconfigure #5958

fax4ever opened this issue Nov 2, 2023 · 6 comments · Fixed by #6110
Labels
Feature Request Suggest an idea for this project

Comments

@fax4ever
Copy link
Contributor

fax4ever commented Nov 2, 2023

Is your feature request related to a problem? Please describe.

We would like to replace the okhttp based sender. It seems to be possible replacing the usages of opentelemetry-exporter-sender-okhttp with opentelemetry-exporter-sender-jdk and opentelemetry-exporter-sender-grpc-managed-channel.

All work if create the SDK manually and setting the channel:

ManagedChannel managedChannel = ManagedChannelBuilder.forAddress("localhost", 4317).usePlaintext().build();
OtlpGrpcSpanExporter otlpGrpcExporter = OtlpGrpcSpanExporter.builder()
            .setChannel(managedChannel)
            .build();

What if the OpenTelemetry SDK is created by the autoconfigure project?

Describe the solution you'd like

Find a way to replace the okhttp based sender with the SDK autoconfigure.

Describe alternatives you've considered

I tried to add a SpanExporterCustomizer without success.

Additional context

Hi tried to do it here: https://github.com/fax4ever/telemetry-play/blob/main/sdk-autoconfigure-exporter/src/main/java/fax/play/OTelConfig.java.

@fax4ever fax4ever added the Feature Request Suggest an idea for this project label Nov 2, 2023
@jack-berg
Copy link
Member

It seems to be possible replacing the usages of opentelemetry-exporter-sender-okhttp with opentelemetry-exporter-sender-jdk and opentelemetry-exporter-sender-grpc-managed-channel.

That's correct. Based on your source code, you're using the gRPC version. The only alternative to the okhttp implementation for gRPC is opentelemetry-exporter-sender-grpc-managed-channel. To use, include the artifact (and exclude the opentelemetry-exporter-sender-okhttp, then call setChannel.

If you want to do this with autoconfigure, you'll want to provide an implementation of AutoConfigurationCustomizerProvider, and call addSpanExporterCustomizer, something like:

@AutoService(AutoConfigurationCustomizerProvider.class)
public class FooCustomizer implements AutoConfigurationCustomizerProvider {
  @Override
  public void customize(AutoConfigurationCustomizer autoConfiguration) {
    autoConfiguration.addSpanExporterCustomizer(
            (spanExporter, configProperties) -> {
              if (spanExporter instanceof OtlpGrpcSpanExporter) {
                spanExporter.shutdown().join(10, TimeUnit.SECONDS);
                return ((OtlpGrpcSpanExporter) spanExporter).toBuilder().setChannel(...).build();
              }
            });
  }
}

Can also call do this programmatically if you're manually invoking AutoConfiguredOpenTelemetrySdk:

    AutoConfiguredOpenTelemetrySdk.builder()
            .addSpanExporterCustomizer(((spanExporter, configProperties) -> {
              ...
            }))
            .build(); 

@jack-berg
Copy link
Member

I tried to add a SpanExporterCustomizer without success.

Oh I see you tried my recommendation above. What was the problem?

@fax4ever
Copy link
Contributor Author

fax4ever commented Nov 6, 2023

It seems to be possible replacing the usages of opentelemetry-exporter-sender-okhttp with opentelemetry-exporter-sender-jdk and opentelemetry-exporter-sender-grpc-managed-channel.

That's correct. Based on your source code, you're using the gRPC version. The only alternative to the okhttp implementation for gRPC is opentelemetry-exporter-sender-grpc-managed-channel. To use, include the artifact (and exclude the opentelemetry-exporter-sender-okhttp, then call setChannel.

If you want to do this with autoconfigure, you'll want to provide an implementation of AutoConfigurationCustomizerProvider, and call addSpanExporterCustomizer, something like:

@AutoService(AutoConfigurationCustomizerProvider.class)
public class FooCustomizer implements AutoConfigurationCustomizerProvider {
  @Override
  public void customize(AutoConfigurationCustomizer autoConfiguration) {
    autoConfiguration.addSpanExporterCustomizer(
            (spanExporter, configProperties) -> {
              if (spanExporter instanceof OtlpGrpcSpanExporter) {
                spanExporter.shutdown().join(10, TimeUnit.SECONDS);
                return ((OtlpGrpcSpanExporter) spanExporter).toBuilder().setChannel(...).build();
              }
            });
  }
}

Can also call do this programmatically if you're manually invoking AutoConfiguredOpenTelemetrySdk:

    AutoConfiguredOpenTelemetrySdk.builder()
            .addSpanExporterCustomizer(((spanExporter, configProperties) -> {
              ...
            }))
            .build(); 

Thanks @jack-berg I'll try to do that

@fax4ever
Copy link
Contributor Author

fax4ever commented Nov 8, 2023

I tried to add a SpanExporterCustomizer without success.

Oh I see you tried my recommendation above. What was the problem?

Probably I'm doing something wrong. I think I tried both the options here => https://github.com/fax4ever/telemetry-play/tree/main/sdk-autoconfigure-exporter

@jack-berg
Copy link
Member

I finally got around to taking a closer look at this an you are totally right that something is wrong:

  • If you include opentelemetry-exporter-sender-grpc-managed-channel and exclude opentelemetry-exporter-sender-okhttp, autoconfigure will fail to initialize because opentelemetry-exporter-sender-grpc-managed-channel expects the user to call setChannel, and there's no chance to do that before the span exporter customizer gets called, so we get a NPE.
  • The NPE occurs because UpstreamGrpcSenderProvider doesn't have any way to accommodate a null managedChannel argument.
  • I'm pushing a fix in Use minimal fallback managed channel when none is specified #6110 which updates UpstreamGrpcSenderProvider to initialize a minimal managed channel is none is explicitly set by the user. This will allow autoconfigure to initialize without error, but still requires the user to use a span exporter customer to customize any of the options of the managed channel.
  • I pushed a commit which demonstrates the end to end usage. Note this depends on #Use minimal fallback managed channel when none is specified #6110 being merged.

@fax4ever
Copy link
Contributor Author

fax4ever commented Jan 7, 2024

I finally got around to taking a closer look at this an you are totally right that something is wrong:

  • If you include opentelemetry-exporter-sender-grpc-managed-channel and exclude opentelemetry-exporter-sender-okhttp, autoconfigure will fail to initialize because opentelemetry-exporter-sender-grpc-managed-channel expects the user to call setChannel, and there's no chance to do that before the span exporter customizer gets called, so we get a NPE.
  • The NPE occurs because UpstreamGrpcSenderProvider doesn't have any way to accommodate a null managedChannel argument.
  • I'm pushing a fix in Use minimal fallback managed channel when none is specified #6110 which updates UpstreamGrpcSenderProvider to initialize a minimal managed channel is none is explicitly set by the user. This will allow autoconfigure to initialize without error, but still requires the user to use a span exporter customer to customize any of the options of the managed channel.
  • I pushed a commit which demonstrates the end to end usage. Note this depends on #Use minimal fallback managed channel when none is specified #6110 being merged.

Thank you @jack-berg. It does make sense to me.

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.

2 participants