-
Notifications
You must be signed in to change notification settings - Fork 37
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
Changes from 7 commits
d897321
7536eff
1dc5545
b28b844
7843fce
ce1227a
15edcbd
a7a2b08
f022817
9e633ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
/** | ||
* 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. | ||
|
@@ -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? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concerns here about There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
This file was deleted.
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) | ||
} | ||
} |
There was a problem hiding this comment.
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 canget()
from it are services (but they're not, they're instrumentation). We also have theServiceManager
, so puttingService
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.
There was a problem hiding this comment.
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.