Instrumentation API part 3 #422
Conversation
* Moving network provider tools to services * Moving network service related tests * Moving network attr init feature to the agent * Making network instrumentation depend on the agent * Initializing CurrentNetworkProvider as service * Clean up config * Created NetworkChangeInstrumentation * Updated network monitor tests * Clean up * Renaming installOn by start * Adding internal class docs * Fixing visible for tests comment * Rename .java to .kt * Removing Carrier.Builder
…tic OpenTelemetryRum.builder function
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| public interface ApplicationStateListener { |
There was a problem hiding this comment.
This has been copied from here. It should have been just moved, but the original interface is still referenced in all the modules that haven't been migrated, so I'm planning to remove it from the original place after all the modules are migrated, otherwise we wouldn't get a green build in the PRs in the meantime.
| /** | ||
| * This class is internal and not for public use. Its APIs are unstable and can change at any time. | ||
| */ | ||
| internal class ApplicationStateWatcher : DefaultLifecycleObserver { |
There was a problem hiding this comment.
This class is meant to replace this one.
| val tracer = sdk.getTracer(OpenTelemetryRum::class.java.simpleName) | ||
| sessionId.setSessionIdChangeListener(SessionIdChangeTracer(tracer)) | ||
|
|
||
| // InstrumentedApplication instrumentedApplication = |
There was a problem hiding this comment.
Will this commented code be restored in a later PR or were they accidentally left?
There was a problem hiding this comment.
It will be replaced in one of the last PRs regarding the instrumentation changes, right now I left it commented to not forget about it 😅
| /** | ||
| * This class is internal and not for public use. Its APIs are unstable and can change at any time. | ||
| */ | ||
| class AppLifecycleService internal constructor( |
There was a problem hiding this comment.
Any particular reason you prefer this particular setting of visibility modifier? It feels like making the entire class internal and change the visibility of the constructor ought to be good enough if we want to contain its instantiation to this module. I think given that we are spinning of the instrumentation from the agent module, this would be even more reasonable.
What do you think?
There was a problem hiding this comment.
There's probably a better way to address this, though at the moment the reasoning behind it is that all services are essentially helper tools that might be reused by our own submodules, such as instrumentations. Right now, this service provides app lifecycle info that's useful for the network change instrumentation, and I also think it will provide useful info for the activity/fragment instrumentations too iirc. We could replicate this service's functionality in other modules though, but for now, given that the instrumentations will have a dependency on the core agent anyway, which is where the AndroidInstrumentation interface lives, it seemed sensible to reuse those tools.
There was a problem hiding this comment.
So this is effectively a private interface for "friends" but because of module boundaries, it's not declared internal? I wonder if we can just make it public but exclude it from docs, state it's not a part of the public interface, and is liable to be changed at any time, perhaps even add an annotation in there fore good measure? We're effectively hiding it, just making it hard to use
There was a problem hiding this comment.
So this is effectively a private interface for "friends" but because of module boundaries, it's not declared internal?
That's right, this is currently the case for all services. This reminded me of a conversation we had in one of the meetings about potentially making them public for all instrumentations. I think this topic is big enough to have its own conversation, so I've created this issue to follow up on it!
There was a problem hiding this comment.
Yeah, pulling these in an internal package, putting comments to say this should not be depended on and can change at any time, and using the annotations like @hide to keep it out of auto generated docs is probably the best way of do this given the way Kotlin visibility works. Maybe we can put the implementation and API into different modules so apps will never have to explicitly pull in modules that contain the implementation, but we don't want to overdo this and make it too hard on ourselves either 😅 Definitely something to talk about for later and not now
|
|
||
| @ExtendWith(MockitoExtension.class) | ||
| class OpenTelemetryRumBuilderTest { | ||
| @RunWith(RobolectricTestRunner.class) |
There was a problem hiding this comment.
You can now use the AndroidJUnit4 runner which supercedes this one, even for Robolectric tests. You can have a look at the docs here.
There was a problem hiding this comment.
Cool! I wasn't aware of it, I'll make the changes.
There was a problem hiding this comment.
Thanks, it's updated now!
# Conflicts: # android-agent/src/main/java/io/opentelemetry/android/internal/services/ServiceManager.kt # android-agent/src/main/java/io/opentelemetry/android/internal/services/ServiceManagerImpl.kt # android-agent/src/test/java/io/opentelemetry/android/OpenTelemetryRumBuilderTest.java # android-agent/src/test/java/io/opentelemetry/android/internal/features/networkattrs/CurrentNetworkAttributesExtractorTest.java # instrumentation/network/src/main/java/io/opentelemetry/android/instrumentation/network/NetworkApplicationListener.java # instrumentation/network/src/main/java/io/opentelemetry/android/instrumentation/network/NetworkChangeInstrumentation.java # instrumentation/network/src/main/java/io/opentelemetry/android/instrumentation/network/NetworkChangeMonitor.java # instrumentation/network/src/test/java/io/opentelemetry/android/instrumentation/network/NetworkChangeMonitorTest.java
This part focuses on the lifecycle observer tool, that's needed in different modules, becoming a service.