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

add enhanced SDK autoconfig for exporters #6368

Closed

Conversation

zeitlinger
Copy link
Member

In open-telemetry/opentelemetry-java-examples#378 - you can see that it's quite difficult to create exporters programmatically - which also respect all config properties.

This PR tries to make this easier.
For the purpose of discussing the approach, only the span exporter is enhanced - but the approach can be used for metrics and logs, too.

@zeitlinger
Copy link
Member Author

@jack-berg please let me know what you think. I'm open to putting the code somewhere else (or make it package protected to indicate incubating) if the general approach seems useful.

@zeitlinger zeitlinger self-assigned this Apr 9, 2024
@zeitlinger
Copy link
Member Author

@jack-berg
Copy link
Member

In open-telemetry/opentelemetry-java-examples#378 - you can see that it's quite difficult to create exporters programmatically - which also respect all config properties.

I think there's an easier way to accomplish this:

OtlpHttpSpanExporter exporter =
    ((OtlpHttpSpanExporter)
            new OtlpSpanExporterProvider()
                .createExporter(
                    DefaultConfigProperties.create(
                        ImmutableMap.of("otel.exporter.otlp.protocol", "http/protobuf"))))
        .toBuilder().addHeader("Authorization", "Bearer my-token").build();

If this is a pattern we to be common, I'd be more interested in fixing the public API surface area than extending the internal APIs and encouraging their use. For example, we could add an overload for obtaining a builder which is prepopulated with the config from a ConfigProperties instance:

public final class OtlpHttpSpanExporter {
  ..
  public static OtlpHttpSpanExporterBuilder builder(ConfigProperties) { ...}
  ..
}

But why not use AutoConfigurationCustomizer#addSpanExporterCustomizer?

@zeitlinger
Copy link
Member Author

But why not use AutoConfigurationCustomizer#addSpanExporterCustomizer?

I didn't realize that there's a toBuilder method.
I think this could be a good solution:

  void init() {
    AutoConfiguredOpenTelemetrySdkBuilder builder = AutoConfiguredOpenTelemetrySdk.builder();

    builder.addSpanExporterCustomizer(
        (exporter, config) -> ((OtlpHttpSpanExporter) exporter).toBuilder().setHeaders(this::headers).build());
  }

  private Map<String, String> headers() {
    return null;
  }

@psx95
Copy link
Contributor

psx95 commented Apr 18, 2024

But why not use AutoConfigurationCustomizer#addSpanExporterCustomizer?

I didn't realize that there's a toBuilder method. I think this could be a good solution:

  void init() {
    AutoConfiguredOpenTelemetrySdkBuilder builder = AutoConfiguredOpenTelemetrySdk.builder();

    builder.addSpanExporterCustomizer(
        (exporter, config) -> ((OtlpHttpSpanExporter) exporter).toBuilder().setHeaders(this::headers).build());
  }

  private Map<String, String> headers() {
    return null;
  }

Just confirming this here - you would need to check the type of the exporter before type casting it when using AutoConfiguredOpenTelemetrySdk, correct ?

I was doing something similar recently and found that all those if based type checks make the code a little verbose and was wondering if there are ways to avoid the type checks?

@zeitlinger
Copy link
Member Author

zeitlinger commented Apr 18, 2024

I was doing something similar recently and found that all those if based type checks make the code a little verbose and was wondering if there are ways to avoid the type checks?

grpc and http builders have different properties - not sure how you'd avoid the type cast

(all but 3 options are identical - so it would be possible to extract an interface)

@psx95
Copy link
Contributor

psx95 commented Apr 18, 2024

I was doing something similar recently and found that all those if based type checks make the code a little verbose and was wondering if there are ways to avoid the type checks?

grpc and http builders have different properties - not sure how you'd avoid the type cast

(all but 3 options are identical - so it would be possible to extract an interface)

+1 to extracting an interface out of it.

IIUC, the proposed interface would attempt to unify the Otlp{Http|Grpc}SpanExporterBuilders, by moving setter methods for properties common between both builders.

@zeitlinger
Copy link
Member Author

+1 to extracting an interface out of it.

I just tried it out.
It turns out that the span customizer is not only called for OTLP, but also for other (e.g. console), so you have to have a typecast
and typecheck anyways.

Subject: [PATCH] improve logging for http sender
---
Index: exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java b/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java
--- a/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java	(revision b499bdc4ecbc8972fd0f49451e0e757f8a47606d)
+++ b/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java	(date 1713508344606)
@@ -15,6 +15,7 @@
 import io.opentelemetry.exporter.internal.compression.CompressorUtil;
 import io.opentelemetry.exporter.internal.grpc.GrpcExporterBuilder;
 import io.opentelemetry.exporter.internal.otlp.metrics.MetricsRequestMarshaler;
+import io.opentelemetry.exporter.otlp.OtlpExporterBuilder;
 import io.opentelemetry.exporter.otlp.internal.OtlpUserAgent;
 import io.opentelemetry.sdk.common.export.MemoryMode;
 import io.opentelemetry.sdk.common.export.RetryPolicy;
@@ -35,7 +36,7 @@
  *
  * @since 1.14.0
  */
-public final class OtlpGrpcMetricExporterBuilder {
+public final class OtlpGrpcMetricExporterBuilder implements OtlpExporterBuilder<OtlpGrpcMetricExporterBuilder, OtlpGrpcMetricExporter> {
 
   private static final String GRPC_SERVICE_NAME =
       "opentelemetry.proto.collector.metrics.v1.MetricsService";
@@ -102,6 +103,7 @@
    * Sets the maximum time to wait for the collector to process an exported batch of metrics. If
    * unset, defaults to {@value DEFAULT_TIMEOUT_SECS}s.
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setTimeout(long timeout, TimeUnit unit) {
     requireNonNull(unit, "unit");
     checkArgument(timeout >= 0, "timeout must be non-negative");
@@ -113,6 +115,7 @@
    * Sets the maximum time to wait for the collector to process an exported batch of metrics. If
    * unset, defaults to {@value DEFAULT_TIMEOUT_SECS}s.
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setTimeout(Duration timeout) {
     requireNonNull(timeout, "timeout");
     delegate.setTimeout(timeout);
@@ -125,6 +128,7 @@
    *
    * @since 1.36.0
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
     requireNonNull(unit, "unit");
     checkArgument(timeout >= 0, "timeout must be non-negative");
@@ -138,6 +142,7 @@
    *
    * @since 1.36.0
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setConnectTimeout(Duration timeout) {
     requireNonNull(timeout, "timeout");
     return setConnectTimeout(timeout.toNanos(), TimeUnit.NANOSECONDS);
@@ -147,6 +152,7 @@
    * Sets the OTLP endpoint to connect to. If unset, defaults to {@value DEFAULT_ENDPOINT_URL}. The
    * endpoint must start with either http:// or https://.
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setEndpoint(String endpoint) {
     requireNonNull(endpoint, "endpoint");
     delegate.setEndpoint(endpoint);
@@ -158,6 +164,7 @@
    * method "gzip" and "none" are supported out of the box. Support for additional compression
    * methods is available by implementing {@link Compressor} and {@link CompressorProvider}.
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setCompression(String compressionMethod) {
     requireNonNull(compressionMethod, "compressionMethod");
     Compressor compressor = CompressorUtil.validateAndResolveCompressor(compressionMethod);
@@ -170,6 +177,7 @@
    * should contain an X.509 certificate collection in PEM format. If not set, TLS connections will
    * use the system default trusted certificates.
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setTrustedCertificates(byte[] trustedCertificatesPem) {
     delegate.setTrustManagerFromCerts(trustedCertificatesPem);
     return this;
@@ -179,6 +187,7 @@
    * Sets ths client key and the certificate chain to use for verifying client when TLS is enabled.
    * The key must be PKCS8, and both must be in PEM format.
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setClientTls(byte[] privateKeyPem, byte[] certificatePem) {
     delegate.setKeyManagerFromCerts(privateKeyPem, certificatePem);
     return this;
@@ -190,6 +199,7 @@
    *
    * @since 1.26.0
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setSslContext(
       SSLContext sslContext, X509TrustManager trustManager) {
     delegate.setSslContext(sslContext, trustManager);
@@ -206,6 +216,7 @@
    * @param value header value
    * @return this builder's instance
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder addHeader(String key, String value) {
     delegate.addConstantHeader(key, value);
     return this;
@@ -218,6 +229,7 @@
    *
    * @since 1.33.0
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setHeaders(Supplier<Map<String, String>> headerSupplier) {
     delegate.setHeadersSupplier(headerSupplier);
     return this;
@@ -259,6 +271,7 @@
    *
    * @since 1.28.0
    */
+  @Override
   public OtlpGrpcMetricExporterBuilder setRetryPolicy(RetryPolicy retryPolicy) {
     requireNonNull(retryPolicy, "retryPolicy");
     delegate.setRetryPolicy(retryPolicy);
@@ -277,6 +290,7 @@
    *
    * @return a new exporter's instance
    */
+  @Override
   public OtlpGrpcMetricExporter build() {
     return new OtlpGrpcMetricExporter(
         delegate,
Index: exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java b/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java
--- a/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java	(revision b499bdc4ecbc8972fd0f49451e0e757f8a47606d)
+++ b/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java	(date 1713508233794)
@@ -16,6 +16,7 @@
 import io.opentelemetry.exporter.internal.http.HttpExporterBuilder;
 import io.opentelemetry.exporter.internal.otlp.traces.TraceRequestMarshaler;
 import io.opentelemetry.exporter.otlp.internal.OtlpUserAgent;
+import io.opentelemetry.exporter.otlp.OtlpExporterBuilder;
 import io.opentelemetry.sdk.common.export.ProxyOptions;
 import io.opentelemetry.sdk.common.export.RetryPolicy;
 import java.time.Duration;
@@ -30,7 +31,7 @@
  *
  * @since 1.5.0
  */
-public final class OtlpHttpSpanExporterBuilder {
+public final class OtlpHttpSpanExporterBuilder implements OtlpExporterBuilder<OtlpHttpSpanExporterBuilder, OtlpHttpSpanExporter> {
 
   private static final String DEFAULT_ENDPOINT = "http://localhost:4318/v1/traces";
 
@@ -49,6 +50,7 @@
    * Sets the maximum time to wait for the collector to process an exported batch of spans. If
    * unset, defaults to {@value HttpExporterBuilder#DEFAULT_TIMEOUT_SECS}s.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setTimeout(long timeout, TimeUnit unit) {
     requireNonNull(unit, "unit");
     checkArgument(timeout >= 0, "timeout must be non-negative");
@@ -60,6 +62,7 @@
    * Sets the maximum time to wait for the collector to process an exported batch of spans. If
    * unset, defaults to {@value HttpExporterBuilder#DEFAULT_TIMEOUT_SECS}s.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setTimeout(Duration timeout) {
     requireNonNull(timeout, "timeout");
     return setTimeout(timeout.toNanos(), TimeUnit.NANOSECONDS);
@@ -71,6 +74,7 @@
    *
    * @since 1.33.0
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
     requireNonNull(unit, "unit");
     checkArgument(timeout >= 0, "timeout must be non-negative");
@@ -84,6 +88,7 @@
    *
    * @since 1.33.0
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setConnectTimeout(Duration timeout) {
     requireNonNull(timeout, "timeout");
     return setConnectTimeout(timeout.toNanos(), TimeUnit.NANOSECONDS);
@@ -93,6 +98,7 @@
    * Sets the OTLP endpoint to connect to. If unset, defaults to {@value DEFAULT_ENDPOINT}. The
    * endpoint must start with either http:// or https://, and include the full HTTP path.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setEndpoint(String endpoint) {
     requireNonNull(endpoint, "endpoint");
     delegate.setEndpoint(endpoint);
@@ -104,6 +110,7 @@
    * method "gzip" and "none" are supported out of the box. Support for additional compression
    * methods is available by implementing {@link Compressor} and {@link CompressorProvider}.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setCompression(String compressionMethod) {
     requireNonNull(compressionMethod, "compressionMethod");
     Compressor compressor = CompressorUtil.validateAndResolveCompressor(compressionMethod);
@@ -115,6 +122,7 @@
    * Add a constant header to requests. If the {@code key} collides with another constant header
    * name or a one from {@link #setHeaders(Supplier)}, the values from both are included.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder addHeader(String key, String value) {
     delegate.addConstantHeaders(key, value);
     return this;
@@ -126,6 +134,7 @@
    *
    * @since 1.33.0
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setHeaders(Supplier<Map<String, String>> headerSupplier) {
     delegate.setHeadersSupplier(headerSupplier);
     return this;
@@ -136,6 +145,7 @@
    * should contain an X.509 certificate collection in PEM format. If not set, TLS connections will
    * use the system default trusted certificates.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setTrustedCertificates(byte[] trustedCertificatesPem) {
     delegate.setTrustManagerFromCerts(trustedCertificatesPem);
     return this;
@@ -145,6 +155,7 @@
    * Sets ths client key and the certificate chain to use for verifying client when TLS is enabled.
    * The key must be PKCS8, and both must be in PEM format.
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setClientTls(byte[] privateKeyPem, byte[] certificatePem) {
     delegate.setKeyManagerFromCerts(privateKeyPem, certificatePem);
     return this;
@@ -156,6 +167,7 @@
    *
    * @since 1.26.0
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setSslContext(
       SSLContext sslContext, X509TrustManager trustManager) {
     delegate.setSslContext(sslContext, trustManager);
@@ -167,6 +179,7 @@
    *
    * @since 1.28.0
    */
+  @Override
   public OtlpHttpSpanExporterBuilder setRetryPolicy(RetryPolicy retryPolicy) {
     requireNonNull(retryPolicy, "retryPolicy");
     delegate.setRetryPolicy(retryPolicy);
@@ -212,6 +225,7 @@
    *
    * @return a new exporter's instance
    */
+  @Override
   public OtlpHttpSpanExporter build() {
     return new OtlpHttpSpanExporter(delegate, delegate.build());
   }
Index: exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/OtlpExporterBuilder.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/OtlpExporterBuilder.java b/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/OtlpExporterBuilder.java
new file mode 100644
--- /dev/null	(date 1713508197538)
+++ b/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/OtlpExporterBuilder.java	(date 1713508197538)
@@ -0,0 +1,37 @@
+package io.opentelemetry.exporter.otlp;
+
+import io.opentelemetry.sdk.common.export.RetryPolicy;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.X509TrustManager;
+import java.time.Duration;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
+
+public interface OtlpExporterBuilder<B extends OtlpExporterBuilder<?, ?>, E> {
+  B setTimeout(long timeout, TimeUnit unit);
+
+  B setTimeout(Duration timeout);
+
+  B setConnectTimeout(long timeout, TimeUnit unit);
+
+  B setConnectTimeout(Duration timeout);
+
+  B setEndpoint(String endpoint);
+
+  B setCompression(String compressionMethod);
+
+  B addHeader(String key, String value);
+
+  B setHeaders(Supplier<Map<String, String>> headerSupplier);
+
+  B setRetryPolicy(RetryPolicy retryPolicy);
+
+  B setTrustedCertificates(byte[] trustedCertificatesPem);
+
+  B setClientTls(byte[] privateKeyPem, byte[] certificatePem);
+
+  B setSslContext(SSLContext sslContext, X509TrustManager trustManager);
+
+  E build();
+}

@zeitlinger
Copy link
Member Author

@zeitlinger zeitlinger closed this Apr 19, 2024
@zeitlinger zeitlinger deleted the enhanced-sdk-autoconfig branch April 19, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants