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

Instrumentation registry changes #516

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -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.AndroidInstrumentationServices
import io.opentelemetry.android.instrumentation.activity.ActivityLifecycleInstrumentation
import io.opentelemetry.android.instrumentation.anr.AnrInstrumentation
import io.opentelemetry.android.instrumentation.common.ScreenNameExtractor
Expand All @@ -32,49 +32,49 @@ import java.time.Duration
*/

fun OtelRumConfig.setActivityTracerCustomizer(customizer: (Tracer) -> Tracer): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(ActivityLifecycleInstrumentation::class.java)
AndroidInstrumentationServices.getService(ActivityLifecycleInstrumentation::class.java)
?.setTracerCustomizer(customizer)
return this
}

fun OtelRumConfig.setActivityNameExtractor(screenNameExtractor: ScreenNameExtractor): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(ActivityLifecycleInstrumentation::class.java)
AndroidInstrumentationServices.getService(ActivityLifecycleInstrumentation::class.java)
?.setScreenNameExtractor(screenNameExtractor)
return this
}

fun OtelRumConfig.setFragmentTracerCustomizer(customizer: (Tracer) -> Tracer): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(FragmentLifecycleInstrumentation::class.java)
AndroidInstrumentationServices.getService(FragmentLifecycleInstrumentation::class.java)
?.setTracerCustomizer(customizer)
return this
}

fun OtelRumConfig.setFragmentNameExtractor(screenNameExtractor: ScreenNameExtractor): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(FragmentLifecycleInstrumentation::class.java)
AndroidInstrumentationServices.getService(FragmentLifecycleInstrumentation::class.java)
?.setScreenNameExtractor(screenNameExtractor)
return this
}

fun OtelRumConfig.addAnrAttributesExtractor(extractor: AttributesExtractor<Array<StackTraceElement>, Void>): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(AnrInstrumentation::class.java)
AndroidInstrumentationServices.getService(AnrInstrumentation::class.java)
?.addAttributesExtractor(extractor)
return this
}

fun OtelRumConfig.addCrashAttributesExtractor(extractor: AttributesExtractor<CrashDetails, Void>): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(CrashReporterInstrumentation::class.java)
AndroidInstrumentationServices.getService(CrashReporterInstrumentation::class.java)
?.addAttributesExtractor(extractor)
return this
}

fun OtelRumConfig.addNetworkChangeAttributesExtractor(extractor: AttributesExtractor<CurrentNetwork, Void>): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(NetworkChangeInstrumentation::class.java)
AndroidInstrumentationServices.getService(NetworkChangeInstrumentation::class.java)
?.addAttributesExtractor(extractor)
return this
}

fun OtelRumConfig.setSlowRenderingDetectionPollInterval(interval: Duration): OtelRumConfig {
AndroidInstrumentationRegistry.get().get(SlowRenderingInstrumentation::class.java)
AndroidInstrumentationServices.getService(SlowRenderingInstrumentation::class.java)
?.setSlowRenderingDetectionPollInterval(interval)
return this
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.AndroidInstrumentationServices
import io.opentelemetry.android.internal.services.ServiceManager
import io.opentelemetry.android.internal.services.applifecycle.AppLifecycleService
import io.opentelemetry.sdk.OpenTelemetrySdk
Expand Down Expand Up @@ -68,7 +68,7 @@ class SdkPreconfiguredRumBuilder

private fun getInstrumentations(): List<AndroidInstrumentation> {
if (discoverInstrumentations) {
instrumentations.addAll(AndroidInstrumentationRegistry.get().getAll())
instrumentations.addAll(AndroidInstrumentationServices.get().getAll())
}

return instrumentations
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,19 @@

package io.opentelemetry.android.instrumentation

import io.opentelemetry.android.internal.instrumentation.AndroidInstrumentationServicesImpl

/**
* Stores and provides all the available instrumentations.
* Loads and provides Java SPI services of type [AndroidInstrumentation] from the runtime classpath.
*/
interface AndroidInstrumentationRegistry {
interface AndroidInstrumentationServices {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to say so, but I don't love this name. It's leaking out the fact that instrumentations are discovered via AutoService (hence "Services") and suggests that the things you can get() from it are services (but they're not, they're instrumentation). We also have the ServiceManager, so putting Service in the name implies a relationship there, where we might not want one.

Some alternative names:

  • AndroidInstrumentionDiscovery
  • DetectedInstrumentationLoader
  • InstrumentationFinder
  • AndroidInstrumentationLoader
  • AutoDiscoveredAndroidInstrumentationLoader

My preference is for the last one. It's more verbose, but I hope it's very clear in what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always looking for name ideas, as that's the part I struggle the most with, so thanks for your suggestions!

I went with AndroidInstrumentationLoader because I think it's the most concise. Since users might need to access this type to configure some instrumentations I think it's better to keep the name as short as possible.

/**
* Provides a single instrumentation if available.
*
* @param type The type of the instrumentation to retrieve.
* @return The instrumentation instance if available, null otherwise.
*/
fun <T : AndroidInstrumentation> get(type: Class<out T>): T?
fun <T : AndroidInstrumentation> getByType(type: Class<out T>): T?

/**
* Provides all registered instrumentations.
Expand All @@ -24,26 +26,25 @@ interface AndroidInstrumentationRegistry {
*/
fun getAll(): Collection<AndroidInstrumentation>

/**
* 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: AndroidInstrumentationServices? = null

@JvmStatic
fun get(): AndroidInstrumentationRegistry {
fun get(): AndroidInstrumentationServices {
if (instance == null) {
instance = AndroidInstrumentationRegistryImpl()
instance = AndroidInstrumentationServicesImpl()
}
return instance!!
}

/**
* Convenience method for [AndroidInstrumentationServices.getByType].
*/
@JvmStatic
fun <T : AndroidInstrumentation> getService(type: Class<out T>): T? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concerns here about Service. Would prefer to call this getInstrumentation().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it's updated now.

return get().getByType(type)
}

@JvmStatic
fun resetForTest() {
instance = null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.AndroidInstrumentationServices
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 AndroidInstrumentationServicesImpl : AndroidInstrumentationServices {
private val instrumentations: MutableMap<Class<out AndroidInstrumentation>, AndroidInstrumentation> by lazy {
ServiceLoader.load(AndroidInstrumentation::class.java).associateBy { it.javaClass }
.toMutableMap()
}

@Suppress("UNCHECKED_CAST")
override fun <T : AndroidInstrumentation> get(type: Class<out T>): T? {
override fun <T : AndroidInstrumentation> getByType(type: Class<out T>): T? {
return instrumentations[type] as? T
}

Expand All @@ -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.")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.AndroidInstrumentationServices;
import io.opentelemetry.android.internal.initialization.InitializationEvents;
import io.opentelemetry.android.internal.instrumentation.AndroidInstrumentationServicesImpl;
import io.opentelemetry.android.internal.services.CacheStorage;
import io.opentelemetry.android.internal.services.Preferences;
import io.opentelemetry.android.internal.services.ServiceManager;
Expand Down Expand Up @@ -116,7 +117,7 @@ public void setup() {
public void tearDown() throws Exception {
SignalFromDiskExporter.resetForTesting();
InitializationEvents.resetForTest();
AndroidInstrumentationRegistry.resetForTest();
AndroidInstrumentationServices.resetForTest();
mocks.close();
}

Expand Down Expand Up @@ -198,7 +199,9 @@ public void shouldInstallInstrumentation() {
SessionIdTimeoutHandler timeoutHandler = mock();
AndroidInstrumentation localInstrumentation = mock();
AndroidInstrumentation classpathInstrumentation = mock();
AndroidInstrumentationRegistry.get().register(classpathInstrumentation);
AndroidInstrumentationServicesImpl androidInstrumentationServices =
(AndroidInstrumentationServicesImpl) AndroidInstrumentationServices.get();
androidInstrumentationServices.registerForTest(classpathInstrumentation);

new OpenTelemetryRumBuilder(application, buildConfig(), timeoutHandler)
.addInstrumentation(localInstrumentation)
Expand All @@ -216,7 +219,9 @@ public void shouldInstallInstrumentation_excludingClasspathImplsWhenRequestedInC
SessionIdTimeoutHandler timeoutHandler = mock();
AndroidInstrumentation localInstrumentation = mock();
AndroidInstrumentation classpathInstrumentation = mock();
AndroidInstrumentationRegistry.get().register(classpathInstrumentation);
AndroidInstrumentationServicesImpl androidInstrumentationServices =
(AndroidInstrumentationServicesImpl) AndroidInstrumentationServices.get();
androidInstrumentationServices.registerForTest(classpathInstrumentation);

new OpenTelemetryRumBuilder(
application,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 AndroidInstrumentationServicesTest {
@Test
fun `Verify singleton`() {
val registry = AndroidInstrumentationRegistry.get()
val registry = AndroidInstrumentationServices.get()

assertThat(registry).isEqualTo(AndroidInstrumentationRegistry.get())
assertThat(registry).isEqualTo(AndroidInstrumentationServices.get())
}
}
Original file line number Diff line number Diff line change
@@ -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 AndroidInstrumentationServicesImplTest {
private lateinit var registry: AndroidInstrumentationServicesImpl

@BeforeEach
fun setUp() {
registry = AndroidInstrumentationServicesImpl()
}

@Test
fun `Find implementations available in the classpath when querying an instrumentation`() {
val instrumentation = registry.getByType(TestAndroidInstrumentation::class.java)!!

assertThat(instrumentation.installed).isFalse()

instrumentation.install(mockk(), mockk())

assertThat(registry.getByType(TestAndroidInstrumentation::class.java)!!.installed).isTrue()
}

@Test
fun `Find implementations available in the classpath when querying all instrumentations`() {
val instrumentations = registry.getAll()

assertThat(instrumentations).hasSize(1)
assertThat(instrumentations.first()).isInstanceOf(TestAndroidInstrumentation::class.java)
}
}