From 76d461bc29aa0ed5c0f53aa144e8bd6dff80c7ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar?= <56847527+LikeTheSalad@users.noreply.github.com> Date: Mon, 12 Aug 2024 09:42:04 +0200 Subject: [PATCH] Instrumentation registry changes (#516) * Moving AndroidInstrumentationRegistryImpl to an internal package * Removing register method from AndroidInstrumentationServices * Updating tests * Updating docs * Updating docs * Convenience method AndroidInstrumentationServices.getService * Renaming AndroidInstrumentationServicesImpl.register to registerForTest * Renaming AndroidInstrumentationServices to AndroidInstrumentationLoader * Renaming vars * Renaming AndroidInstrumentationLoader.Companion.getInstrumentation --- .../android/agent/OtelRumConfigExtensions.kt | 18 ++--- .../android/SdkPreconfiguredRumBuilder.kt | 4 +- .../instrumentation/AndroidInstrumentation.kt | 3 - ...try.kt => AndroidInstrumentationLoader.kt} | 31 ++++---- .../AndroidInstrumentationLoaderImpl.kt} | 14 ++-- .../android/OpenTelemetryRumBuilderTest.java | 13 ++-- ...kt => AndroidInstrumentationLoaderTest.kt} | 6 +- .../AndroidInstrumentationRegistryImplTest.kt | 72 ------------------- .../AndroidInstrumentationLoaderImplTest.kt | 40 +++++++++++ 9 files changed, 89 insertions(+), 112 deletions(-) rename core/src/main/java/io/opentelemetry/android/instrumentation/{AndroidInstrumentationRegistry.kt => AndroidInstrumentationLoader.kt} (51%) rename core/src/main/java/io/opentelemetry/android/{instrumentation/AndroidInstrumentationRegistryImpl.kt => internal/instrumentation/AndroidInstrumentationLoaderImpl.kt} (61%) rename core/src/test/java/io/opentelemetry/android/instrumentation/{AndroidInstrumentationRegistryTest.kt => AndroidInstrumentationLoaderTest.kt} (60%) delete mode 100644 core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt create mode 100644 core/src/test/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImplTest.kt diff --git a/android-agent/src/main/kotlin/io/opentelemetry/android/agent/OtelRumConfigExtensions.kt b/android-agent/src/main/kotlin/io/opentelemetry/android/agent/OtelRumConfigExtensions.kt index 9bf17d895..c3a011208 100644 --- a/android-agent/src/main/kotlin/io/opentelemetry/android/agent/OtelRumConfigExtensions.kt +++ b/android-agent/src/main/kotlin/io/opentelemetry/android/agent/OtelRumConfigExtensions.kt @@ -6,7 +6,7 @@ package io.opentelemetry.android.agent import io.opentelemetry.android.config.OtelRumConfig -import io.opentelemetry.android.instrumentation.AndroidInstrumentationRegistry +import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader import io.opentelemetry.android.instrumentation.activity.ActivityLifecycleInstrumentation import io.opentelemetry.android.instrumentation.anr.AnrInstrumentation import io.opentelemetry.android.instrumentation.common.ScreenNameExtractor @@ -32,49 +32,49 @@ import java.time.Duration */ fun OtelRumConfig.setActivityTracerCustomizer(customizer: (Tracer) -> Tracer): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(ActivityLifecycleInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(ActivityLifecycleInstrumentation::class.java) ?.setTracerCustomizer(customizer) return this } fun OtelRumConfig.setActivityNameExtractor(screenNameExtractor: ScreenNameExtractor): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(ActivityLifecycleInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(ActivityLifecycleInstrumentation::class.java) ?.setScreenNameExtractor(screenNameExtractor) return this } fun OtelRumConfig.setFragmentTracerCustomizer(customizer: (Tracer) -> Tracer): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(FragmentLifecycleInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(FragmentLifecycleInstrumentation::class.java) ?.setTracerCustomizer(customizer) return this } fun OtelRumConfig.setFragmentNameExtractor(screenNameExtractor: ScreenNameExtractor): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(FragmentLifecycleInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(FragmentLifecycleInstrumentation::class.java) ?.setScreenNameExtractor(screenNameExtractor) return this } fun OtelRumConfig.addAnrAttributesExtractor(extractor: AttributesExtractor, Void>): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(AnrInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(AnrInstrumentation::class.java) ?.addAttributesExtractor(extractor) return this } fun OtelRumConfig.addCrashAttributesExtractor(extractor: AttributesExtractor): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(CrashReporterInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(CrashReporterInstrumentation::class.java) ?.addAttributesExtractor(extractor) return this } fun OtelRumConfig.addNetworkChangeAttributesExtractor(extractor: AttributesExtractor): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(NetworkChangeInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(NetworkChangeInstrumentation::class.java) ?.addAttributesExtractor(extractor) return this } fun OtelRumConfig.setSlowRenderingDetectionPollInterval(interval: Duration): OtelRumConfig { - AndroidInstrumentationRegistry.get().get(SlowRenderingInstrumentation::class.java) + AndroidInstrumentationLoader.getInstrumentation(SlowRenderingInstrumentation::class.java) ?.setSlowRenderingDetectionPollInterval(interval) return this } diff --git a/core/src/main/java/io/opentelemetry/android/SdkPreconfiguredRumBuilder.kt b/core/src/main/java/io/opentelemetry/android/SdkPreconfiguredRumBuilder.kt index 5ab869129..9b1894313 100644 --- a/core/src/main/java/io/opentelemetry/android/SdkPreconfiguredRumBuilder.kt +++ b/core/src/main/java/io/opentelemetry/android/SdkPreconfiguredRumBuilder.kt @@ -7,7 +7,7 @@ package io.opentelemetry.android import android.app.Application import io.opentelemetry.android.instrumentation.AndroidInstrumentation -import io.opentelemetry.android.instrumentation.AndroidInstrumentationRegistry +import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader import io.opentelemetry.android.internal.services.ServiceManager import io.opentelemetry.android.internal.services.applifecycle.AppLifecycleService import io.opentelemetry.sdk.OpenTelemetrySdk @@ -68,7 +68,7 @@ class SdkPreconfiguredRumBuilder private fun getInstrumentations(): List { if (discoverInstrumentations) { - instrumentations.addAll(AndroidInstrumentationRegistry.get().getAll()) + instrumentations.addAll(AndroidInstrumentationLoader.get().getAll()) } return instrumentations diff --git a/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt b/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt index 3f4c41e07..bc3727185 100644 --- a/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt +++ b/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt @@ -22,9 +22,6 @@ import io.opentelemetry.android.OpenTelemetryRum * Even though users shouldn't have to write code to make an AndroidInstrumentation implementation work, * implementations should expose configurable options whenever possible to allow users to customize relevant * options depending on the use-case. - * - * Access to an implementation, either to configure it or to install it, must be made through - * [AndroidInstrumentationRegistry.get] or [AndroidInstrumentationRegistry.getAll]. */ interface AndroidInstrumentation { /** diff --git a/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistry.kt b/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationLoader.kt similarity index 51% rename from core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistry.kt rename to core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationLoader.kt index 45d66bec0..480dc95c4 100644 --- a/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistry.kt +++ b/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationLoader.kt @@ -5,17 +5,19 @@ package io.opentelemetry.android.instrumentation +import io.opentelemetry.android.internal.instrumentation.AndroidInstrumentationLoaderImpl + /** - * Stores and provides all the available instrumentations. + * Loads and provides [AndroidInstrumentation] instances from the runtime classpath. */ -interface AndroidInstrumentationRegistry { +interface AndroidInstrumentationLoader { /** * Provides a single instrumentation if available. * * @param type The type of the instrumentation to retrieve. * @return The instrumentation instance if available, null otherwise. */ - fun get(type: Class): T? + fun getByType(type: Class): T? /** * Provides all registered instrumentations. @@ -24,26 +26,25 @@ interface AndroidInstrumentationRegistry { */ fun getAll(): Collection - /** - * Stores an instrumentation as long as there is not other instrumentation already registered with the same - * type. - * - * @param instrumentation The instrumentation to register. - * @throws IllegalStateException If the instrumentation couldn't be registered. - */ - fun register(instrumentation: AndroidInstrumentation) - companion object { - private var instance: AndroidInstrumentationRegistry? = null + private var instance: AndroidInstrumentationLoader? = null @JvmStatic - fun get(): AndroidInstrumentationRegistry { + fun get(): AndroidInstrumentationLoader { if (instance == null) { - instance = AndroidInstrumentationRegistryImpl() + instance = AndroidInstrumentationLoaderImpl() } return instance!! } + /** + * Convenience method for [AndroidInstrumentationLoader.getByType]. + */ + @JvmStatic + fun getInstrumentation(type: Class): T? { + return get().getByType(type) + } + @JvmStatic fun resetForTest() { instance = null diff --git a/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImpl.kt b/core/src/main/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImpl.kt similarity index 61% rename from core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImpl.kt rename to core/src/main/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImpl.kt index e265f46e7..cd4d91b1a 100644 --- a/core/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImpl.kt +++ b/core/src/main/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImpl.kt @@ -3,18 +3,24 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.android.instrumentation +package io.opentelemetry.android.internal.instrumentation +import io.opentelemetry.android.instrumentation.AndroidInstrumentation +import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader import java.util.ServiceLoader -internal class AndroidInstrumentationRegistryImpl : AndroidInstrumentationRegistry { +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change + * at any time. + */ +class AndroidInstrumentationLoaderImpl : AndroidInstrumentationLoader { private val instrumentations: MutableMap, AndroidInstrumentation> by lazy { ServiceLoader.load(AndroidInstrumentation::class.java).associateBy { it.javaClass } .toMutableMap() } @Suppress("UNCHECKED_CAST") - override fun get(type: Class): T? { + override fun getByType(type: Class): T? { return instrumentations[type] as? T } @@ -23,7 +29,7 @@ internal class AndroidInstrumentationRegistryImpl : AndroidInstrumentationRegist } @Throws(IllegalStateException::class) - override fun register(instrumentation: AndroidInstrumentation) { + fun registerForTest(instrumentation: AndroidInstrumentation) { if (instrumentation::class.java in instrumentations) { throw IllegalStateException("Instrumentation with type '${instrumentation::class.java}' already exists.") } diff --git a/core/src/test/java/io/opentelemetry/android/OpenTelemetryRumBuilderTest.java b/core/src/test/java/io/opentelemetry/android/OpenTelemetryRumBuilderTest.java index b5f56d680..7ce472b6f 100644 --- a/core/src/test/java/io/opentelemetry/android/OpenTelemetryRumBuilderTest.java +++ b/core/src/test/java/io/opentelemetry/android/OpenTelemetryRumBuilderTest.java @@ -34,8 +34,9 @@ import io.opentelemetry.android.features.diskbuffering.SignalFromDiskExporter; import io.opentelemetry.android.features.diskbuffering.scheduler.ExportScheduleHandler; import io.opentelemetry.android.instrumentation.AndroidInstrumentation; -import io.opentelemetry.android.instrumentation.AndroidInstrumentationRegistry; +import io.opentelemetry.android.instrumentation.AndroidInstrumentationLoader; import io.opentelemetry.android.internal.initialization.InitializationEvents; +import io.opentelemetry.android.internal.instrumentation.AndroidInstrumentationLoaderImpl; import io.opentelemetry.android.internal.services.CacheStorage; import io.opentelemetry.android.internal.services.Preferences; import io.opentelemetry.android.internal.services.ServiceManager; @@ -116,7 +117,7 @@ public void setup() { public void tearDown() throws Exception { SignalFromDiskExporter.resetForTesting(); InitializationEvents.resetForTest(); - AndroidInstrumentationRegistry.resetForTest(); + AndroidInstrumentationLoader.resetForTest(); mocks.close(); } @@ -198,7 +199,9 @@ public void shouldInstallInstrumentation() { SessionIdTimeoutHandler timeoutHandler = mock(); AndroidInstrumentation localInstrumentation = mock(); AndroidInstrumentation classpathInstrumentation = mock(); - AndroidInstrumentationRegistry.get().register(classpathInstrumentation); + AndroidInstrumentationLoaderImpl androidInstrumentationServices = + (AndroidInstrumentationLoaderImpl) AndroidInstrumentationLoader.get(); + androidInstrumentationServices.registerForTest(classpathInstrumentation); new OpenTelemetryRumBuilder(application, buildConfig(), timeoutHandler) .addInstrumentation(localInstrumentation) @@ -216,7 +219,9 @@ public void shouldInstallInstrumentation_excludingClasspathImplsWhenRequestedInC SessionIdTimeoutHandler timeoutHandler = mock(); AndroidInstrumentation localInstrumentation = mock(); AndroidInstrumentation classpathInstrumentation = mock(); - AndroidInstrumentationRegistry.get().register(classpathInstrumentation); + AndroidInstrumentationLoaderImpl androidInstrumentationServices = + (AndroidInstrumentationLoaderImpl) AndroidInstrumentationLoader.get(); + androidInstrumentationServices.registerForTest(classpathInstrumentation); new OpenTelemetryRumBuilder( application, diff --git a/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryTest.kt b/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationLoaderTest.kt similarity index 60% rename from core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryTest.kt rename to core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationLoaderTest.kt index 2a17be5c2..4c209dc98 100644 --- a/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryTest.kt +++ b/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationLoaderTest.kt @@ -8,11 +8,11 @@ package io.opentelemetry.android.instrumentation import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.Test -class AndroidInstrumentationRegistryTest { +class AndroidInstrumentationLoaderTest { @Test fun `Verify singleton`() { - val registry = AndroidInstrumentationRegistry.get() + val registry = AndroidInstrumentationLoader.get() - assertThat(registry).isEqualTo(AndroidInstrumentationRegistry.get()) + assertThat(registry).isEqualTo(AndroidInstrumentationLoader.get()) } } diff --git a/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt b/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt deleted file mode 100644 index b42aa60a3..000000000 --- a/core/src/test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.android.instrumentation - -import android.app.Application -import io.mockk.mockk -import io.opentelemetry.android.OpenTelemetryRum -import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.assertThatThrownBy -import org.junit.jupiter.api.BeforeEach -import org.junit.jupiter.api.Test - -class AndroidInstrumentationRegistryImplTest { - private lateinit var registry: AndroidInstrumentationRegistryImpl - - @BeforeEach - fun setUp() { - registry = AndroidInstrumentationRegistryImpl() - } - - @Test - fun `Find and register implementations available in the classpath when querying an instrumentation`() { - val instrumentation = registry.get(TestAndroidInstrumentation::class.java)!! - - assertThat(instrumentation.installed).isFalse() - - instrumentation.install(mockk(), mockk()) - - assertThat(registry.get(TestAndroidInstrumentation::class.java)!!.installed).isTrue() - } - - @Test - fun `Find and register implementations available in the classpath when querying all instrumentations`() { - val instrumentations = registry.getAll() - - assertThat(instrumentations).hasSize(1) - assertThat(instrumentations.first()).isInstanceOf(TestAndroidInstrumentation::class.java) - } - - @Test - fun `Register instrumentations`() { - val instrumentation = DummyInstrumentation("test") - - registry.register(instrumentation) - - assertThat(registry.get(DummyInstrumentation::class.java)!!.name).isEqualTo("test") - } - - @Test - fun `Register only one instrumentation per type`() { - val instrumentation = DummyInstrumentation("test") - val instrumentation2 = DummyInstrumentation("test2") - - registry.register(instrumentation) - - assertThatThrownBy { - registry.register(instrumentation2) - }.isInstanceOf(IllegalStateException::class.java) - .hasMessage("Instrumentation with type '${DummyInstrumentation::class.java}' already exists.") - } - - private class DummyInstrumentation(val name: String) : AndroidInstrumentation { - override fun install( - application: Application, - openTelemetryRum: OpenTelemetryRum, - ) { - } - } -} diff --git a/core/src/test/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImplTest.kt b/core/src/test/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImplTest.kt new file mode 100644 index 000000000..a8e1a1b60 --- /dev/null +++ b/core/src/test/java/io/opentelemetry/android/internal/instrumentation/AndroidInstrumentationLoaderImplTest.kt @@ -0,0 +1,40 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.android.internal.instrumentation + +import io.mockk.mockk +import io.opentelemetry.android.instrumentation.TestAndroidInstrumentation +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test + +class AndroidInstrumentationLoaderImplTest { + private lateinit var loader: AndroidInstrumentationLoaderImpl + + @BeforeEach + fun setUp() { + loader = AndroidInstrumentationLoaderImpl() + } + + @Test + fun `Find implementations available in the classpath when querying an instrumentation`() { + val instrumentation = loader.getByType(TestAndroidInstrumentation::class.java)!! + + assertThat(instrumentation.installed).isFalse() + + instrumentation.install(mockk(), mockk()) + + assertThat(loader.getByType(TestAndroidInstrumentation::class.java)!!.installed).isTrue() + } + + @Test + fun `Find implementations available in the classpath when querying all instrumentations`() { + val instrumentations = loader.getAll() + + assertThat(instrumentations).hasSize(1) + assertThat(instrumentations.first()).isInstanceOf(TestAndroidInstrumentation::class.java) + } +}