Address the comments from the reviews

Hinge · Dec 16, 2024 · 5c4f0e5 · 5c4f0e5
1 parent ce6d6f7
commit 5c4f0e5
Show file tree

Hide file tree

Showing 6 changed files with 279 additions and 207 deletions.
diff --git a/core/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java b/core/src/main/java/io/opentelemetry/android/OpenTelemetryRumBuilder.java
@@ -96,13 +96,11 @@ public final class OpenTelemetryRumBuilder {
 
     private Resource resource;
 
-    private final Object lock = new Object();
+    private boolean isBuilt = false;
 
-    // Writes guarded by "lock"
-    @Nullable private volatile ServiceManager serviceManager;
+    @Nullable private ServiceManager serviceManager;
 
-    // Writes guarded by "lock"
-    @Nullable private volatile ExportScheduleHandler exportScheduleHandler;
+    @Nullable private ExportScheduleHandler exportScheduleHandler;
 
     private static TextMapPropagator buildDefaultPropagator() {
         return TextMapPropagator.composite(
@@ -129,6 +127,7 @@ public static OpenTelemetryRumBuilder create(Application application, OtelRumCon
      * @return {@code this}
      */
     public OpenTelemetryRumBuilder setResource(Resource resource) {
+        checkNotBuilt();
         this.resource = resource;
         return this;
     }
@@ -141,6 +140,7 @@ public OpenTelemetryRumBuilder setResource(Resource resource) {
      * @return {@code this}
      */
     public OpenTelemetryRumBuilder mergeResource(Resource resource) {
+        checkNotBuilt();
         this.resource = this.resource.merge(resource);
         return this;
     }
@@ -180,6 +180,7 @@ public OpenTelemetryRumBuilder addTracerProviderCustomizer(
      */
     public OpenTelemetryRumBuilder addMeterProviderCustomizer(
             BiFunction<SdkMeterProviderBuilder, Application, SdkMeterProviderBuilder> customizer) {
+        checkNotBuilt();
         meterProviderCustomizers.add(customizer);
         return this;
     }
@@ -200,6 +201,7 @@ public OpenTelemetryRumBuilder addMeterProviderCustomizer(
     public OpenTelemetryRumBuilder addLoggerProviderCustomizer(
             BiFunction<SdkLoggerProviderBuilder, Application, SdkLoggerProviderBuilder>
                     customizer) {
+        checkNotBuilt();
         loggerProviderCustomizers.add(customizer);
         return this;
     }
@@ -211,6 +213,7 @@ public OpenTelemetryRumBuilder addLoggerProviderCustomizer(
      */
     public OpenTelemetryRumBuilder addInstrumentation(AndroidInstrumentation instrumentation) {
         instrumentations.add(instrumentation);
+        checkNotBuilt();
         return this;
     }
 
@@ -225,6 +228,7 @@ public OpenTelemetryRumBuilder addInstrumentation(AndroidInstrumentation instrum
     public OpenTelemetryRumBuilder addPropagatorCustomizer(
             Function<? super TextMapPropagator, ? extends TextMapPropagator> propagatorCustomizer) {
         requireNonNull(propagatorCustomizer, "propagatorCustomizer");
+        checkNotBuilt();
         Function<? super TextMapPropagator, ? extends TextMapPropagator> existing =
                 this.propagatorCustomizer;
         this.propagatorCustomizer =
@@ -244,6 +248,7 @@ public OpenTelemetryRumBuilder addPropagatorCustomizer(
     public OpenTelemetryRumBuilder addSpanExporterCustomizer(
             Function<? super SpanExporter, ? extends SpanExporter> spanExporterCustomizer) {
         requireNonNull(spanExporterCustomizer, "spanExporterCustomizer");
+        checkNotBuilt();
         Function<? super SpanExporter, ? extends SpanExporter> existing =
                 this.spanExporterCustomizer;
         this.spanExporterCustomizer =
@@ -263,6 +268,7 @@ public OpenTelemetryRumBuilder addSpanExporterCustomizer(
     public OpenTelemetryRumBuilder addLogRecordExporterCustomizer(
             Function<? super LogRecordExporter, ? extends LogRecordExporter>
                     logRecordExporterCustomizer) {
+        checkNotBuilt();
         Function<? super LogRecordExporter, ? extends LogRecordExporter> existing =
                 this.logRecordExporterCustomizer;
         this.logRecordExporterCustomizer =
@@ -283,6 +289,10 @@ public OpenTelemetryRumBuilder addLogRecordExporterCustomizer(
      * @return A new {@link OpenTelemetryRum} instance.
      */
     public OpenTelemetryRum build() {
+        if (isBuilt) {
+            throw new IllegalStateException("You cannot call build multiple times");
+        }
+        isBuilt = true;
         InitializationEvents initializationEvents = InitializationEvents.get();
         applyConfiguration(initializationEvents);
 
@@ -373,21 +383,16 @@ private void initializeExporters(
     @NonNull
     private ServiceManager getServiceManager() {
         if (serviceManager == null) {
-            synchronized (lock) {
-                if (serviceManager == null) {
-                    serviceManager = ServiceManagerImpl.Companion.create(application);
-                }
-            }
+            serviceManager = ServiceManagerImpl.Companion.create(application);
         }
         // This can never be null since we never write `null` to it
         return requireNonNull(serviceManager);
     }
 
     public OpenTelemetryRumBuilder setServiceManager(@NonNull ServiceManager serviceManager) {
         requireNonNull(serviceManager, "serviceManager cannot be null");
-        synchronized (lock) {
-            this.serviceManager = serviceManager;
-        }
+        checkNotBuilt();
+        this.serviceManager = serviceManager;
         return this;
     }
 
@@ -398,9 +403,8 @@ public OpenTelemetryRumBuilder setServiceManager(@NonNull ServiceManager service
     public OpenTelemetryRumBuilder setExportScheduleHandler(
             @NonNull ExportScheduleHandler exportScheduleHandler) {
         requireNonNull(exportScheduleHandler, "exportScheduleHandler cannot be null");
-        synchronized (lock) {
-            this.exportScheduleHandler = exportScheduleHandler;
-        }
+        checkNotBuilt();
+        this.exportScheduleHandler = exportScheduleHandler;
         return this;
     }
 
@@ -423,18 +427,13 @@ private StorageConfiguration createStorageConfiguration() throws IOException {
 
     private void scheduleDiskTelemetryReader(@Nullable SignalFromDiskExporter signalExporter) {
         if (exportScheduleHandler == null) {
-            synchronized (lock) {
-                if (exportScheduleHandler == null) {
-                    ServiceManager serviceManager = getServiceManager();
-                    // TODO: Is it safe to get the work service yet here? If so, we can
-                    // avoid all this lazy supplier stuff....
-                    Function0<PeriodicWorkService> getWorkService =
-                            serviceManager::getPeriodicWorkService;
-                    exportScheduleHandler =
-                            new DefaultExportScheduleHandler(
-                                    new DefaultExportScheduler(getWorkService), getWorkService);
-                }
-            }
+            ServiceManager serviceManager = getServiceManager();
+            // TODO: Is it safe to get the work service yet here? If so, we can
+            // avoid all this lazy supplier stuff....
+            Function0<PeriodicWorkService> getWorkService = serviceManager::getPeriodicWorkService;
+            exportScheduleHandler =
+                    new DefaultExportScheduleHandler(
+                            new DefaultExportScheduler(getWorkService), getWorkService);
         }
 
         final ExportScheduleHandler exportScheduleHandler =
@@ -461,6 +460,7 @@ private void scheduleDiskTelemetryReader(@Nullable SignalFromDiskExporter signal
      * @return this
      */
     public OpenTelemetryRumBuilder addOtelSdkReadyListener(Consumer<OpenTelemetrySdk> callback) {
+        checkNotBuilt();
         otelSdkReadyListeners.add(callback);
         return this;
     }
@@ -574,4 +574,10 @@ private ContextPropagators buildFinalPropagators() {
         TextMapPropagator defaultPropagator = buildDefaultPropagator();
         return ContextPropagators.create(propagatorCustomizer.apply(defaultPropagator));
     }
+
+    private void checkNotBuilt() {
+        if (isBuilt) {
+            throw new IllegalStateException("This method cannot be called after calling build");
+        }
+    }
 }
diff --git a/core/src/main/java/io/opentelemetry/android/export/BufferDelegatingLogExporter.kt b/core/src/main/java/io/opentelemetry/android/export/BufferDelegatingLogExporter.kt
@@ -17,19 +17,22 @@ import io.opentelemetry.sdk.logs.export.LogRecordExporter
  */
 internal class BufferDelegatingLogExporter(
     maxBufferedLogs: Int = 5_000,
-) : BufferedDelegatingExporter<LogRecordData, LogRecordExporter>(bufferedSignals = maxBufferedLogs),
-    LogRecordExporter {
-    override fun exportToDelegate(
-        delegate: LogRecordExporter,
-        data: Collection<LogRecordData>,
-    ): CompletableResultCode = delegate.export(data)
+) : LogRecordExporter {
+    private val delegatingExporter =
+        DelegatingExporter<LogRecordExporter, LogRecordData>(
+            doExport = LogRecordExporter::export,
+            doFlush = LogRecordExporter::flush,
+            doShutdown = LogRecordExporter::shutdown,
+            maxBufferedData = maxBufferedLogs,
+        )
 
-    override fun shutdownDelegate(delegate: LogRecordExporter): CompletableResultCode = delegate.shutdown()
+    fun setDelegate(delegate: LogRecordExporter) {
+        delegatingExporter.setDelegate(delegate)
+    }
 
-    override fun export(logs: Collection<LogRecordData>): CompletableResultCode = bufferOrDelegate(logs)
+    override fun export(logs: Collection<LogRecordData>): CompletableResultCode = delegatingExporter.export(logs)
 
-    override fun flush(): CompletableResultCode =
-        withDelegateOrNull { delegate ->
-            delegate?.flush() ?: CompletableResultCode.ofSuccess()
-        }
+    override fun flush(): CompletableResultCode = delegatingExporter.flush()
+
+    override fun shutdown(): CompletableResultCode = delegatingExporter.shutdown()
 }
diff --git a/core/src/main/java/io/opentelemetry/android/export/BufferDelegatingSpanExporter.kt b/core/src/main/java/io/opentelemetry/android/export/BufferDelegatingSpanExporter.kt
@@ -17,19 +17,22 @@ import io.opentelemetry.sdk.trace.export.SpanExporter
  */
 internal class BufferDelegatingSpanExporter(
     maxBufferedSpans: Int = 5_000,
-) : BufferedDelegatingExporter<SpanData, SpanExporter>(bufferedSignals = maxBufferedSpans),
-    SpanExporter {
-    override fun exportToDelegate(
-        delegate: SpanExporter,
-        data: Collection<SpanData>,
-    ): CompletableResultCode = delegate.export(data)
+) : SpanExporter {
+    private val delegatingExporter =
+        DelegatingExporter<SpanExporter, SpanData>(
+            doExport = SpanExporter::export,
+            doFlush = SpanExporter::flush,
+            doShutdown = SpanExporter::shutdown,
+            maxBufferedData = maxBufferedSpans,
+        )
 
-    override fun shutdownDelegate(delegate: SpanExporter): CompletableResultCode = delegate.shutdown()
+    fun setDelegate(delegate: SpanExporter) {
+        delegatingExporter.setDelegate(delegate)
+    }
 
-    override fun export(spans: Collection<SpanData>): CompletableResultCode = bufferOrDelegate(spans)
+    override fun export(spans: Collection<SpanData>): CompletableResultCode = delegatingExporter.export(spans)
 
-    override fun flush(): CompletableResultCode =
-        withDelegateOrNull { delegate ->
-            delegate?.flush() ?: CompletableResultCode.ofSuccess()
-        }
+    override fun flush(): CompletableResultCode = delegatingExporter.flush()
+
+    override fun shutdown(): CompletableResultCode = delegatingExporter.shutdown()
 }
diff --git a/core/src/main/java/io/opentelemetry/android/export/BufferedDelegatingExporter.kt b/core/src/main/java/io/opentelemetry/android/export/BufferedDelegatingExporter.kt